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

Groups > linux.kernel > #1328384 > unrolled thread

[PATCH v2] net: ethernet: davicom: fix devicetree irq resource

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] net: ethernet: davicom: fix devicetree irq resource Robert Jarzmik <robert.jarzmik@free.fr> - 2016-02-06 23:20 +0100
    Re: [PATCH v2] net: ethernet: davicom: fix devicetree irq resource Francois Romieu <romieu@fr.zoreil.com> - 2016-02-07 11:30 +0100
      Re: [PATCH v2] net: ethernet: davicom: fix devicetree irq resource Robert Jarzmik <robert.jarzmik@free.fr> - 2016-02-07 11:50 +0100
        Re: [PATCH v2] net: ethernet: davicom: fix devicetree irq resource Francois Romieu <romieu@fr.zoreil.com> - 2016-02-07 13:40 +0100
    Re: [PATCH v2] net: ethernet: davicom: fix devicetree irq resource Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-02-07 11:50 +0100
      Re: [PATCH v2] net: ethernet: davicom: fix devicetree irq resource Robert Jarzmik <robert.jarzmik@free.fr> - 2016-02-07 12:50 +0100

#1328384 — [PATCH v2] net: ethernet: davicom: fix devicetree irq resource

The dm9000 driver doesn't work in at least one device-tree
configuration, spitting an error message on irq resource :
[    1.062495] dm9000 8000000.ethernet: insufficient resources
[    1.068439] dm9000 8000000.ethernet: not found (-2).
[    1.073451] dm9000: probe of 8000000.ethernet failed with error -2

The reason behind is that the interrupt might be provided by a gpio
controller, not probed when dm9000 is probed, and needing the probe
deferral mechanism to apply.

Currently, the interrupt is directly taken from resources. This patch
changes this to use the more generic platform_get_irq(), which handles
the deferral.

