[PATCH] net: davinci_emac: Replace devm_request_irq with request_irq

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] net: davinci_emac: Replace devm_request_irq with request_irq

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 reverts said commit partially: It changes the driver back
to use request_irq instead of devm_request_irq, puts free_irq back in
place, but keeps the remaining changes of the original patch.

Reported-by: Jon Ringle <[hidden email]>
Signed-off-by: Christian Riesch <[hidden email]>
Cc: Lad, Prabhakar <[hidden email]>
Cc: <[hidden email]>
---
 drivers/net/ethernet/ti/davinci_emac.c |   25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index cd9b164..2514304 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1532,7 +1532,7 @@ static int emac_dev_open(struct net_device *ndev)
  struct device *emac_dev = &ndev->dev;
  u32 cnt;
  struct resource *res;
- int ret;
+ int q, m, ret;
  int i = 0;
  int k = 0;
  struct emac_priv *priv = netdev_priv(ndev);
@@ -1567,8 +1567,7 @@ static int emac_dev_open(struct net_device *ndev)
 
  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))
+ if (request_irq(i, emac_irq, 0, ndev->name, ndev))
  goto rollback;
  }
  k++;
@@ -1641,7 +1640,15 @@ static int emac_dev_open(struct net_device *ndev)
 
 rollback:
 
- dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed");
+ dev_err(emac_dev, "DaVinci EMAC: request_irq() failed");
+
+ for (q = k; k >= 0; k--) {
+ for (m = i; m >= res->start; m--)
+ free_irq(m, ndev);
+ res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k-1);
+ m = res->end;
+ }
+
  ret = -EBUSY;
 err:
  pm_runtime_put(&priv->pdev->dev);
@@ -1659,6 +1666,9 @@ err:
  */
 static int emac_dev_stop(struct net_device *ndev)
 {
+ struct resource *res;
+ int i = 0;
+ int irq_num;
  struct emac_priv *priv = netdev_priv(ndev);
  struct device *emac_dev = &ndev->dev;
 
@@ -1674,6 +1684,13 @@ static int emac_dev_stop(struct net_device *ndev)
  if (priv->phydev)
  phy_disconnect(priv->phydev);
 
+ /* Free IRQ */
+ while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, i))) {
+ for (irq_num = res->start; irq_num <= res->end; irq_num++)
+ free_irq(irq_num, priv->ndev);
+ i++;
+ }
+
  if (netif_msg_drv(priv))
  dev_notice(emac_dev, "DaVinci EMAC: %s stopped\n", ndev->name);
 
--
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
|

Re: [PATCH] net: davinci_emac: Replace devm_request_irq with request_irq

Lad, Prabhakar
Hi Christian,

Thanks for the patch.

On Wed, Mar 5, 2014 at 3:55 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 reverts said commit partially: It changes the driver back
> to use request_irq instead of devm_request_irq, puts free_irq back in
> place, but keeps the remaining changes of the original patch.
>
> Reported-by: Jon Ringle <[hidden email]>
> Signed-off-by: Christian Riesch <[hidden email]>
> Cc: Lad, Prabhakar <[hidden email]>
> Cc: <[hidden email]>

Acked-by: Lad, Prabhakar <[hidden email]>

Regards,
--Prabhakar lad
_______________________________________________
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
|

Re: [PATCH] net: davinci_emac: Replace devm_request_irq with request_irq

Mugunthan V N
In reply to this post by Christian Riesch
On Wednesday 05 March 2014 03:55 PM, Christian Riesch 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 reverts said commit partially: It changes the driver back
> to use request_irq instead of devm_request_irq, puts free_irq back in
> place, but keeps the remaining changes of the original patch.
>
> Reported-by: Jon Ringle <[hidden email]>
> Signed-off-by: Christian Riesch <[hidden email]>
> Cc: Lad, Prabhakar <[hidden email]>
> Cc: <[hidden email]>

