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 <[email protected]>
Reported-by: Detlev Zundel <[email protected]>
Tested-by: Detlev Zundel <[email protected]>
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
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
On Mon, Jun 17, 2013 at 2:49 PM, Pavel Machek <[email protected]> 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 <[email protected]>
> Reported-by: Detlev Zundel <[email protected]>
> Tested-by: Detlev Zundel <[email protected]>
>
> 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.
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 <[email protected]>
Reported-by: Detlev Zundel <[email protected]>
---
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
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
On Tue, Jun 18, 2013 at 1:52 PM, Pavel Machek <[email protected]> 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 <[email protected]>
> Reported-by: Detlev Zundel <[email protected]>
>
> ---
>
> 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
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 <[email protected]>
> > Reported-by: Detlev Zundel <[email protected]>
> > @@ -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
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 <[email protected]>
Reported-by: Detlev Zundel <[email protected]>
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
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 <[email protected]>
> Reported-by: Detlev Zundel <[email protected]>
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
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 <[email protected]>
> > Reported-by: Detlev Zundel <[email protected]>
>
> 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