[PATCH] net: davinci_emac: Move call of devm_request_irq to emac_dev_open

[PATCH] net: davinci_emac: Move call of devm_request_irq to emac_dev_open

[Christian Riesch]

In commit 6892b41d9701283085b655c6086fb57a5d63fa47

Author: Lad, Prabhakar <prabhakar.csengg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date:   Tue Jun 25 21:24:51 2013 +0530
net: davinci: emac: Convert to devm_* api

the call of request_irq is replaced by devm_request_irq and the call
of free_irq is removed. But since interrupts are requested in
emac_dev_open, doing ifconfig up/down on the board requests the
interrupts again each time, causing devm_request_irq to fail.

This patch moves the interrupt requests to emac_dev_open and thus
fixes this regression.

Reported-by: Jon Ringle <jon-7zJyie9iPacdnm+yROfE0A@public.gmane.org>
Signed-off-by: Christian Riesch <christian.riesch-3mrvs1K0uXizZXS1Dc/lvw@public.gmane.org>
Cc: Lad, Prabhakar <prabhakar.csengg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---

Hi,
This is an attempt to fix the bug discussed in
https://lkml.org/lkml/2014/3/4/218

Since I am not really familiar with interrupts I am not sure if this is the right way to
do it. I am looking forward to your comments.

Christian

 drivers/net/ethernet/ti/davinci_emac.c |   32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index cd9b164..97c6036 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1531,10 +1531,8 @@ static int emac_dev_open(struct net_device *ndev)
 {
 	struct device *emac_dev = &ndev->dev;
 	u32 cnt;
-	struct resource *res;
 	int ret;
 	int i = 0;
-	int k = 0;
 	struct emac_priv *priv = netdev_priv(ndev);
 
 	pm_runtime_get(&priv->pdev->dev);
@@ -1563,16 +1561,6 @@ static int emac_dev_open(struct net_device *ndev)
 			break;
 	}
 
-	/* Request IRQ */
-
-	while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) {
-		for (i = res->start; i <= res->end; i++) {
-			if (devm_request_irq(&priv->pdev->dev, i, emac_irq,
-					     0, ndev->name, ndev))
-				goto rollback;
-		}
-		k++;
-	}
 
 	/* Start/Enable EMAC hardware */
 	emac_hw_enable(priv);
@@ -1639,10 +1627,6 @@ static int emac_dev_open(struct net_device *ndev)
 
 	return 0;
 
-rollback:
-
-	dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed");
-	ret = -EBUSY;
 err:
 	pm_runtime_put(&priv->pdev->dev);
 	return ret;
@@ -1839,6 +1823,8 @@ static int davinci_emac_probe(struct platform_device *pdev)
 	struct clk *emac_clk;
 	unsigned long emac_bus_frequency;
 
+	int k = 0;
+	int i = 0;
 
 	/* obtain emac clock from kernel */
 	emac_clk = devm_clk_get(&pdev->dev, NULL);
@@ -1968,11 +1954,25 @@ static int davinci_emac_probe(struct platform_device *pdev)
 			   (void *)priv->emac_base_phys, ndev->irq);
 	}
 
+	/* Request IRQ */
+	while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) {
+		for (i = res->start; i <= res->end; i++) {
+			if (devm_request_irq(&priv->pdev->dev, i, emac_irq,
+					     0, ndev->name, ndev))
+				goto no_irq;
+		}
+		k++;
+	}
+
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_resume(&pdev->dev);
 
 	return 0;
 
+no_irq:
+	dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed");
+	rc = -EBUSY;
+
 no_cpdma_chan:
 	if (priv->txchan)
 		cpdma_chan_destroy(priv->txchan);
-- 
1.7.9.5

Re: [PATCH] net: davinci_emac: Move call of devm_request_irq to emac_dev_open

[Christian Riesch]

Uh, wrong subject, this should of course read

net: davinci_emac: Move call of devm_request_irq to davinci_emac_probe()

--On March 04, 2014 15:07 +0100 Christian Riesch 
<christian.riesch@omicron.at> wrote:

> In commit 6892b41d9701283085b655c6086fb57a5d63fa47
>
> Author: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Date:   Tue Jun 25 21:24:51 2013 +0530
> net: davinci: emac: Convert to devm_* api
>
> the call of request_irq is replaced by devm_request_irq and the call
> of free_irq is removed. But since interrupts are requested in
> emac_dev_open, doing ifconfig up/down on the board requests the
> interrupts again each time, causing devm_request_irq to fail.
>
> This patch moves the interrupt requests to emac_dev_open and thus
> fixes this regression.