Instead of moving back to request irq. can you move the devm interrupt
request code from open to probe so that the issue is fixed, IRQ will be
requested on module insert and IRQ will be free in module remove and
there will not be any failures in devm request irq while repeating
ifup/ifdown and also during module insert/remove.

Regards
Mugunthan V N
_______________________________________________
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
|

Re: [PATCH] net: davinci_emac: Replace devm_request_irq with request_irq

Christian Riesch
--On March 06, 2014 13:57 +0530 Mugunthan V N <[hidden email]> wrote:

> On Wednesday 05 March 2014 03:55 PM, Christian Riesch 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 reverts said commit partially: It changes the driver back
>> to use request_irq instead of devm_request_irq, puts free_irq back in
>> place, but keeps the remaining changes of the original patch.
>>
>> Reported-by: Jon Ringle <[hidden email]>
>> Signed-off-by: Christian Riesch <[hidden email]>
>> Cc: Lad, Prabhakar <[hidden email]>
>> Cc: <[hidden email]>
>
> Instead of moving back to request irq. can you move the devm interrupt
> request code from open to probe so that the issue is fixed, IRQ will be
> requested on module insert and IRQ will be free in module remove and
> there will not be any failures in devm request irq while repeating
> ifup/ifdown and also during module insert/remove.

This is exactly what I tried first, please have a look at my first patch
and the discussion [1]. Florian Fainelli asked me to revert Prabhakar's
patch instead to bring back the "usual situation", "just in case some
uncontrolled interrupt bit triggers an interrupt while the interface is
down for instance..."

Florian Fainelli, Mugunthan VN, which solution is better?

Regards, Christian

[1] http://marc.info/?t=139394310200001&r=1&w=2

>
> Regards
> Mugunthan V N
> _______________________________________________
> 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
|

Re: [PATCH] net: davinci_emac: Replace devm_request_irq with request_irq

Florian Fainelli-2
In reply to this post by Mugunthan V N
2014-03-06 0:51 GMT-08:00 Christian Riesch <[hidden email]>:

> --On March 06, 2014 13:57 +0530 Mugunthan V N <[hidden email]> wrote:
>>
>> On Wednesday 05 March 2014 03:55 PM, Christian Riesch 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 reverts said commit partially: It changes the driver back
>>> to use request_irq instead of devm_request_irq, puts free_irq back in
>>> place, but keeps the remaining changes of the original patch.
>>>
>>> Reported-by: Jon Ringle <[hidden email]>
>>> Signed-off-by: Christian Riesch <[hidden email]>
>>> Cc: Lad, Prabhakar <[hidden email]>
>>> Cc: <[hidden email]>
>>
>>
>> Instead of moving back to request irq. can you move the devm interrupt
>> request code from open to probe so that the issue is fixed, IRQ will be
>> requested on module insert and IRQ will be free in module remove and
>> there will not be any failures in devm request irq while repeating
>> ifup/ifdown and also during module insert/remove.
>
>
> This is exactly what I tried first, please have a look at my first patch and
> the discussion [1]. Florian Fainelli asked me to revert Prabhakar's patch
> instead to bring back the "usual situation", "just in case some uncontrolled
> interrupt bit triggers an interrupt while the interface is down for
> instance..."
>
> Florian Fainelli, Mugunthan VN, which solution is better?

I think the solution that does:

- request_irq() in ndo_open()
- free_irq() in ndo_stop()

is the safest option because it ensure that the various interrupt
lines are masked at the interrupt controller level, and not just at
Ethernet MAC level, and as such gives you more control over what the
hardware does. It also mimics what 99% of the Ethernet drivers do,
which will help identifying those common patterns programmatically.
--
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
|

Re: [PATCH] net: davinci_emac: Replace devm_request_irq with request_irq

David Miller-13
In reply to this post by Christian Riesch
From: Christian Riesch <[hidden email]>
Date: Wed, 5 Mar 2014 11:25:28 +0100

