[PATCH v2 1/2] net: davinci_emac: Replace devm_request_irq with request_irq

[Christian Riesch]

In commit 6892b41d9701283085b655c6086fb57a5d63fa47

Author: Lad, Prabhakar <prabhakar.csengg at 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. The
interface is dead until the device is rebooted.

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 <jon at ringle.org>
Signed-off-by: Christian Riesch <christian.riesch at omicron.at>
Cc: Lad, Prabhakar <prabhakar.csengg at gmail.com>
Cc: <stable at vger.kernel.org>
---
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);

[Christian Riesch]

If an error occurs during the initialization in emac_dev_open() (the
driver's ndo_open function), interrupts, DMA descriptors etc. must be freed.
The current rollback code is buggy in several ways.

1) Freeing the interrupts. The current code will not free all interrupts
that were requested by the driver. Furthermore, the code tries to do a
platform_get_resource(priv->pdev, IORESOURCE_IRQ, -1) in its last
iteration.

This patch fixes these bugs.

2) Wrong order of err: and rollback: labels. If the setup of the PHY in
the code fails, the interrupts that have been requested before are
not freed:

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

rollback:
free irqs
err:

This patch brings the code into the correct order.

3) The code calls napi_enable() and emac_int_enable(), but does not
undo both in case of an error.

This patch adds calls of emac_int_disable() and napi_disable() to the
rollback code.

4) 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.

This patch adds code for freeing the DMA descriptors in the case of
an initialization error. This required a modification of
cpdma_ctrl_stop() in davinci_cpdma.c: We must be able to call this
function to free the DMA descriptors while the DMA channels are
in IDLE state (before cpdma_ctlr_start() was called).

Tested on a custom board with the Texas Instruments AM1808.

Signed-off-by: Christian Riesch <christian.riesch at omicron.at>
---
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;
}