And of course: "This patch moves the interrupt requests to 
davinci_emac_probe() and thus
fixes this regression.

I will fix that in the next version of the patch.

Christian

>
> Reported-by: Jon Ringle <jon@ringle.org>
> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
> Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Cc: <stable@vger.kernel.org>
> ---
>
> Hi,
> This is an attempt to fix the bug discussed in
> https://lkml.org/lkml/2014/3/4/218
>
> Since I am not really familiar with interrupts I am not sure if this is
> the right way to do it. I am looking forward to your comments.
>
> Christian
>
>  drivers/net/ethernet/ti/davinci_emac.c |   32
> ++++++++++++++++----------------  1 file changed, 16 insertions(+), 16
> deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c
> b/drivers/net/ethernet/ti/davinci_emac.c index cd9b164..97c6036 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -1531,10 +1531,8 @@ static int emac_dev_open(struct net_device *ndev)
>  {
>  	struct device *emac_dev = &ndev->dev;
>  	u32 cnt;
> -	struct resource *res;
>  	int ret;
>  	int i = 0;
> -	int k = 0;
>  	struct emac_priv *priv = netdev_priv(ndev);
>
>  	pm_runtime_get(&priv->pdev->dev);
> @@ -1563,16 +1561,6 @@ static int emac_dev_open(struct net_device *ndev)
>  			break;
>  	}
>
> -	/* Request IRQ */
> -
> -	while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) {
> -		for (i = res->start; i <= res->end; i++) {
> -			if (devm_request_irq(&priv->pdev->dev, i, emac_irq,
> -					     0, ndev->name, ndev))
> -				goto rollback;
> -		}
> -		k++;
> -	}
>
>  	/* Start/Enable EMAC hardware */
>  	emac_hw_enable(priv);
> @@ -1639,10 +1627,6 @@ static int emac_dev_open(struct net_device *ndev)
>
>  	return 0;
>
> -rollback:
> -
> -	dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed");
> -	ret = -EBUSY;
>  err:
>  	pm_runtime_put(&priv->pdev->dev);
>  	return ret;
> @@ -1839,6 +1823,8 @@ static int davinci_emac_probe(struct
> platform_device *pdev)  	struct clk *emac_clk;
>  	unsigned long emac_bus_frequency;
>
> +	int k = 0;
> +	int i = 0;
>
>  	/* obtain emac clock from kernel */
>  	emac_clk = devm_clk_get(&pdev->dev, NULL);
> @@ -1968,11 +1954,25 @@ static int davinci_emac_probe(struct
> platform_device *pdev)  			   (void *)priv->emac_base_phys, ndev->irq);
>  	}
>
> +	/* Request IRQ */
> +	while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) {
> +		for (i = res->start; i <= res->end; i++) {
> +			if (devm_request_irq(&priv->pdev->dev, i, emac_irq,
> +					     0, ndev->name, ndev))
> +				goto no_irq;
> +		}
> +		k++;
> +	}
> +
>  	pm_runtime_enable(&pdev->dev);
>  	pm_runtime_resume(&pdev->dev);
>
>  	return 0;
>
> +no_irq:
> +	dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed");
> +	rc = -EBUSY;
> +
>  no_cpdma_chan:
>  	if (priv->txchan)
>  		cpdma_chan_destroy(priv->txchan);
> --
> 1.7.9.5
>
> _______________________________________________
> Davinci-linux-open-source mailing list
> Davinci-linux-open-source@linux.davincidsp.com
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

Re: [PATCH] net: davinci_emac: Move call of devm_request_irq to emac_dev_open

[Prabhakar Lad]

Hi Christian,

Thanks for the patch.


On Tue, Mar 4, 2014 at 7:37 PM, Christian Riesch
<christian.riesch@omicron.at> wrote:
> In commit 6892b41d9701283085b655c6086fb57a5d63fa47
>
> Author: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Date:   Tue Jun 25 21:24:51 2013 +0530
> net: davinci: emac: Convert to devm_* api
>
> the call of request_irq is replaced by devm_request_irq and the call
> of free_irq is removed. But since interrupts are requested in
> emac_dev_open, doing ifconfig up/down on the board requests the
> interrupts again each time, causing devm_request_irq to fail.
>
> This patch moves the interrupt requests to emac_dev_open and thus
> fixes this regression.
>
> Reported-by: Jon Ringle <jon@ringle.org>
> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
> Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Cc: <stable@vger.kernel.org>

