Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1477418 > unrolled thread

[PATCH 2/4] ARM: common/locomo: remove NO_IRQ check

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/4] ARM: common/locomo: remove NO_IRQ check Arnd Bergmann <arnd@arndb.de> - 2016-09-06 16:00 +0200
    Re: [PATCH 2/4] ARM: common/locomo: remove NO_IRQ check Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-09-06 16:30 +0200
      Re: [PATCH 2/4] ARM: common/locomo: remove NO_IRQ check Arnd Bergmann <arnd@arndb.de> - 2016-09-06 17:00 +0200
        [PATCH v2] ARM: common/locomo: remove NO_IRQ check Arnd Bergmann <arnd@arndb.de> - 2016-09-06 17:30 +0200

#1477418 — [PATCH 2/4] ARM: common/locomo: remove NO_IRQ check

Since commit 489447380a29 ("[PATCH] handle errors returned by
platform_get_irq*()") ten years ago, the locomo driver refuses to
work without an interrupt line passed in its resources, so the
check for NO_IRQ is unnecessary.

We still check the irq_base argument for NO_IRQ, but as both
platforms that use locomo (poodle and collie) provide both
'irq' and 'irq_base', this can be done more consistently
by just checking that both are valid in the probe function
and otherwise returning an error.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm/common/locomo.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/arm/common/locomo.c b/arch/arm/common/locomo.c
index 0e97b4b871f9..81abb04e5254 100644
--- a/arch/arm/common/locomo.c
+++ b/arch/arm/common/locomo.c
@@ -253,8 +253,7 @@ locomo_init_one_child(struct locomo *lchip, struct locomo_dev_info *info)
 		dev->mapbase = 0;
 	dev->length = info->length;
 
-	dev->irq[0] = (lchip->irq_base == NO_IRQ) ?
-			NO_IRQ : lchip->irq_base + info->irq[0];
+	dev->irq[0] = lchip->irq_base + info->irq[0];
 
 	ret = device_register(&dev->dev);
 	if (ret) {
@@ -376,6 +375,9 @@ __locomo_probe(struct device *me, struct resource *mem, int irq)
 	unsigned long r;
 	int i, ret = -ENODEV;
 
+	if (!pdata->irq_base)
+		return ret;
+
 	lchip = kzalloc(sizeof(struct locomo), GFP_KERNEL);
 	if (!lchip)
 		return -ENOMEM;
@@ -387,7 +389,7 @@ __locomo_probe(struct device *me, struct resource *mem, int irq)
 
 	lchip->phys = mem->start;
 	lchip->irq = irq;
-	lchip->irq_base = (pdata) ? pdata->irq_base : NO_IRQ;
+	lchip->irq_base = pdata->irq_base;
 
 	/*
 	 * Map the whole region.  This also maps the
@@ -454,8 +456,7 @@ __locomo_probe(struct device *me, struct resource *mem, int irq)
 	 * The interrupt controller must be initialised before any
 	 * other device to ensure that the interrupts are available.
 	 */
-	if (lchip->irq != NO_IRQ && lchip->irq_base != NO_IRQ)
-		locomo_setup_irq(lchip);
+	locomo_setup_irq(lchip);
 
 	for (i = 0; i < ARRAY_SIZE(locomo_devices); i++)
 		locomo_init_one_child(lchip, &locomo_devices[i]);
@@ -476,9 +477,7 @@ static void __locomo_remove(struct locomo *lchip)
 {
 	device_for_each_child(lchip->dev, NULL, locomo_remove_child);
 
-	if (lchip->irq != NO_IRQ) {
-		irq_set_chained_handler_and_data(lchip->irq, NULL, NULL);
-	}
+	irq_set_chained_handler_and_data(lchip->irq, NULL, NULL);
 
 	iounmap(lchip->base);
 	kfree(lchip);
-- 
2.9.0

[toc] | [next] | [standalone]

#1477461

On Tue, Sep 06, 2016 at 03:53:28PM +0200, Arnd Bergmann wrote:
> Since commit 489447380a29 ("[PATCH] handle errors returned by
> platform_get_irq*()") ten years ago, the locomo driver refuses to
> work without an interrupt line passed in its resources, so the
> check for NO_IRQ is unnecessary.

This description is inaccurate and misleading (it looks like it was
cut'n'pasted from patch 1.)

platform_get_irq() has nothing to do with your change, as your change
is more about the irq_base value passed through platform data, and
not through IRQ resources.

> We still check the irq_base argument for NO_IRQ, but as both
> platforms that use locomo (poodle and collie) provide both
> 'irq' and 'irq_base', this can be done more consistently
> by just checking that both are valid in the probe function
> and otherwise returning an error.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  arch/arm/common/locomo.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/common/locomo.c b/arch/arm/common/locomo.c
> index 0e97b4b871f9..81abb04e5254 100644
> --- a/arch/arm/common/locomo.c
> +++ b/arch/arm/common/locomo.c
> @@ -253,8 +253,7 @@ locomo_init_one_child(struct locomo *lchip, struct locomo_dev_info *info)
>  		dev->mapbase = 0;
>  	dev->length = info->length;
>  
> -	dev->irq[0] = (lchip->irq_base == NO_IRQ) ?
> -			NO_IRQ : lchip->irq_base + info->irq[0];
> +	dev->irq[0] = lchip->irq_base + info->irq[0];
>  
>  	ret = device_register(&dev->dev);
>  	if (ret) {
> @@ -376,6 +375,9 @@ __locomo_probe(struct device *me, struct resource *mem, int irq)
>  	unsigned long r;
>  	int i, ret = -ENODEV;
>  
> +	if (!pdata->irq_base)
> +		return ret;
> +
>  	lchip = kzalloc(sizeof(struct locomo), GFP_KERNEL);
>  	if (!lchip)
>  		return -ENOMEM;
> @@ -387,7 +389,7 @@ __locomo_probe(struct device *me, struct resource *mem, int irq)
>  
>  	lchip->phys = mem->start;
>  	lchip->irq = irq;
> -	lchip->irq_base = (pdata) ? pdata->irq_base : NO_IRQ;
> +	lchip->irq_base = pdata->irq_base;

This removes a NULL pointer check.  Before this change, a NULL pdata
would be accepted and would lead to the interrupts not being setup.
After this change, it results in a NULL pointer deference.

Thankfully, both collie and poodle supply platform data, and are the
only providers of the locomo device.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1477484

On Tuesday, September 6, 2016 3:21:44 PM CEST Russell King - ARM Linux wrote:
> On Tue, Sep 06, 2016 at 03:53:28PM +0200, Arnd Bergmann wrote:
> > Since commit 489447380a29 ("[PATCH] handle errors returned by
> > platform_get_irq*()") ten years ago, the locomo driver refuses to
> > work without an interrupt line passed in its resources, so the
> > check for NO_IRQ is unnecessary.
> 
> This description is inaccurate and misleading (it looks like it was
> cut'n'pasted from patch 1.)
> 
> platform_get_irq() has nothing to do with your change, as your change
> is more about the irq_base value passed through platform data, and
> not through IRQ resources.

It was copied, but this part refers to this hunk

        irq = platform_get_irq(dev, 0);
        if (irq < 0)
                return -ENXIO;

from locomo_probe that was changed in the same patch as
the on in sa1111.c

> > We still check the irq_base argument for NO_IRQ, but as both

where the irq_base comes in.

I'll try to reword this to make it clearer.

> > @@ -387,7 +389,7 @@ __locomo_probe(struct device *me, struct resource *mem, int irq)
> >  
> >  	lchip->phys = mem->start;
> >  	lchip->irq = irq;
> > -	lchip->irq_base = (pdata) ? pdata->irq_base : NO_IRQ;
> > +	lchip->irq_base = pdata->irq_base;
> 
> This removes a NULL pointer check.  Before this change, a NULL pdata
> would be accepted and would lead to the interrupts not being setup.
> After this change, it results in a NULL pointer deference.
> 
> Thankfully, both collie and poodle supply platform data, and are the
> only providers of the locomo device.

Right, that is what I tried to say above. With the check I've added
in __locomo_probe, it would actually get the NULL pointer dereference
earlier than this line. I'll add back that check earlier in the function
and return an error in that case.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1477507 — [PATCH v2] ARM: common/locomo: remove NO_IRQ check

The locomo driver uses two irq numbers, its own IRQ as passed
in the platform resources, and the base number for the interupts
of its child devices, passed in platform_data.

Since commit 489447380a29 ("[PATCH] handle errors returned by
platform_get_irq*()") ten years ago, the locomo driver refuses to
work without an interrupt line passed in its resources, so the
check comparing lchip->irq to NO_IRQ is unnecessary.

We still check the irq_base provided in the platform_data for
NO_IRQ, but as both platforms that use locomo (poodle and collie)
provide an irq_base, this can be done more consistently
by just checking that both are valid in the probe function
and otherwise returning an error.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: add back NULL pointer check, clarify changelog

diff --git a/arch/arm/common/locomo.c b/arch/arm/common/locomo.c
index 0e97b4b871f9..38ca2db0cf12 100644
--- a/arch/arm/common/locomo.c
+++ b/arch/arm/common/locomo.c
@@ -253,8 +253,7 @@ locomo_init_one_child(struct locomo *lchip, struct locomo_dev_info *info)
 		dev->mapbase = 0;
 	dev->length = info->length;
 
-	dev->irq[0] = (lchip->irq_base == NO_IRQ) ?
-			NO_IRQ : lchip->irq_base + info->irq[0];
+	dev->irq[0] = lchip->irq_base + info->irq[0];
 
 	ret = device_register(&dev->dev);
 	if (ret) {
@@ -376,6 +375,9 @@ __locomo_probe(struct device *me, struct resource *mem, int irq)
 	unsigned long r;
 	int i, ret = -ENODEV;
 
+	if (!pdata || !pdata->irq_base)
+		return ret;
+
 	lchip = kzalloc(sizeof(struct locomo), GFP_KERNEL);
 	if (!lchip)
 		return -ENOMEM;
@@ -387,7 +389,7 @@ __locomo_probe(struct device *me, struct resource *mem, int irq)
 
 	lchip->phys = mem->start;
 	lchip->irq = irq;
-	lchip->irq_base = (pdata) ? pdata->irq_base : NO_IRQ;
+	lchip->irq_base = pdata->irq_base;
 
 	/*
 	 * Map the whole region.  This also maps the
@@ -454,8 +456,7 @@ __locomo_probe(struct device *me, struct resource *mem, int irq)
 	 * The interrupt controller must be initialised before any
 	 * other device to ensure that the interrupts are available.
 	 */
-	if (lchip->irq != NO_IRQ && lchip->irq_base != NO_IRQ)
-		locomo_setup_irq(lchip);
+	locomo_setup_irq(lchip);
 
 	for (i = 0; i < ARRAY_SIZE(locomo_devices); i++)
 		locomo_init_one_child(lchip, &locomo_devices[i]);
@@ -476,9 +477,7 @@ static void __locomo_remove(struct locomo *lchip)
 {
 	device_for_each_child(lchip->dev, NULL, locomo_remove_child);
 
-	if (lchip->irq != NO_IRQ) {
-		irq_set_chained_handler_and_data(lchip->irq, NULL, NULL);
-	}
+	irq_set_chained_handler_and_data(lchip->irq, NULL, NULL);
 
 	iounmap(lchip->base);
 	kfree(lchip);

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web