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

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Christian Riesch
In commit 6892b41d9701283085b655c6086fb57a5d63fa47

Author: Lad, Prabhakar <[hidden email]>
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 <[hidden email]>
Signed-off-by: Christian Riesch <[hidden email]>
Cc: Lad, Prabhakar <[hidden email]>
Cc: <[hidden email]>
---

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
[hidden email]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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
<[hidden email]> wrote:

> In commit 6892b41d9701283085b655c6086fb57a5d63fa47
>
> Author: Lad, Prabhakar <[hidden email]>
> 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 <[hidden email]>
> Signed-off-by: Christian Riesch <[hidden email]>
> Cc: Lad, Prabhakar <[hidden email]>
> Cc: <[hidden email]>
> ---
>
> 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
> [hidden email]
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source




_______________________________________________
Davinci-linux-open-source mailing list
[hidden email]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Lad, Prabhakar
In reply to this post by Christian Riesch
Hi Christian,

Thanks for the patch.


On Tue, Mar 4, 2014 at 7:37 PM, Christian Riesch
<[hidden email]> wrote:

> In commit 6892b41d9701283085b655c6086fb57a5d63fa47
>
> Author: Lad, Prabhakar <[hidden email]>
> 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 <[hidden email]>
> Signed-off-by: Christian Riesch <[hidden email]>
> Cc: Lad, Prabhakar <[hidden email]>
> Cc: <[hidden email]>

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

 Reported-and-tested-by: Lad, Prabhakar <[hidden email]>

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
>
_______________________________________________
Davinci-linux-open-source mailing list
[hidden email]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Florian Fainelli-2
2014-03-04 9:30 GMT-08:00 Prabhakar Lad <[hidden email]>:

> Hi Christian,
>
> Thanks for the patch.
>
>
> On Tue, Mar 4, 2014 at 7:37 PM, Christian Riesch
> <[hidden email]> wrote:
>> In commit 6892b41d9701283085b655c6086fb57a5d63fa47
>>
>> Author: Lad, Prabhakar <[hidden email]>
>> 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 <[hidden email]>
>> Signed-off-by: Christian Riesch <[hidden email]>
>> Cc: Lad, Prabhakar <[hidden email]>
>> Cc: <[hidden email]>
>
> This patch fixes issue pointed at
> http://comments.gmane.org/gmane.linux.davinci/28135
> tested on Logic PD.
>
>  Reported-and-tested-by: Lad, Prabhakar <[hidden email]>
>
> 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
_______________________________________________
Davinci-linux-open-source mailing list
[hidden email]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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 <[hidden email]>
wrote:

> 2014-03-04 9:30 GMT-08:00 Prabhakar Lad <[hidden email]>:
>> Hi Christian,
>>
>> Thanks for the patch.
>>
>>
>> On Tue, Mar 4, 2014 at 7:37 PM, Christian Riesch
>> <[hidden email]> wrote:
>>> In commit 6892b41d9701283085b655c6086fb57a5d63fa47
>>>
>>> Author: Lad, Prabhakar <[hidden email]>
>>> 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 <[hidden email]>
>>> Signed-off-by: Christian Riesch <[hidden email]>
>>> Cc: Lad, Prabhakar <[hidden email]>
>>> Cc: <[hidden email]>
>>
>> This patch fixes issue pointed at
>> http://comments.gmane.org/gmane.linux.davinci/28135
>> tested on Logic PD.
>>
>>  Reported-and-tested-by: Lad, Prabhakar <[hidden email]>
>>
>> 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 [hidden email]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html




_______________________________________________
Davinci-linux-open-source mailing list
[hidden email]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Loading...