This patch fixes issue pointed at
http://comments.gmane.org/gmane.linux.davinci/28135
tested on Logic PD.

 Reported-and-tested-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>

Assuming you will respin the patch fixing the header.
> ---
>
> Hi,
> This is an attempt to fix the bug discussed in
> https://lkml.org/lkml/2014/3/4/218
>
> Since I am not really familiar with interrupts I am not sure if this is the right way to
> do it. I am looking forward to your comments.
>
Looks OK.

Thanks,
--Prabhakar Lad


> Christian
>
>  drivers/net/ethernet/ti/davinci_emac.c |   32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index cd9b164..97c6036 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -1531,10 +1531,8 @@ static int emac_dev_open(struct net_device *ndev)
>  {
>         struct device *emac_dev = &ndev->dev;
>         u32 cnt;
> -       struct resource *res;
>         int ret;
>         int i = 0;
> -       int k = 0;
>         struct emac_priv *priv = netdev_priv(ndev);
>
>         pm_runtime_get(&priv->pdev->dev);
> @@ -1563,16 +1561,6 @@ static int emac_dev_open(struct net_device *ndev)
>                         break;
>         }
>
> -       /* Request IRQ */
> -
> -       while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) {
> -               for (i = res->start; i <= res->end; i++) {
> -                       if (devm_request_irq(&priv->pdev->dev, i, emac_irq,
> -                                            0, ndev->name, ndev))
> -                               goto rollback;
> -               }
> -               k++;
> -       }
>
>         /* Start/Enable EMAC hardware */
>         emac_hw_enable(priv);
> @@ -1639,10 +1627,6 @@ static int emac_dev_open(struct net_device *ndev)
>
>         return 0;
>
> -rollback:
> -
> -       dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed");
> -       ret = -EBUSY;
>  err:
>         pm_runtime_put(&priv->pdev->dev);
>         return ret;
> @@ -1839,6 +1823,8 @@ static int davinci_emac_probe(struct platform_device *pdev)
>         struct clk *emac_clk;
>         unsigned long emac_bus_frequency;
>
> +       int k = 0;
> +       int i = 0;
>
>         /* obtain emac clock from kernel */
>         emac_clk = devm_clk_get(&pdev->dev, NULL);
> @@ -1968,11 +1954,25 @@ static int davinci_emac_probe(struct platform_device *pdev)
>                            (void *)priv->emac_base_phys, ndev->irq);
>         }
>
> +       /* Request IRQ */
> +       while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) {
> +               for (i = res->start; i <= res->end; i++) {
> +                       if (devm_request_irq(&priv->pdev->dev, i, emac_irq,
> +                                            0, ndev->name, ndev))
> +                               goto no_irq;
> +               }
> +               k++;
> +       }
> +
>         pm_runtime_enable(&pdev->dev);
>         pm_runtime_resume(&pdev->dev);
>
>         return 0;
>
> +no_irq:
> +       dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed");
> +       rc = -EBUSY;
> +
>  no_cpdma_chan:
>         if (priv->txchan)
>                 cpdma_chan_destroy(priv->txchan);
> --
> 1.7.9.5
>

Re: [PATCH] net: davinci_emac: Move call of devm_request_irq to emac_dev_open

[Florian Fainelli]

2014-03-04 9:30 GMT-08:00 Prabhakar Lad <prabhakar.csengg@gmail.com>:
> Hi Christian,
>
> Thanks for the patch.
>
>
> On Tue, Mar 4, 2014 at 7:37 PM, Christian Riesch
> <christian.riesch@omicron.at> wrote:
>> In commit 6892b41d9701283085b655c6086fb57a5d63fa47
>>
>> Author: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>> Date:   Tue Jun 25 21:24:51 2013 +0530
>> net: davinci: emac: Convert to devm_* api
>>
>> the call of request_irq is replaced by devm_request_irq and the call
>> of free_irq is removed. But since interrupts are requested in
>> emac_dev_open, doing ifconfig up/down on the board requests the
>> interrupts again each time, causing devm_request_irq to fail.
>>
>> This patch moves the interrupt requests to emac_dev_open and thus
>> fixes this regression.
>>
>> Reported-by: Jon Ringle <jon@ringle.org>
>> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
>> Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>> Cc: <stable@vger.kernel.org>
>
> This patch fixes issue pointed at
> http://comments.gmane.org/gmane.linux.davinci/28135
> tested on Logic PD.
>
>  Reported-and-tested-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>
> Assuming you will respin the patch fixing the header.
>> ---
>>
>> Hi,
>> This is an attempt to fix the bug discussed in
>> https://lkml.org/lkml/2014/3/4/218
>>
>> Since I am not really familiar with interrupts I am not sure if this is the right way to
>> do it. I am looking forward to your comments.
>>
> Looks OK.

