[PATCH] fix UIO with device tree but no assigned interrupt

[PATCH] fix UIO with device tree but no assigned interrupt

[Pavel Machek]

If device is initialized from device tree, but has no interrupt assigned,
uio will still try to request and interrupt old way, fails, and fails registration.
   
This is wrong; don't try initializing irq using platform data if device tree is
available.
    
Signed-off-by: Pavel Machek <pavel@denx.de>
Reported-by: Detlev Zundel <dzu@denx.de>
Tested-by: Detlev Zundel <dzu@denx.de>

diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 8fcc2c7..f709ead 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -213,7 +213,8 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 		goto bad0;
 	}
 
-	if (!uioinfo->irq) {
+	/* interrupts from device tree are already handled above */
+	if (!pdev->dev.of_node && !uioinfo->irq) {
 		ret = platform_get_irq(pdev, 0);
 		if (ret < 0) {
 			dev_err(&pdev->dev, "failed to get IRQ\n");

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] fix UIO with device tree but no assigned interrupt

[Greg KH]

On Mon, Jun 17, 2013 at 03:49:57PM +0200, Pavel Machek wrote:
> If device is initialized from device tree, but has no interrupt assigned,
> uio will still try to request and interrupt old way, fails, and fails registration.
>    
> This is wrong; don't try initializing irq using platform data if device tree is
> available.

Doesn't apply cleanly to my tree, can you redo it against the
char-misc-next branch of my git tree?

thanks,

greg k-h

Re: [PATCH] fix UIO with device tree but no assigned interrupt

[Pavel Machek]

On Mon 2013-06-17 16:46:36, Greg KH wrote:
> On Mon, Jun 17, 2013 at 03:49:57PM +0200, Pavel Machek wrote:
> > If device is initialized from device tree, but has no interrupt assigned,
> > uio will still try to request and interrupt old way, fails, and fails registration.
> >    
> > This is wrong; don't try initializing irq using platform data if device tree is
> > available.
> 
> Doesn't apply cleanly to my tree, can you redo it against the
> char-misc-next branch of my git tree?

Done. I also notice problems with leaking memory, fixed those, too.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] fix UIO with device tree but no assigned interrupt

[Grant Likely]

On Mon, Jun 17, 2013 at 2:49 PM, Pavel Machek <pavel@denx.de> wrote:
> If device is initialized from device tree, but has no interrupt assigned,
> uio will still try to request and interrupt old way, fails, and fails registration.
>
> This is wrong; don't try initializing irq using platform data if device tree is
> available.
>
> Signed-off-by: Pavel Machek <pavel@denx.de>
> Reported-by: Detlev Zundel <dzu@denx.de>
> Tested-by: Detlev Zundel <dzu@denx.de>
>
> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> index 8fcc2c7..f709ead 100644
> --- a/drivers/uio/uio_pdrv_genirq.c
> +++ b/drivers/uio/uio_pdrv_genirq.c
> @@ -213,7 +213,8 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>                 goto bad0;
>         }
>
> -       if (!uioinfo->irq) {
> +       /* interrupts from device tree are already handled above */
> +       if (!pdev->dev.of_node && !uioinfo->irq) {
>                 ret = platform_get_irq(pdev, 0);
>                 if (ret < 0) {
>                         dev_err(&pdev->dev, "failed to get IRQ\n");

This looks backwards. The OF code block is actually duplicating this
block. It would be better to drop the call to platform_get_irq from
the of node processing and let it fall through to this call.

g.

Re: [PATCH] fix UIO with device tree but no assigned interrupt

[Pavel Machek]

If device is initialized from device tree, but has no interrupt
assigned, uio will still try to request and interrupt old way, fails,
and fails registration.

This is wrong; don't try initializing irq using platform data if
device tree is available.

Simplified code based on suggestion by Grant Likely.

Fixed memory leak in "irq can not be registered" error path.

Signed-off-by: Pavel Machek <pavel@denx.de>
Reported-by: Detlev Zundel <dzu@denx.de>

---

Also redone against char-misc-next.

diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index c122bca..a27fd50 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -103,24 +103,16 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 	int i;
 
 	if (pdev->dev.of_node) {
-		int irq;
-
 		/* alloc uioinfo for one device */
 		uioinfo = kzalloc(sizeof(*uioinfo), GFP_KERNEL);
 		if (!uioinfo) {
 			ret = -ENOMEM;
 			dev_err(&pdev->dev, "unable to kmalloc\n");
-			goto bad2;
+			return ret;
 		}
 		uioinfo->name = pdev->dev.of_node->name;
 		uioinfo->version = "devicetree";
-
 		/* Multiple IRQs are not supported */
-		irq = platform_get_irq(pdev, 0);
-		if (irq == -ENXIO)
-			uioinfo->irq = UIO_IRQ_NONE;
-		else
-			uioinfo->irq = irq;
 	}
 
 	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
@@ -146,14 +138,15 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 	priv->flags = 0; /* interrupt is enabled to begin with */
 	priv->pdev = pdev;
 
-	if (!uioinfo->irq) {
-		ret = platform_get_irq(pdev, 0);
-		if (ret < 0) {
-			dev_err(&pdev->dev, "failed to get IRQ\n");
-			goto bad0;
-		}
-		uioinfo->irq = ret;
+	ret = platform_get_irq(pdev, 0);
+	uioinfo->irq = ret;
+	if (ret == -ENXIO && pdev->dev.of_node)
+		uioinfo->irq = UIO_IRQ_NONE;
+	else if (ret < 0) {
+		dev_err(&pdev->dev, "failed to get IRQ\n");
+		goto bad1;
 	}
+
 	uiomem = &uioinfo->mem[0];
 
 	for (i = 0; i < pdev->num_resources; ++i) {
@@ -206,19 +199,19 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 	ret = uio_register_device(&pdev->dev, priv->uioinfo);
 	if (ret) {
 		dev_err(&pdev->dev, "unable to register uio device\n");
-		goto bad1;
+		goto bad2;
 	}
 
 	platform_set_drvdata(pdev, priv);
 	return 0;
+ bad2:
+	pm_runtime_disable(&pdev->dev);
  bad1:
 	kfree(priv);
-	pm_runtime_disable(&pdev->dev);
  bad0:
 	/* kfree uioinfo for OF */
 	if (pdev->dev.of_node)
 		kfree(uioinfo);
- bad2:
 	return ret;
 }
 


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] fix UIO with device tree but no assigned interrupt

[Grant Likely]

On Tue, Jun 18, 2013 at 1:52 PM, Pavel Machek <pavel@denx.de> wrote:
>
> If device is initialized from device tree, but has no interrupt
> assigned, uio will still try to request and interrupt old way, fails,
> and fails registration.
>
> This is wrong; don't try initializing irq using platform data if
> device tree is available.
>
> Simplified code based on suggestion by Grant Likely.
>
> Fixed memory leak in "irq can not be registered" error path.
>
> Signed-off-by: Pavel Machek <pavel@denx.de>
> Reported-by: Detlev Zundel <dzu@denx.de>
>
> ---
>
> Also redone against char-misc-next.
>
> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> index c122bca..a27fd50 100644
> --- a/drivers/uio/uio_pdrv_genirq.c
> +++ b/drivers/uio/uio_pdrv_genirq.c
> @@ -103,24 +103,16 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>         int i;
>
>         if (pdev->dev.of_node) {
> -               int irq;
> -
>                 /* alloc uioinfo for one device */
>                 uioinfo = kzalloc(sizeof(*uioinfo), GFP_KERNEL);
>                 if (!uioinfo) {
>                         ret = -ENOMEM;
>                         dev_err(&pdev->dev, "unable to kmalloc\n");
> -                       goto bad2;
> +                       return ret;
>                 }
>                 uioinfo->name = pdev->dev.of_node->name;
>                 uioinfo->version = "devicetree";
> -
>                 /* Multiple IRQs are not supported */
> -               irq = platform_get_irq(pdev, 0);
> -               if (irq == -ENXIO)
> -                       uioinfo->irq = UIO_IRQ_NONE;
> -               else
> -                       uioinfo->irq = irq;
>         }
>
>         if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> @@ -146,14 +138,15 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>         priv->flags = 0; /* interrupt is enabled to begin with */
>         priv->pdev = pdev;
>
> -       if (!uioinfo->irq) {
> -               ret = platform_get_irq(pdev, 0);
> -               if (ret < 0) {
> -                       dev_err(&pdev->dev, "failed to get IRQ\n");
> -                       goto bad0;
> -               }
> -               uioinfo->irq = ret;
> +       ret = platform_get_irq(pdev, 0);
> +       uioinfo->irq = ret;
> +       if (ret == -ENXIO && pdev->dev.of_node)
> +               uioinfo->irq = UIO_IRQ_NONE;

This short-circuits the platform data use case where uioinfo->irq is
already set. The if (!uioinfo->irq) test is still needed. The original
code looks like it already handles it correctly for both the
platform_data and DT use cases because in the DT case the uioinfo
structure is zeroed by kzalloc().

As an aside, switching to devm_kzalloc() would simplify the unwind path.

> +       else if (ret < 0) {
> +               dev_err(&pdev->dev, "failed to get IRQ\n");
> +               goto bad1;
>         }
> +
>         uiomem = &uioinfo->mem[0];
>
>         for (i = 0; i < pdev->num_resources; ++i) {
> @@ -206,19 +199,19 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
>         ret = uio_register_device(&pdev->dev, priv->uioinfo);
>         if (ret) {
>                 dev_err(&pdev->dev, "unable to register uio device\n");
> -               goto bad1;
> +               goto bad2;
>         }
>
>         platform_set_drvdata(pdev, priv);
>         return 0;
> + bad2:
> +       pm_runtime_disable(&pdev->dev);
>   bad1:
>         kfree(priv);
> -       pm_runtime_disable(&pdev->dev);
>   bad0:
>         /* kfree uioinfo for OF */
>         if (pdev->dev.of_node)
>                 kfree(uioinfo);
> - bad2:
>         return ret;
>  }
>
>
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] fix UIO with device tree but no assigned interrupt

[Pavel Machek]

Hi!

> > If device is initialized from device tree, but has no interrupt
> > assigned, uio will still try to request and interrupt old way, fails,
> > and fails registration.
> >
> > This is wrong; don't try initializing irq using platform data if
> > device tree is available.
> >
> > Simplified code based on suggestion by Grant Likely.
> >
> > Fixed memory leak in "irq can not be registered" error path.
> >
> > Signed-off-by: Pavel Machek <pavel@denx.de>
> > Reported-by: Detlev Zundel <dzu@denx.de>

> > @@ -146,14 +138,15 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
> >         priv->flags = 0; /* interrupt is enabled to begin with */
> >         priv->pdev = pdev;
> >
> > -       if (!uioinfo->irq) {
> > -               ret = platform_get_irq(pdev, 0);
> > -               if (ret < 0) {
> > -                       dev_err(&pdev->dev, "failed to get IRQ\n");
> > -                       goto bad0;
> > -               }
> > -               uioinfo->irq = ret;
> > +       ret = platform_get_irq(pdev, 0);
> > +       uioinfo->irq = ret;
> > +       if (ret == -ENXIO && pdev->dev.of_node)
> > +               uioinfo->irq = UIO_IRQ_NONE;
> 
> This short-circuits the platform data use case where uioinfo->irq is
> already set. The if (!uioinfo->irq) test is still needed. The original
> code looks like it already handles it correctly for both the
> platform_data and DT use cases because in the DT case the uioinfo
> structure is zeroed by kzalloc().

Ok, fixed, thanks!

> As an aside, switching to devm_kzalloc() would simplify the unwind
>   path.

Well, lets leave that for another day...

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] fix UIO with device tree but no assigned interrupt

[Pavel Machek]

If device is initialized from device tree, but has no interrupt
assigned, uio will still try to request and interrupt old way,
fails, and fails registration.

This is wrong; don't try initializing irq using platform data if
device tree is available.

Simplified code based on suggestion by Grant Likely.

Fixed memory leak in "irq can not be registered" error path.

Signed-off-by: Pavel Machek <pavel@denx.de>
Reported-by: Detlev Zundel <dzu@denx.de>

diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index c122bca..d594dd9 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -103,24 +103,16 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 	int i;
 
 	if (pdev->dev.of_node) {
-		int irq;
-
 		/* alloc uioinfo for one device */
 		uioinfo = kzalloc(sizeof(*uioinfo), GFP_KERNEL);
 		if (!uioinfo) {
 			ret = -ENOMEM;
 			dev_err(&pdev->dev, "unable to kmalloc\n");
-			goto bad2;
+			return ret;
 		}
 		uioinfo->name = pdev->dev.of_node->name;
 		uioinfo->version = "devicetree";
-
 		/* Multiple IRQs are not supported */
-		irq = platform_get_irq(pdev, 0);
-		if (irq == -ENXIO)
-			uioinfo->irq = UIO_IRQ_NONE;
-		else
-			uioinfo->irq = irq;
 	}
 
 	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
@@ -148,12 +140,15 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 
 	if (!uioinfo->irq) {
 		ret = platform_get_irq(pdev, 0);
-		if (ret < 0) {
+		uioinfo->irq = ret;
+		if (ret == -ENXIO && pdev->dev.of_node)
+			uioinfo->irq = UIO_IRQ_NONE;
+		else if (ret < 0) {
 			dev_err(&pdev->dev, "failed to get IRQ\n");
-			goto bad0;
+			goto bad1;
 		}
-		uioinfo->irq = ret;
 	}
+
 	uiomem = &uioinfo->mem[0];
 
 	for (i = 0; i < pdev->num_resources; ++i) {
@@ -206,19 +201,19 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
 	ret = uio_register_device(&pdev->dev, priv->uioinfo);
 	if (ret) {
 		dev_err(&pdev->dev, "unable to register uio device\n");
-		goto bad1;
+		goto bad2;
 	}
 
 	platform_set_drvdata(pdev, priv);
 	return 0;
+ bad2:
+	pm_runtime_disable(&pdev->dev);
  bad1:
 	kfree(priv);
-	pm_runtime_disable(&pdev->dev);
  bad0:
 	/* kfree uioinfo for OF */
 	if (pdev->dev.of_node)
 		kfree(uioinfo);
- bad2:
 	return ret;
 }
 


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] fix UIO with device tree but no assigned interrupt

[Greg Kroah-Hartman]

On Tue, Jun 18, 2013 at 04:26:34PM +0200, Pavel Machek wrote:
> 
> If device is initialized from device tree, but has no interrupt
> assigned, uio will still try to request and interrupt old way,
> fails, and fails registration.
> 
> This is wrong; don't try initializing irq using platform data if
> device tree is available.
> 
> Simplified code based on suggestion by Grant Likely.
> 
> Fixed memory leak in "irq can not be registered" error path.
> 
> Signed-off-by: Pavel Machek <pavel@denx.de>
> Reported-by: Detlev Zundel <dzu@denx.de>

I see a ton of patches in this thread, and have no idea which one is
"newest" and correct.

Please resend, in a format that I do not have to edit at all (hint, the
subject line counts), that I can apply properly.

thanks,

greg k-h

Re: [PATCH] fix UIO with device tree but no assigned interrupt

[Pavel Machek]

Hi!

> > If device is initialized from device tree, but has no interrupt
> > assigned, uio will still try to request and interrupt old way,
> > fails, and fails registration.
> > 
> > This is wrong; don't try initializing irq using platform data if
> > device tree is available.
> > 
> > Simplified code based on suggestion by Grant Likely.
> > 
> > Fixed memory leak in "irq can not be registered" error path.
> > 
> > Signed-off-by: Pavel Machek <pavel@denx.de>
> > Reported-by: Detlev Zundel <dzu@denx.de>
> 
> I see a ton of patches in this thread, and have no idea which one is
> "newest" and correct.
> 
> Please resend, in a format that I do not have to edit at all (hint, the
> subject line counts), that I can apply properly.

I resent both patches, hopefully I got subject lines right this time.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html