> @@ -1641,7 +1640,15 @@ static int emac_dev_open(struct net_device *ndev)
>  
>  rollback:
>  
> - dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed");
> + dev_err(emac_dev, "DaVinci EMAC: request_irq() failed");
> +
> + for (q = k; k >= 0; k--) {
> + for (m = i; m >= res->start; m--)
> + free_irq(m, ndev);
> + res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k-1);
> + m = res->end;
> + }

This final assignment in the loop of 'm' is unused?

The inner for() loop always started with "m = i;"
_______________________________________________
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
|

Re: [PATCH] net: davinci_emac: Replace devm_request_irq with request_irq

Christian Riesch


On Thursday, March 6, 2014, David Miller <[hidden email]> wrote:
From: Christian Riesch <<a href="javascript:;" onclick="_e(event, &#39;cvml&#39;, &#39;christian.riesch@omicron.at&#39;)">christian.riesch@...>
Date: Wed, 5 Mar 2014 11:25:28 +0100

> @@ -1641,7 +1640,15 @@ static int emac_dev_open(struct net_device *ndev)
>
>  rollback:
>
> -     dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed");
> +     dev_err(emac_dev, "DaVinci EMAC: request_irq() failed");
> +
> +     for (q = k; k >= 0; k--) {
> +             for (m = i; m >= res->start; m--)
> +                     free_irq(m, ndev);
> +             res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k-1);
> +             m = res->end;
> +     }

This final assignment in the loop of 'm' is unused?

The inner for() loop always started with "m = i;"


This should probably read 

i = res->end; 

I just took the old code, without thinking about it. Thanks for catching it.

Christian
 

_______________________________________________
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
|

Re: [PATCH] net: davinci_emac: Replace devm_request_irq with request_irq

Christian Riesch
In reply to this post by David Miller-13
[Sent again, sorry for the HTML mail before.]

--On March 06, 2014 16:57 -0500 David Miller <[hidden email]> wrote:

> From: Christian Riesch <[hidden email]>
> Date: Wed, 5 Mar 2014 11:25:28 +0100
>
>> @@ -1641,7 +1640,15 @@ static int emac_dev_open(struct net_device *ndev)
>>
>>  rollback:
>>
>> - dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed");
>> + dev_err(emac_dev, "DaVinci EMAC: request_irq() failed");
>> +
>> + for (q = k; k >= 0; k--) {
>> + for (m = i; m >= res->start; m--)
>> + free_irq(m, ndev);
>> + res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k-1);
>> + m = res->end;
>> + }
>
> This final assignment in the loop of 'm' is unused?
>
> The inner for() loop always started with "m = i;"

This should probably read 

i = res->end; 

I just took the old code without thinking about it. Thanks for catching it.

Christian
 

_______________________________________________
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
|

Re: [PATCH] net: davinci_emac: Replace devm_request_irq with request_irq

Christian Riesch
In reply to this post by David Miller-13
Hi,

--On March 06, 2014 16:57 -0500 David Miller <[hidden email]> wrote:

> From: Christian Riesch <[hidden email]>
> Date: Wed, 5 Mar 2014 11:25:28 +0100
>
>> @@ -1641,7 +1640,15 @@ static int emac_dev_open(struct net_device *ndev)
>>
>>  rollback:
>>
>> - dev_err(emac_dev, "DaVinci EMAC: devm_request_irq() failed");
>> + dev_err(emac_dev, "DaVinci EMAC: request_irq() failed");
>> +
>> + for (q = k; k >= 0; k--) {
>> + for (m = i; m >= res->start; m--)
>> + free_irq(m, ndev);
>> + res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k-1);
>> + m = res->end;
            ^^^^
So this should definitely read "i = res->end;".

>> + }
>
> This final assignment in the loop of 'm' is unused?
>
> The inner for() loop always started with "m = i;"

But it is more than just that. The entire rollback mechanism of
emac_dev_open is a mess. It just won't work.

