| Message ID | 20191209085828.16183-1-hslester96@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx7-mipi-csis: Add the missed v4l2_async_notifier_cleanup in remove | expand |
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 <hslester96@gmail.com> Reviewed-by: Rui Miguel Silva <rmfrfs@gmail.com> ------ 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 >
Steve, Philipp, I'd like one (or both) of you to look over this first. It looks as if the subdev_notifier field of struct csi_state is never used, except by the existing v4l2_async_notifier_unregister() call. If I am right, then the real issue is that that field should be removed. Regards, Hans On 12/11/19 11:59 AM, Rui Miguel Silva wrote: > 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 <hslester96@gmail.com> > > Reviewed-by: Rui Miguel Silva <rmfrfs@gmail.com> > > ------ > 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 <hslester96@gmail.com> > --- > 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 <hslester96@gmail.com> > > --- > > 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 <hslester96@gmail.com> --- > > 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 > devel@linuxdriverproject.org > 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 <hslester96@gmail.com> --- >>> 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 >> devel@linuxdriverproject.org >> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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);
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 <hslester96@gmail.com> --- drivers/staging/media/imx/imx7-mipi-csis.c | 1 + 1 file changed, 1 insertion(+)