I think a plain revert of Prabhakar patch would bring us in an "usual"
situation where the drivers's ndo_open():

- masks all interrupts bits at the Ethernet MAC level
- calls request_irq()
- unmask relevant interrupts bits at the Ethernet Mac level

just in case some uncontrolled interrupt bit triggers an interrupt
while the interface is down for instance...

On a wider note, it would be good to get some tool to notify people
that attempting to replace devm_request_irq() in a network driver's
ndo_open() function is not going to produce the expected results as
the two are not strictly identical.
-- 
Florian

Re: [PATCH] net: davinci_emac: Move call of devm_request_irq to emac_dev_open

[Christian Riesch]

--On March 04, 2014 09:53 -0800 Florian Fainelli <f.fainelli@gmail.com> 
wrote:

> 2014-03-04 9:30 GMT-08:00 Prabhakar Lad <prabhakar.csengg@gmail.com>:
>> Hi Christian,
>>
>> Thanks for the patch.
>>
>>
>> On Tue, Mar 4, 2014 at 7:37 PM, Christian Riesch
>> <christian.riesch@omicron.at> wrote:
>>> In commit 6892b41d9701283085b655c6086fb57a5d63fa47
>>>
>>> Author: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>>> Date:   Tue Jun 25 21:24:51 2013 +0530
>>> net: davinci: emac: Convert to devm_* api
>>>
>>> the call of request_irq is replaced by devm_request_irq and the call
>>> of free_irq is removed. But since interrupts are requested in
>>> emac_dev_open, doing ifconfig up/down on the board requests the
>>> interrupts again each time, causing devm_request_irq to fail.
>>>
>>> This patch moves the interrupt requests to emac_dev_open and thus
>>> fixes this regression.
>>>
>>> Reported-by: Jon Ringle <jon@ringle.org>
>>> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
>>> Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>>> Cc: <stable@vger.kernel.org>
>>
>> This patch fixes issue pointed at
>> http://comments.gmane.org/gmane.linux.davinci/28135
>> tested on Logic PD.
>>
>>  Reported-and-tested-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>>
>> Assuming you will respin the patch fixing the header.
>>> ---
>>>
>>> Hi,
>>> This is an attempt to fix the bug discussed in
>>> https://lkml.org/lkml/2014/3/4/218
>>>
>>> Since I am not really familiar with interrupts I am not sure if this is
>>> the right way to do it. I am looking forward to your comments.
>>>
>> Looks OK.
>
> I think a plain revert of Prabhakar patch would bring us in an "usual"
> situation where the drivers's ndo_open():
>
> - masks all interrupts bits at the Ethernet MAC level
> - calls request_irq()
> - unmask relevant interrupts bits at the Ethernet Mac level
>
> just in case some uncontrolled interrupt bit triggers an interrupt
> while the interface is down for instance...

I am fine with either solution and I will be happy to prepare a patch if we 
agree that we should revert the patch. What was the reason for commit 
6892b41d9701283085b655c6086fb57a5d63fa47 in the first place? Does it fix a 
problem?

Should the full commit 6892b41d9701283085b655c6086fb57a5d63fa47 be 
reverted? Actually it consists of two parts, the first part replaces 
request_irq with devm_request_irq and throws out free_irq, the second part 
replaces devm_request_mem_region/devm_ioremap with devm_ioremap_resource. 
The first part causes the problem. Shall we keep the second part or shall 
this be reverted as well?

Christian


>
> On a wider note, it would be good to get some tool to notify people
> that attempting to replace devm_request_irq() in a network driver's
> ndo_open() function is not going to produce the expected results as
> the two are not strictly identical.
> --
> Florian
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html