1) Freeing the interrupts. Currently, the code tries to do a
platform_get_resource(priv->pdev, IORESOURCE_IRQ, -1) in its last
iteration. To make this work, the code should be something like

for (q = k; q >= 0; q--) {
        res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k-1);
        if (q != k)
                i = res->end;

        for (m = i; m >= res->start; m--)
                free_irq(m, ndev);
}

2) Wrong order of err: and rollback: labels. Currently the function does
something like this:

        pm_runtime_get
        request irq
                if requesting irqs fails, goto rollback
        setup phy
                if phy setup fails, goto err
        return 0

rollback:
        free irqs
err:
        pm_runtime_put

So if PHY initialization fails, the IRQs are never freed. So rollback and
err must be exchanged, and some additonal code must be added to keep the
correct values of k and i for rollback.

3) RX DMA descriptors are not freed in case of an error: Right before
requesting the irqs, the function creates DMA descriptors for the RX
channel. These RX descriptors are never freed when we jump to either
rollback or err. And I think there is also no way to free them as long as
cpdma_ctlr_start() has not been called (which is done between requesting
irqs and phy setup in the code).

Problems 1+2 are easy to solve. Prabhakar, Mugunthan, any ideas for 3? I
think cpsw_ndo_start() in cpsw.c has the same problem. It will also try to
call cpdma_ctlr_stop in error handling before cpdma_ctlr_start was called
in some cases.

Regards, Christian




_______________________________________________
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
|

Re: [PATCH] net: davinci_emac: Replace devm_request_irq with request_irq

Grygorii Strashko
In reply to this post by Florian Fainelli-2
On 03/06/2014 07:16 PM, Florian Fainelli wrote:

> 2014-03-06 0:51 GMT-08:00 Christian Riesch <[hidden email]>:
>> --On March 06, 2014 13:57 +0530 Mugunthan V N <[hidden email]> wrote:
>>>
>>> On Wednesday 05 March 2014 03:55 PM, Christian Riesch 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 reverts said commit partially: It changes the driver back
>>>> to use request_irq instead of devm_request_irq, puts free_irq back in
>>>> place, but keeps the remaining changes of the original patch.
>>>>
>>>> Reported-by: Jon Ringle <[hidden email]>
>>>> Signed-off-by: Christian Riesch <[hidden email]>
>>>> Cc: Lad, Prabhakar <[hidden email]>
>>>> Cc: <[hidden email]>
>>>
>>>
>>> Instead of moving back to request irq. can you move the devm interrupt
>>> request code from open to probe so that the issue is fixed, IRQ will be
>>> requested on module insert and IRQ will be free in module remove and
>>> there will not be any failures in devm request irq while repeating
>>> ifup/ifdown and also during module insert/remove.
>>
>>
>> This is exactly what I tried first, please have a look at my first patch and
>> the discussion [1]. Florian Fainelli asked me to revert Prabhakar's patch
>> instead to bring back the "usual situation", "just in case some uncontrolled
>> interrupt bit triggers an interrupt while the interface is down for
>> instance..."
>>
>> Florian Fainelli, Mugunthan VN, which solution is better?
>
> I think the solution that does:
>
> - request_irq() in ndo_open()
> - free_irq() in ndo_stop()
>
> is the safest option because it ensure that the various interrupt
> lines are masked at the interrupt controller level, and not just at
> Ethernet MAC level, and as such gives you more control over what the
> hardware does. It also mimics what 99% of the Ethernet drivers do,
> which will help identifying those common patterns programmatically.
>

Why are you not using enable_irq()/disable_irq()?
Also, IRQ_NOAUTOEN can be used
  irq_set_status_flags(IRQ_NOAUTOEN);
before [devm_]request_irq().

The normal way is to request resources from .rpobe(), use them and free on .remove().

Actually, I've found at least one example - drivers/net/ethernet/apple/bmac.c

Regards,
- grygorii

 



_______________________________________________
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
|

