All drivers in imx call v4l2_async_notifier_cleanup() after unregistering
the notifier except this driver.
This should be a miss and we need to add the call to fix it.
Signed-off-by: Chuhong Yuan <[email protected]>
---
drivers/staging/media/imx/imx7-mipi-csis.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 99166afca071..2bfa85bb84e7 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -1105,6 +1105,7 @@ static int mipi_csis_remove(struct platform_device *pdev)
mipi_csis_debugfs_exit(state);
v4l2_async_unregister_subdev(&state->mipi_sd);
v4l2_async_notifier_unregister(&state->subdev_notifier);
+ v4l2_async_notifier_cleanup(&state->subdev_notifier);
pm_runtime_disable(&pdev->dev);
mipi_csis_pm_suspend(&pdev->dev, true);
--
2.24.0
Hi Chuhong,
Thanks for the patch.
On Mon, Dec 09, 2019 at 04:58:28PM +0800, Chuhong Yuan wrote:
> All drivers in imx call v4l2_async_notifier_cleanup() after unregistering
> the notifier except this driver.
> This should be a miss and we need to add the call to fix it.
>
> Signed-off-by: Chuhong Yuan <[email protected]>
Reviewed-by: Rui Miguel Silva <[email protected]>
------
Cheers,
Rui
> ---
> drivers/staging/media/imx/imx7-mipi-csis.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index 99166afca071..2bfa85bb84e7 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -1105,6 +1105,7 @@ static int mipi_csis_remove(struct platform_device *pdev)
> mipi_csis_debugfs_exit(state);
> v4l2_async_unregister_subdev(&state->mipi_sd);
> v4l2_async_notifier_unregister(&state->subdev_notifier);
> + v4l2_async_notifier_cleanup(&state->subdev_notifier);
>
> pm_runtime_disable(&pdev->dev);
> mipi_csis_pm_suspend(&pdev->dev, true);
> --
> 2.24.0
>
On Mon, Dec 09, 2019 at 04:58:28PM +0800, Chuhong Yuan wrote:
> All drivers in imx call v4l2_async_notifier_cleanup() after unregistering
> the notifier except this driver.
> This should be a miss and we need to add the call to fix it.
>
> Signed-off-by: Chuhong Yuan <[email protected]>
> ---
> drivers/staging/media/imx/imx7-mipi-csis.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index 99166afca071..2bfa85bb84e7 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -1105,6 +1105,7 @@ static int mipi_csis_remove(struct platform_device *pdev)
> mipi_csis_debugfs_exit(state);
> v4l2_async_unregister_subdev(&state->mipi_sd);
> v4l2_async_notifier_unregister(&state->subdev_notifier);
> + v4l2_async_notifier_cleanup(&state->subdev_notifier);
>
In this case the "state->subdev_notifier" was never initialized or used
so both v4l2_async_notifier_unregister() and v4l2_async_notifier_cleanup()
are no-ops.
We should just delete "subdev_notifier".
regards,
dan carpenter
On Thu, 2019-12-12 at 14:51 +0300, Dan Carpenter wrote:
> On Mon, Dec 09, 2019 at 04:58:28PM +0800, Chuhong Yuan wrote:
> > All drivers in imx call v4l2_async_notifier_cleanup() after unregistering
> > the notifier except this driver.
> > This should be a miss and we need to add the call to fix it.
> >
> > Signed-off-by: Chuhong Yuan <[email protected]>
> > ---
> > drivers/staging/media/imx/imx7-mipi-csis.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> > index 99166afca071..2bfa85bb84e7 100644
> > --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> > @@ -1105,6 +1105,7 @@ static int mipi_csis_remove(struct platform_device *pdev)
> > mipi_csis_debugfs_exit(state);
> > v4l2_async_unregister_subdev(&state->mipi_sd);
> > v4l2_async_notifier_unregister(&state->subdev_notifier);
> > + v4l2_async_notifier_cleanup(&state->subdev_notifier);
> >
>
> In this case the "state->subdev_notifier" was never initialized or used
> so both v4l2_async_notifier_unregister() and v4l2_async_notifier_cleanup()
> are no-ops.
>
> We should just delete "subdev_notifier".
I agree with Dan and Hans, the subdev_notifier field and the
v4l2_async_notifier_unregister() call should be removed. Since
this issue was there from the start, the patch can be tagged
as fixing commit 7807063b862b.
regards
Philipp
Hi Dan,
Thanks for the inputs.
On Thu, Dec 12, 2019 at 02:51:34PM +0300, Dan Carpenter wrote:
> On Mon, Dec 09, 2019 at 04:58:28PM +0800, Chuhong Yuan wrote:
> > All drivers in imx call v4l2_async_notifier_cleanup() after
> > unregistering the notifier except this driver. This should be a
> > miss and we need to add the call to fix it.
> >
> > Signed-off-by: Chuhong Yuan <[email protected]> ---
> > drivers/staging/media/imx/imx7-mipi-csis.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c
> > b/drivers/staging/media/imx/imx7-mipi-csis.c index
> > 99166afca071..2bfa85bb84e7 100644 ---
> > a/drivers/staging/media/imx/imx7-mipi-csis.c +++
> > b/drivers/staging/media/imx/imx7-mipi-csis.c @@ -1105,6 +1105,7 @@
> > static int mipi_csis_remove(struct platform_device *pdev)
> > mipi_csis_debugfs_exit(state);
> > v4l2_async_unregister_subdev(&state->mipi_sd);
> > v4l2_async_notifier_unregister(&state->subdev_notifier); +
> > v4l2_async_notifier_cleanup(&state->subdev_notifier);
> >
>
> In this case the "state->subdev_notifier" was never initialized or
> used so both v4l2_async_notifier_unregister() and
> v4l2_async_notifier_cleanup() are no-ops.
I have applied this patch on top of Steve's series [0], since by the
timeline I was expecting to be applied before this one, that series
adds a bound notifier, even though, it is not named the same, eheh.
That trigged me to think that this cleanup was correct since a
notifier was initialized in probe.
But as you say, it is a no-ops in the end.
@Steve, that said, it looks that in [0], you will need to add some
unregister and cleanup for the notifiers that you are adding in
several places.
A patch to fix this will follow.
------
Cheers,
Rui
[0]: https://patchwork.kernel.org/project/linux-media/list/?series=207517
>
> We should just delete "subdev_notifier".
>
> regards, dan carpenter
>
> _______________________________________________ devel mailing list
> [email protected]
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
On 12/12/19 11:08 AM, Rui Miguel Silva wrote:
> Hi Dan,
> Thanks for the inputs.
> On Thu, Dec 12, 2019 at 02:51:34PM +0300, Dan Carpenter wrote:
>> On Mon, Dec 09, 2019 at 04:58:28PM +0800, Chuhong Yuan wrote:
>>> All drivers in imx call v4l2_async_notifier_cleanup() after
>>> unregistering the notifier except this driver. This should be a
>>> miss and we need to add the call to fix it.
>>>
>>> Signed-off-by: Chuhong Yuan <[email protected]> ---
>>> drivers/staging/media/imx/imx7-mipi-csis.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c
>>> b/drivers/staging/media/imx/imx7-mipi-csis.c index
>>> 99166afca071..2bfa85bb84e7 100644 ---
>>> a/drivers/staging/media/imx/imx7-mipi-csis.c +++
>>> b/drivers/staging/media/imx/imx7-mipi-csis.c @@ -1105,6 +1105,7 @@
>>> static int mipi_csis_remove(struct platform_device *pdev)
>>> mipi_csis_debugfs_exit(state);
>>> v4l2_async_unregister_subdev(&state->mipi_sd);
>>> v4l2_async_notifier_unregister(&state->subdev_notifier); +
>>> v4l2_async_notifier_cleanup(&state->subdev_notifier);
>>>
>> In this case the "state->subdev_notifier" was never initialized or
>> used so both v4l2_async_notifier_unregister() and
>> v4l2_async_notifier_cleanup() are no-ops.
> I have applied this patch on top of Steve's series [0], since by the
> timeline I was expecting to be applied before this one, that series
> adds a bound notifier, even though, it is not named the same, eheh.
>
> That trigged me to think that this cleanup was correct since a
> notifier was initialized in probe.
>
> But as you say, it is a no-ops in the end.
>
> @Steve, that said, it looks that in [0], you will need to add some
> unregister and cleanup for the notifiers that you are adding in
> several places.
Well, turns out I had failed to notice that an async notifier was
already included in 'struct imx7_csi' as 'subdev_notifier', even though
it was unused. So I ended up creating a duplicate 'notifier'. I'll
cleaning that up in v3 of [0].
Steve
> A patch to fix this will follow.
>
> ------
> Cheers,
> Rui
>
>
>
> [0]: https://patchwork.kernel.org/project/linux-media/list/?series=207517
>
>> We should just delete "subdev_notifier".
>>
>> regards, dan carpenter
>>
>> _______________________________________________ devel mailing list
>> [email protected]
>> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel