[PATCH 1/1] omap3isp: Fix async notifier registration order

[PATCH 1/1] omap3isp: Fix async notifier registration order

[Sakari Ailus]

The async notifier was registered before the v4l2_device was registered and
before the notifier callbacks were set. This could lead to missing the
bound() and complete() callbacks and to attempting to spin_lock() and
uninitialised spin lock.

Also fix unregistering the async notifier in the case of an error --- the
function may not fail anymore after the notifier is registered.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/platform/omap3isp/isp.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 18d0a87..0fa95cc 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2423,10 +2423,6 @@ static int isp_probe(struct platform_device *pdev)
 		ret = isp_of_parse_nodes(&pdev->dev, &isp->notifier);
 		if (ret < 0)
 			return ret;
-		ret = v4l2_async_notifier_register(&isp->v4l2_dev,
-						   &isp->notifier);
-		if (ret)
-			return ret;
 	} else {
 		isp->pdata = pdev->dev.platform_data;
 		isp->syscon = syscon_regmap_lookup_by_pdevname("syscon.0");
@@ -2557,18 +2553,27 @@ static int isp_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto error_iommu;
 
-	isp->notifier.bound = isp_subdev_notifier_bound;
-	isp->notifier.complete = isp_subdev_notifier_complete;
-
 	ret = isp_register_entities(isp);
 	if (ret < 0)
 		goto error_modules;
 
+	if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) {
+		isp->notifier.bound = isp_subdev_notifier_bound;
+		isp->notifier.complete = isp_subdev_notifier_complete;
+
+		ret = v4l2_async_notifier_register(&isp->v4l2_dev,
+						   &isp->notifier);
+		if (ret)
+			goto error_register_entities;
+	}
+
 	isp_core_init(isp, 1);
 	omap3isp_put(isp);
 
 	return 0;
 
+error_register_entities:
+	isp_unregister_entities(isp);
 error_modules:
 	isp_cleanup_modules(isp);
 error_iommu:
-- 
1.7.10.4

Re: [PATCH 1/1] omap3isp: Fix async notifier registration order

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

Hi Sakari,

On Wed, May 20, 2015 at 02:08:05AM +0300, Sakari Ailus wrote:
> The async notifier was registered before the v4l2_device was registered and
> before the notifier callbacks were set. This could lead to missing the
> bound() and complete() callbacks and to attempting to spin_lock() and
> uninitialised spin lock.
> 
> Also fix unregistering the async notifier in the case of an error --- the
> function may not fail anymore after the notifier is registered.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

I already noticed this during my Camera for N900 work and solved it
the same way, so:

Reviewed-By: Sebastian Reichel <sre@kernel.org>

You may want to add a Fixes Tag against the patch implementing the
async notifier support in omap3isp, since its quite easy to run into
the callback problems (at least I did) and the missing resource
freeing (due to EPROBE_AGAIN).

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/1] omap3isp: Fix async notifier registration order

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Wednesday 20 May 2015 01:41:43 Sebastian Reichel wrote:
> Hi Sakari,
> 
> On Wed, May 20, 2015 at 02:08:05AM +0300, Sakari Ailus wrote:
> > The async notifier was registered before the v4l2_device was registered
> > and before the notifier callbacks were set. This could lead to missing the
> > bound() and complete() callbacks and to attempting to spin_lock() and
> > uninitialised spin lock.
> > 
> > Also fix unregistering the async notifier in the case of an error --- the
> > function may not fail anymore after the notifier is registered.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> 
> I already noticed this during my Camera for N900 work and solved it
> the same way, so:
> 
> Reviewed-By: Sebastian Reichel <sre@kernel.org>
> 
> You may want to add a Fixes Tag against the patch implementing the
> async notifier support in omap3isp, since its quite easy to run into
> the callback problems (at least I did) and the missing resource
> freeing (due to EPROBE_AGAIN).

Applied to my tree with the Reviewed-by and Fixes tags.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/1] omap3isp: Fix async notifier registration order

[Laurent Pinchart]

On Wednesday 20 May 2015 09:57:33 Laurent Pinchart wrote:
> On Wednesday 20 May 2015 01:41:43 Sebastian Reichel wrote:
> > On Wed, May 20, 2015 at 02:08:05AM +0300, Sakari Ailus wrote:
> > > The async notifier was registered before the v4l2_device was registered
> > > and before the notifier callbacks were set. This could lead to missing
> > > the bound() and complete() callbacks and to attempting to spin_lock()
> > > and uninitialised spin lock.
> > > 
> > > Also fix unregistering the async notifier in the case of an error ---
> > > the function may not fail anymore after the notifier is registered.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > 
> > I already noticed this during my Camera for N900 work and solved it
> > the same way, so:
> > 
> > Reviewed-By: Sebastian Reichel <sre@kernel.org>
> > 
> > You may want to add a Fixes Tag against the patch implementing the
> > async notifier support in omap3isp, since its quite easy to run into
> > the callback problems (at least I did) and the missing resource
> > freeing (due to EPROBE_AGAIN).
> 
> Applied to my tree with the Reviewed-by and Fixes tags.

I spoke too soon. I think you forgot to remove the 
v4l2_async_notifier_unregister() call from isp_register_entities(). I can fix 
while applying if you agree with the change.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/1] omap3isp: Fix async notifier registration order

[Sakari Ailus]

Hi Laurent,

On Wed, May 20, 2015 at 10:14:53AM +0300, Laurent Pinchart wrote:
> On Wednesday 20 May 2015 09:57:33 Laurent Pinchart wrote:
> > On Wednesday 20 May 2015 01:41:43 Sebastian Reichel wrote:
> > > On Wed, May 20, 2015 at 02:08:05AM +0300, Sakari Ailus wrote:
> > > > The async notifier was registered before the v4l2_device was registered
> > > > and before the notifier callbacks were set. This could lead to missing
> > > > the bound() and complete() callbacks and to attempting to spin_lock()
> > > > and uninitialised spin lock.
> > > > 
> > > > Also fix unregistering the async notifier in the case of an error ---
> > > > the function may not fail anymore after the notifier is registered.
> > > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > 
> > > I already noticed this during my Camera for N900 work and solved it
> > > the same way, so:
> > > 
> > > Reviewed-By: Sebastian Reichel <sre@kernel.org>
> > > 
> > > You may want to add a Fixes Tag against the patch implementing the
> > > async notifier support in omap3isp, since its quite easy to run into
> > > the callback problems (at least I did) and the missing resource
> > > freeing (due to EPROBE_AGAIN).
> > 
> > Applied to my tree with the Reviewed-by and Fixes tags.
> 
> I spoke too soon. I think you forgot to remove the 
> v4l2_async_notifier_unregister() call from isp_register_entities(). I can fix 
> while applying if you agree with the change.

Please do. Thanks!

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 1/1] omap3isp: Fix async notifier registration order

[Timur Tabi]

On Tue, May 19, 2015 at 6:08 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> @@ -2557,18 +2553,27 @@ static int isp_probe(struct platform_device *pdev)
>         if (ret < 0)
>                 goto error_iommu;
>
> -       isp->notifier.bound = isp_subdev_notifier_bound;
> -       isp->notifier.complete = isp_subdev_notifier_complete;
> -
>         ret = isp_register_entities(isp);
>         if (ret < 0)
>                 goto error_modules;
>
> +       if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) {

So I have a question (for Laurent, I guess) that's unrelated to this patch.

Why is the "IS_ENABLED(CONFIG_OF)" important?  If CONFIG_OF is
disabled, isn't pdev->dev.of_node always NULL anyway?

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH 1/1] omap3isp: Fix async notifier registration order

[Laurent Pinchart]

Hi Timur,

On Wednesday 15 July 2015 16:04:01 Timur Tabi wrote:
> On Tue, May 19, 2015 at 6:08 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > @@ -2557,18 +2553,27 @@ static int isp_probe(struct platform_device *pdev)
> >         if (ret < 0)
> >                 goto error_iommu;
> > 
> > -       isp->notifier.bound = isp_subdev_notifier_bound;
> > -       isp->notifier.complete = isp_subdev_notifier_complete;
> > -
> >         ret = isp_register_entities(isp);
> >         if (ret < 0)
> >                 goto error_modules;
> > 
> > +       if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) {
> 
> So I have a question (for Laurent, I guess) that's unrelated to this patch.
> 
> Why is the "IS_ENABLED(CONFIG_OF)" important?  If CONFIG_OF is
> disabled, isn't pdev->dev.of_node always NULL anyway?

IS_ENABLED(CONFIG_OF) can be evaluated at compile time, so it will allow the 
compiler to remove the code block completely when OF support is disabled. 
pdev->dev.of_node is a runtime-only check which would allow the same 
optimization.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/1] omap3isp: Fix async notifier registration order

[Timur Tabi]

Laurent Pinchart wrote:
> IS_ENABLED(CONFIG_OF) can be evaluated at compile time, so it will allow the
> compiler to remove the code block completely when OF support is disabled.
> pdev->dev.of_node is a runtime-only check which would allow the same
> optimization.

Ok, thanks.  I was thinking that there was more to it than just 
optimization, but I guess not.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.