[PATCH RFC] net: davinci_emac: Fix rollback of emac_dev_open()

Christian Riesch
In reply to this post by Christian Riesch
Signed-off-by: Christian Riesch <[hidden email]>
Cc: Prabhakar Lad <[hidden email]>
Cc: Mugunthan V N <[hidden email]>
Cc: Florian Fainelli <[hidden email]>
---

Hi again,

How about this solution for fixing the rollback of emac_dev_open()?
Especially the change in davinci_cpdma.c, would this break anything?

The patch applies on top of
[PATCH] net: davinci_emac: Replace devm_request_irq with request_irq

Regards,
Christian

 drivers/net/ethernet/ti/davinci_cpdma.c |    4 +--
 drivers/net/ethernet/ti/davinci_emac.c  |   44 ++++++++++++++++++++-----------
 2 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index 364d0c7..88ef270 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -355,7 +355,7 @@ int cpdma_ctlr_stop(struct cpdma_ctlr *ctlr)
  int i;
 
  spin_lock_irqsave(&ctlr->lock, flags);
- if (ctlr->state != CPDMA_STATE_ACTIVE) {
+ if (ctlr->state == CPDMA_STATE_TEARDOWN) {
  spin_unlock_irqrestore(&ctlr->lock, flags);
  return -EINVAL;
  }
@@ -891,7 +891,7 @@ int cpdma_chan_stop(struct cpdma_chan *chan)
  unsigned timeout;
 
  spin_lock_irqsave(&chan->lock, flags);
- if (chan->state != CPDMA_STATE_ACTIVE) {
+ if (chan->state == CPDMA_STATE_TEARDOWN) {
  spin_unlock_irqrestore(&chan->lock, flags);
  return -EINVAL;
  }
diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index 2514304..8f0e69c 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1533,8 +1533,8 @@ static int emac_dev_open(struct net_device *ndev)
  u32 cnt;
  struct resource *res;
  int q, m, ret;
+ int res_num = 0, irq_num = 0;
  int i = 0;
- int k = 0;
  struct emac_priv *priv = netdev_priv(ndev);
 
  pm_runtime_get(&priv->pdev->dev);
@@ -1564,14 +1564,24 @@ static int emac_dev_open(struct net_device *ndev)
  }
 
  /* Request IRQ */
+ while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ,
+    res_num))) {
+ for (irq_num = res->start; irq_num <= res->end; irq_num++) {
+ dev_err(emac_dev, "Request IRQ %d\n", irq_num);
+ if (request_irq(irq_num, emac_irq, 0, ndev->name,
+ ndev)) {
+ dev_err(emac_dev,
+ "DaVinci EMAC: request_irq() failed\n");
+ ret = -EBUSY;
 
- while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) {
- for (i = res->start; i <= res->end; i++) {
- if (request_irq(i, emac_irq, 0, ndev->name, ndev))
  goto rollback;
+ }
  }
- k++;
+ res_num++;
  }
+ /* prepare counters for rollback in case of an error */
+ res_num--;
+ irq_num--;
 
  /* Start/Enable EMAC hardware */
  emac_hw_enable(priv);
@@ -1638,19 +1648,23 @@ static int emac_dev_open(struct net_device *ndev)
 
  return 0;
 
-rollback:
-
- dev_err(emac_dev, "DaVinci EMAC: request_irq() failed");
+err:
+ emac_int_disable(priv);
+ napi_disable(&priv->napi);
 
- for (q = k; k >= 0; k--) {
- for (m = i; m >= res->start; m--)
+rollback:
+ for (q = res_num; q >= 0; q--) {
+ res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, q);
+ /* at the first iteration, irq_num is already set to the
+ * right value
+ */
+ if (q != res_num)
+ irq_num = res->end;
+
+ for (m = irq_num; m >= res->start; m--)
  free_irq(m, ndev);
- res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k-1);
- m = res->end;
  }