Moreover, since commit Fixes: 7085a7401ba5 ("drivers: platform: parse
IRQ flags from resources"), the interrupt trigger flags are honored in
platform_get_irq(), so remove the needless code in dm9000.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
Acked-by: Marcel Ziswiler <marcel@ziswiler.com>
Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
Since v1: comment style and requested irq test < 0
          David, you should know that Sergei is concerned with the
	  subsystem prefix in the patch subject (too long for him).
---
 drivers/net/ethernet/davicom/dm9000.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
index cf94b72dbacd..2bae5c8c1f85 100644
--- a/drivers/net/ethernet/davicom/dm9000.c
+++ b/drivers/net/ethernet/davicom/dm9000.c
@@ -128,7 +128,6 @@ struct board_info {
 	struct resource *data_res;
 	struct resource	*addr_req;   /* resources requested */
 	struct resource *data_req;
-	struct resource *irq_res;
 
 	int		 irq_wake;
 
@@ -1300,18 +1299,15 @@ static int
 dm9000_open(struct net_device *dev)
 {
 	struct board_info *db = netdev_priv(dev);
-	unsigned long irqflags = db->irq_res->flags & IRQF_TRIGGER_MASK;
+	unsigned long irqflags = 0;
 
 	if (netif_msg_ifup(db))
 		dev_dbg(db->dev, "enabling %s\n", dev->name);
 
-	/* If there is no IRQ type specified, default to something that
-	 * may work, and tell the user that this is a problem */
-
-	if (irqflags == IRQF_TRIGGER_NONE)
-		irqflags = irq_get_trigger_type(dev->irq);
-
-	if (irqflags == IRQF_TRIGGER_NONE)
+	/* If there is no IRQ type specified, tell the user that this is a
+	 * problem
+	 */
+	if (irq_get_trigger_type(dev->irq) == IRQF_TRIGGER_NONE)
 		dev_warn(db->dev, "WARNING: no IRQ resource flags set.\n");
 
 	irqflags |= IRQF_SHARED;
@@ -1500,15 +1496,21 @@ dm9000_probe(struct platform_device *pdev)
 
 	db->addr_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	db->data_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	db->irq_res  = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 
-	if (db->addr_res == NULL || db->data_res == NULL ||
-	    db->irq_res == NULL) {
-		dev_err(db->dev, "insufficient resources\n");
+	if (!db->addr_res || !db->data_res) {
+		dev_err(db->dev, "insufficient resources addr=%p data=%p\n",
+			db->addr_res, db->data_res);
 		ret = -ENOENT;
 		goto out;
 	}
 
+	ndev->irq = platform_get_irq(pdev, 0);
+	if (ndev->irq < 0) {
+		dev_err(db->dev, "interrupt resource unavailable: %d\n",
+			ndev->irq);
+		return ndev->irq;
+	}
+
 	db->irq_wake = platform_get_irq(pdev, 1);
 	if (db->irq_wake >= 0) {
 		dev_dbg(db->dev, "wakeup irq %d\n", db->irq_wake);
@@ -1570,7 +1572,6 @@ dm9000_probe(struct platform_device *pdev)
 
 	/* fill in parameters for net-dev structure */
 	ndev->base_addr = (unsigned long)db->io_addr;
-	ndev->irq	= db->irq_res->start;
 
 	/* ensure at least we have a default set of IO routines */
 	dm9000_set_io(db, iosize);
-- 
2.1.4

[toc] | [next] | [standalone]

#1328463

Robert Jarzmik <robert.jarzmik@free.fr> :
[...]
> diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
> index cf94b72dbacd..2bae5c8c1f85 100644
> --- a/drivers/net/ethernet/davicom/dm9000.c
> +++ b/drivers/net/ethernet/davicom/dm9000.c
[...]
> @@ -1500,15 +1496,21 @@ dm9000_probe(struct platform_device *pdev)
>  
>  	db->addr_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	db->data_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> -	db->irq_res  = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>  
> -	if (db->addr_res == NULL || db->data_res == NULL ||
> -	    db->irq_res == NULL) {
> -		dev_err(db->dev, "insufficient resources\n");
> +	if (!db->addr_res || !db->data_res) {
> +		dev_err(db->dev, "insufficient resources addr=%p data=%p\n",
> +			db->addr_res, db->data_res);
>  		ret = -ENOENT;
>  		goto out;
>  	}
>  
> +	ndev->irq = platform_get_irq(pdev, 0);
> +	if (ndev->irq < 0) {
> +		dev_err(db->dev, "interrupt resource unavailable: %d\n",
> +			ndev->irq);
> +		return ndev->irq;
> +	}
> +

Leak: please set 'ret = ndev->irq;' and proceed to the 'out' label.

If you have some spare time, it would be nice to avoid db->irq_wake leak
on probe failure or driver removal.

-- 
Ueimor

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

#1328477

Francois Romieu <romieu@fr.zoreil.com> writes:

> Robert Jarzmik <robert.jarzmik@free.fr> :
> [...]
>> diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
>> index cf94b72dbacd..2bae5c8c1f85 100644
>> --- a/drivers/net/ethernet/davicom/dm9000.c
>> +++ b/drivers/net/ethernet/davicom/dm9000.c
> [...]
>> @@ -1500,15 +1496,21 @@ dm9000_probe(struct platform_device *pdev)
>>  
>>  	db->addr_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  	db->data_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> -	db->irq_res  = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>  
>> -	if (db->addr_res == NULL || db->data_res == NULL ||
>> -	    db->irq_res == NULL) {
>> -		dev_err(db->dev, "insufficient resources\n");
>> +	if (!db->addr_res || !db->data_res) {
>> +		dev_err(db->dev, "insufficient resources addr=%p data=%p\n",
>> +			db->addr_res, db->data_res);
>>  		ret = -ENOENT;
>>  		goto out;
>>  	}
>>  
>> +	ndev->irq = platform_get_irq(pdev, 0);
>> +	if (ndev->irq < 0) {
>> +		dev_err(db->dev, "interrupt resource unavailable: %d\n",
>> +			ndev->irq);
>> +		return ndev->irq;
>> +	}
>> +
>
> Leak: please set 'ret = ndev->irq;' and proceed to the 'out' label.
Indeed, the etherdev, good catch.

> If you have some spare time, it would be nice to avoid db->irq_wake leak
> on probe failure or driver removal.
Sorry but not in this patch.

I suppose the right patch would be to use devm_*() in the probe function for
ioremaps and request_irqs, which would address this point.

Cheers.

-- 
Robert

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

#1328486

Robert Jarzmik <robert.jarzmik@free.fr> :
> Francois Romieu <romieu@fr.zoreil.com> writes:
[...]
> > If you have some spare time, it would be nice to avoid db->irq_wake leak
> > on probe failure or driver removal.
> Sorry but not in this patch.

Of course. Different topic => different patch.

> I suppose the right patch would be to use devm_*() in the probe function for
> ioremaps and request_irqs, which would address this point.

I'd rather avoid devm_* on netdev.

-- 
Ueimor

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

#1328467

Hello.

On 2/7/2016 1:11 AM, Robert Jarzmik wrote:

> The dm9000 driver doesn't work in at least one device-tree
> configuration, spitting an error message on irq resource :
> [    1.062495] dm9000 8000000.ethernet: insufficient resources
> [    1.068439] dm9000 8000000.ethernet: not found (-2).
> [    1.073451] dm9000: probe of 8000000.ethernet failed with error -2
>
> The reason behind is that the interrupt might be provided by a gpio
> controller, not probed when dm9000 is probed, and needing the probe
> deferral mechanism to apply.
>
> Currently, the interrupt is directly taken from resources. This patch
> changes this to use the more generic platform_get_irq(), which handles
> the deferral.
>
> Moreover, since commit Fixes: 7085a7401ba5 ("drivers: platform: parse

    Fixes: tag here?

> IRQ flags from resources"), the interrupt trigger flags are honored in
> platform_get_irq(), so remove the needless code in dm9000.
>
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> Acked-by: Marcel Ziswiler <marcel@ziswiler.com>
> Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> ---
> Since v1: comment style and requested irq test < 0
>            David, you should know that Sergei is concerned with the
> 	  subsystem prefix in the patch subject (too long for him).
> ---
>   drivers/net/ethernet/davicom/dm9000.c | 29 +++++++++++++++--------------
>   1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
> index cf94b72dbacd..2bae5c8c1f85 100644
> --- a/drivers/net/ethernet/davicom/dm9000.c
> +++ b/drivers/net/ethernet/davicom/dm9000.c
[...]
> @@ -1300,18 +1299,15 @@ static int
>   dm9000_open(struct net_device *dev)
>   {
>   	struct board_info *db = netdev_priv(dev);
> -	unsigned long irqflags = db->irq_res->flags & IRQF_TRIGGER_MASK;
> +	unsigned long irqflags = 0;
>
>   	if (netif_msg_ifup(db))
>   		dev_dbg(db->dev, "enabling %s\n", dev->name);
>
> -	/* If there is no IRQ type specified, default to something that
> -	 * may work, and tell the user that this is a problem */
> -
> -	if (irqflags == IRQF_TRIGGER_NONE)
> -		irqflags = irq_get_trigger_type(dev->irq);
> -
> -	if (irqflags == IRQF_TRIGGER_NONE)
> +	/* If there is no IRQ type specified, tell the user that this is a
> +	 * problem
> +	 */
> +	if (irq_get_trigger_type(dev->irq) == IRQF_TRIGGER_NONE)
>   		dev_warn(db->dev, "WARNING: no IRQ resource flags set.\n");
>
>   	irqflags |= IRQF_SHARED;

    Why not just initialize to IRQF_SHARED?
    But actually you don't need this variable anymore.

> @@ -1500,15 +1496,21 @@ dm9000_probe(struct platform_device *pdev)
>
>   	db->addr_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   	db->data_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> -	db->irq_res  = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>
> -	if (db->addr_res == NULL || db->data_res == NULL ||
> -	    db->irq_res == NULL) {
> -		dev_err(db->dev, "insufficient resources\n");
> +	if (!db->addr_res || !db->data_res) {
> +		dev_err(db->dev, "insufficient resources addr=%p data=%p\n",
> +			db->addr_res, db->data_res);
>   		ret = -ENOENT;
>   		goto out;
>   	}
>
> +	ndev->irq = platform_get_irq(pdev, 0);
> +	if (ndev->irq < 0) {
> +		dev_err(db->dev, "interrupt resource unavailable: %d\n",
> +			ndev->irq);
> +		return ndev->irq;

    As already noted, direct *return* can't be used here.

[...]

MBR, Sergei

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

#1328483

Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes:

>> Moreover, since commit Fixes: 7085a7401ba5 ("drivers: platform: parse
>
>    Fixes: tag here?
Heuh no. It doesn't fix this commit, it uses a feature brought by this commit.

>> +	if (irq_get_trigger_type(dev->irq) == IRQF_TRIGGER_NONE)
>>   		dev_warn(db->dev, "WARNING: no IRQ resource flags set.\n");
>>
>>   	irqflags |= IRQF_SHARED;
>
>    Why not just initialize to IRQF_SHARED?
>    But actually you don't need this variable anymore.
Yeah, makes sense.

Cheers.

-- 
Robert

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web