diff mbox

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

Message ID 130fa9cb-7e94-4698-ac3f-f987a03c0604@mary.at.omicron.at (mailing list archive)
State New, archived
Headers show

Commit Message

Christian Riesch March 7, 2014, 2:07 p.m. UTC
Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
Cc: Prabhakar Lad <prabhakar.csengg@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
---

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(-)

Comments

Mugunthan V N March 7, 2014, 2:45 p.m. UTC | #1
On Friday 07 March 2014 07:37 PM, Christian Riesch wrote:
> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
> Cc: Prabhakar Lad <prabhakar.csengg@gmail.com>
> Cc: Mugunthan V N <mugunthanvnm@ti.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> ---
>
> 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
Christian Riesch March 10, 2014, 7:16 a.m. UTC | #2
--On March 07, 2014 20:15 +0530 Mugunthan V N <mugunthanvnm@ti.com> wrote:

> On Friday 07 March 2014 07:37 PM, Christian Riesch wrote:
>> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
>> Cc: Prabhakar Lad <prabhakar.csengg@gmail.com>
>> Cc: Mugunthan V N <mugunthanvnm@ti.com>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>
>> 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
Lad, Prabhakar March 13, 2014, 6:17 p.m. UTC | #3
Hi Mugunthan,

On Mon, Mar 10, 2014 at 12:46 PM, Christian Riesch
<christian.riesch@omicron.at> wrote:
>
>
> --On March 07, 2014 20:15 +0530 Mugunthan V N <mugunthanvnm@ti.com> wrote:
>
>> On Friday 07 March 2014 07:37 PM, Christian Riesch wrote:
>>>
>>> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
>>> Cc: Prabhakar Lad <prabhakar.csengg@gmail.com>
>>> Cc: Mugunthan V N <mugunthanvnm@ti.com>
>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>> ---
>>>
>>> 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
diff mbox

Patch

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;
 }