-
- ret = -EBUSY;
-err:
+ cpdma_ctlr_stop(priv->dma);
  pm_runtime_put(&priv->pdev->dev);
  return ret;
 }
--
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
|

Re: [PATCH RFC] net: davinci_emac: Fix rollback of emac_dev_open()

Mugunthan V N
On Friday 07 March 2014 07:37 PM, Christian Riesch wrote:

> Signed-off-by: Christian Riesch <[hidden email]>
> Cc: Prabhakar Lad <[hidden email]>
> Cc: Mugunthan V N <[hidden email]>
> Cc: Florian Fainelli <[hidden email]>
> ---
>
> Hi again,
>
> How about this solution for fixing the rollback of emac_dev_open()?
> Especially the change in davinci_cpdma.c, would this break anything?
>
> The patch applies on top of
> [PATCH] net: davinci_emac: Replace devm_request_irq with request_irq
>
> Regards,
> Christian
>
>  drivers/net/ethernet/ti/davinci_cpdma.c |    4 +--
>  drivers/net/ethernet/ti/davinci_emac.c  |   44 ++++++++++++++++++++-----------
>  2 files changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> index 364d0c7..88ef270 100644
> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> @@ -355,7 +355,7 @@ int cpdma_ctlr_stop(struct cpdma_ctlr *ctlr)
>   int i;
>  
>   spin_lock_irqsave(&ctlr->lock, flags);
> - if (ctlr->state != CPDMA_STATE_ACTIVE) {
> + if (ctlr->state == CPDMA_STATE_TEARDOWN) {
>   spin_unlock_irqrestore(&ctlr->lock, flags);
>   return -EINVAL;
>   }
> @@ -891,7 +891,7 @@ int cpdma_chan_stop(struct cpdma_chan *chan)
>   unsigned timeout;
>  
>   spin_lock_irqsave(&chan->lock, flags);
> - if (chan->state != CPDMA_STATE_ACTIVE) {
> + if (chan->state == CPDMA_STATE_TEARDOWN) {
>   spin_unlock_irqrestore(&chan->lock, flags);
>   return -EINVAL;
>   }

Even when in idle mode chan stop should return error.

Regards
Mugunthan V N
_______________________________________________
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
|

Re: [PATCH RFC] net: davinci_emac: Fix rollback of emac_dev_open()

Christian Riesch


--On March 07, 2014 20:15 +0530 Mugunthan V N <[hidden email]> wrote:

> On Friday 07 March 2014 07:37 PM, Christian Riesch wrote:
>> Signed-off-by: Christian Riesch <[hidden email]>
>> Cc: Prabhakar Lad <[hidden email]>
>> Cc: Mugunthan V N <[hidden email]>
>> Cc: Florian Fainelli <[hidden email]>
>> ---
>>
>> Hi again,
>>
>> How about this solution for fixing the rollback of emac_dev_open()?
>> Especially the change in davinci_cpdma.c, would this break anything?
>>
>> The patch applies on top of
>> [PATCH] net: davinci_emac: Replace devm_request_irq with request_irq
>>
>> Regards,
>> Christian
>>
>>  drivers/net/ethernet/ti/davinci_cpdma.c |    4 +--
>>  drivers/net/ethernet/ti/davinci_emac.c  |   44
>>  ++++++++++++++++++++----------- 2 files changed, 31 insertions(+), 17
>>  deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c
>> b/drivers/net/ethernet/ti/davinci_cpdma.c index 364d0c7..88ef270 100644
>> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
>> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
>> @@ -355,7 +355,7 @@ int cpdma_ctlr_stop(struct cpdma_ctlr *ctlr)
>>   int i;
>>
>>   spin_lock_irqsave(&ctlr->lock, flags);
>> - if (ctlr->state != CPDMA_STATE_ACTIVE) {
>> + if (ctlr->state == CPDMA_STATE_TEARDOWN) {
>>   spin_unlock_irqrestore(&ctlr->lock, flags);
>>   return -EINVAL;
>>   }
>> @@ -891,7 +891,7 @@ int cpdma_chan_stop(struct cpdma_chan *chan)
>>   unsigned timeout;
>>
>>   spin_lock_irqsave(&chan->lock, flags);
>> - if (chan->state != CPDMA_STATE_ACTIVE) {
>> + if (chan->state == CPDMA_STATE_TEARDOWN) {
>>   spin_unlock_irqrestore(&chan->lock, flags);
>>   return -EINVAL;
>>   }
>
> Even when in idle mode chan stop should return error.

Can you please explain? I do not see why.

I must be able to call cpdma_ctlr_stop in idle mode to free the rx
descriptors in case ndo_open in davinci_emac.c fails. Any other ideas how
to solve the problem addressed in the rest of the patch?

Thanks, Christian
_______________________________________________
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
|

Re: [PATCH RFC] net: davinci_emac: Fix rollback of emac_dev_open()

Lad, Prabhakar
In reply to this post by Mugunthan V N
Hi Mugunthan,

On Mon, Mar 10, 2014 at 12:46 PM, Christian Riesch
<[hidden email]> wrote:

>
>
> --On March 07, 2014 20:15 +0530 Mugunthan V N <[hidden email]> wrote:
>
>> On Friday 07 March 2014 07:37 PM, Christian Riesch wrote:
>>>
>>> Signed-off-by: Christian Riesch <[hidden email]>
>>> Cc: Prabhakar Lad <[hidden email]>
>>> Cc: Mugunthan V N <[hidden email]>
>>> Cc: Florian Fainelli <[hidden email]>
>>> ---
>>>
>>> Hi again,
>>>
>>> How about this solution for fixing the rollback of emac_dev_open()?
>>> Especially the change in davinci_cpdma.c, would this break anything?
>>>
>>> The patch applies on top of
>>> [PATCH] net: davinci_emac: Replace devm_request_irq with request_irq
>>>
>>> Regards,
>>> Christian
>>>
>>>  drivers/net/ethernet/ti/davinci_cpdma.c |    4 +--
>>>  drivers/net/ethernet/ti/davinci_emac.c  |   44
>>>  ++++++++++++++++++++----------- 2 files changed, 31 insertions(+), 17
>>>  deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c
>>> b/drivers/net/ethernet/ti/davinci_cpdma.c index 364d0c7..88ef270 100644
>>> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
>>> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
>>> @@ -355,7 +355,7 @@ int cpdma_ctlr_stop(struct cpdma_ctlr *ctlr)
>>>         int i;
>>>
>>>         spin_lock_irqsave(&ctlr->lock, flags);
>>> -       if (ctlr->state != CPDMA_STATE_ACTIVE) {
>>> +       if (ctlr->state == CPDMA_STATE_TEARDOWN) {
>>>                 spin_unlock_irqrestore(&ctlr->lock, flags);
>>>                 return -EINVAL;
>>>         }
>>> @@ -891,7 +891,7 @@ int cpdma_chan_stop(struct cpdma_chan *chan)
>>>         unsigned                timeout;
>>>
>>>         spin_lock_irqsave(&chan->lock, flags);
>>> -       if (chan->state != CPDMA_STATE_ACTIVE) {
>>> +       if (chan->state == CPDMA_STATE_TEARDOWN) {
>>>                 spin_unlock_irqrestore(&chan->lock, flags);
>>>                 return -EINVAL;
>>>         }
>>
>>
>> Even when in idle mode chan stop should return error.
>
>
> Can you please explain? I do not see why.
>
> I must be able to call cpdma_ctlr_stop in idle mode to free the rx
> descriptors in case ndo_open in davinci_emac.c fails. Any other ideas how to
> solve the problem addressed in the rest of the patch?
>
Do you have any comments on this patch ? I have tested it on OMAP-L138 evm
and works fine. If you have none comments may be Christian can consolidate this
patch with earlier and repost it.

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