Patchwork [3/5] net/cpsw: don't rely on netif_running() to check which device is active

login
register
mail settings
Submitter Sebastian Siewior
Date April 17, 2013, 9:52 p.m.
Message ID <1366235536-15744-4-git-send-email-bigeasy@linutronix.de>
Download mbox | patch
Permalink /patch/237370/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Sebastian Siewior - April 17, 2013, 9:52 p.m.
netif_running() reports false before even the ->ndo_stop() callback is
called. That means if one executes "ifconfig down" and the system
receives an interrupt before the interrupt source has been disabled we
hang for always for two reasons:
- we never disable the interrupt source because devices claim to be
  already inactive (or non-present)
- since the ISR always reports IRQ_HANDLED the line is never deactivated
  because it looks like the ISR feels respsonsible.

This patch introduces now the ->active field which is set/cleared in
ndo_open / ndo_stop.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/ethernet/ti/cpsw.c |   26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)
Mugunthan V N - April 18, 2013, 11:50 a.m.
On 4/18/2013 3:22 AM, Sebastian Andrzej Siewior wrote:
> netif_running() reports false before even the ->ndo_stop() callback is
> called. That means if one executes "ifconfig down" and the system
> receives an interrupt before the interrupt source has been disabled we
> hang for always for two reasons:
> - we never disable the interrupt source because devices claim to be
>    already inactive (or non-present)
> - since the ISR always reports IRQ_HANDLED the line is never deactivated
>    because it looks like the ISR feels respsonsible.
>
> This patch introduces now the ->active field which is set/cleared in
> ndo_open / ndo_stop.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>   drivers/net/ethernet/ti/cpsw.c |   26 +++++++++++++++++---------
>   1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 3b22a36..c32780d 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -341,6 +341,7 @@ struct cpsw_priv {
>   	int				host_port;
>   	struct clk			*clk;
>   	u8				mac_addr[ETH_ALEN];
> +	u8				active;
Frame work is providing this feature then why you need to have additional
book keeping in device private?
>   	struct cpsw_slave		*slaves;
>   	struct cpdma_ctlr		*dma;
>   	struct cpdma_chan		*txch, *rxch;
> @@ -511,19 +512,24 @@ static irqreturn_t cpsw_interrupt(int irq, void *dev_id)
>   {
>   	struct cpsw_priv *priv = dev_id;
>   
> -	if (likely(netif_running(priv->ndev))) {
> +	if (priv->active) {
>   		cpsw_intr_disable(priv);
>   		cpsw_disable_irq(priv);
>   		napi_schedule(&priv->napi);
> -	} else {
> -		priv = cpsw_get_slave_priv(priv, 1);
> -		if (likely(priv) && likely(netif_running(priv->ndev))) {
> -			cpsw_intr_disable(priv);
> -			cpsw_disable_irq(priv);
> -			napi_schedule(&priv->napi);
> -		}
> +		return IRQ_HANDLED;
> +	}
> +
> +	priv = cpsw_get_slave_priv(priv, 1);
> +	if (!priv)
> +		return IRQ_NONE;
> +
> +	if (priv->active) {
> +		cpsw_intr_disable(priv);
> +		cpsw_disable_irq(priv);
> +		napi_schedule(&priv->napi);
> +		return IRQ_HANDLED;
>   	}
> -	return IRQ_HANDLED;
> +	return IRQ_NONE;
>   }
>   
>   static int cpsw_poll(struct napi_struct *napi, int budget)
> @@ -937,6 +943,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
>   		cpsw_set_coalesce(ndev, &coal);
>   	}
>   
> +	priv->active = 1;
>   	cpdma_ctlr_start(priv->dma);
>   	cpsw_intr_enable(priv);
>   	napi_enable(&priv->napi);
> @@ -980,6 +987,7 @@ static int cpsw_ndo_stop(struct net_device *ndev)
>   	pm_runtime_put_sync(&priv->pdev->dev);
>   	if (priv->data.dual_emac)
>   		priv->slaves[priv->emac_port].open_stat = false;
> +	priv->active = 0;
>   	return 0;
>   }
>   

Regards
Mugunthan V N
--
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
Sebastian Siewior - April 18, 2013, 12:10 p.m.
On 04/18/2013 01:50 PM, Mugunthan V N wrote:

>> diff --git a/drivers/net/ethernet/ti/cpsw.c
>> b/drivers/net/ethernet/ti/cpsw.c
>> index 3b22a36..c32780d 100644
>> --- a/drivers/net/ethernet/ti/cpsw.c
>> +++ b/drivers/net/ethernet/ti/cpsw.c
>> @@ -341,6 +341,7 @@ struct cpsw_priv {
>>       int                host_port;
>>       struct clk            *clk;
>>       u8                mac_addr[ETH_ALEN];
>> +    u8                active;
> Frame work is providing this feature then why you need to have additional
> book keeping in device private?

Which function? I more than happy to use it.

> 
> Regards
> Mugunthan V N

Sebastian
--
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
Mugunthan V N - April 19, 2013, 10:35 a.m.
On 4/18/2013 5:40 PM, Sebastian Andrzej Siewior wrote:
> On 04/18/2013 01:50 PM, Mugunthan V N wrote:
>
>>> diff --git a/drivers/net/ethernet/ti/cpsw.c
>>> b/drivers/net/ethernet/ti/cpsw.c
>>> index 3b22a36..c32780d 100644
>>> --- a/drivers/net/ethernet/ti/cpsw.c
>>> +++ b/drivers/net/ethernet/ti/cpsw.c
>>> @@ -341,6 +341,7 @@ struct cpsw_priv {
>>>        int                host_port;
>>>        struct clk            *clk;
>>>        u8                mac_addr[ETH_ALEN];
>>> +    u8                active;
>> Frame work is providing this feature then why you need to have additional
>> book keeping in device private?
> Which function? I more than happy to use it.
>
The same netif_running(). But why you are telling not to rely on this API?
device state is updated immediately after successful ndo_open.

Regards
Mugunthan V N
--
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
Sergei Shtylyov - April 19, 2013, 1:10 p.m.
Hello.

On 18-04-2013 1:52, Sebastian Andrzej Siewior wrote:

> netif_running() reports false before even the ->ndo_stop() callback is
> called. That means if one executes "ifconfig down" and the system
> receives an interrupt before the interrupt source has been disabled we
> hang for always for two reasons:
> - we never disable the interrupt source because devices claim to be
>    already inactive (or non-present)
> - since the ISR always reports IRQ_HANDLED the line is never deactivated
>    because it looks like the ISR feels respsonsible.

> This patch introduces now the ->active field which is set/cleared in
> ndo_open / ndo_stop.

> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>   drivers/net/ethernet/ti/cpsw.c |   26 +++++++++++++++++---------
>   1 file changed, 17 insertions(+), 9 deletions(-)

> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 3b22a36..c32780d 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -341,6 +341,7 @@ struct cpsw_priv {
>   	int				host_port;
>   	struct clk			*clk;
>   	u8				mac_addr[ETH_ALEN];
> +	u8				active;

    *bool* seems to fit better here.

WBR, Sergei

--
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
Sebastian Siewior - April 22, 2013, 8:33 a.m.
On 04/19/2013 03:10 PM, Sergei Shtylyov wrote:
Hello Sergei,

>> --- a/drivers/net/ethernet/ti/cpsw.c
>> +++ b/drivers/net/ethernet/ti/cpsw.c
>> @@ -341,6 +341,7 @@ struct cpsw_priv {
>>       int                host_port;
>>       struct clk            *clk;
>>       u8                mac_addr[ETH_ALEN];
>> +    u8                active;
> 
>    *bool* seems to fit better here.

yes but would require the compiler to add an extra pad between u8 and
bool and another between bool and the pointer. That way it does not
cause the struct to grow :) Anyway, v2 is out without the need for it.

> 
> WBR, Sergei
> 

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

Patch

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 3b22a36..c32780d 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -341,6 +341,7 @@  struct cpsw_priv {
 	int				host_port;
 	struct clk			*clk;
 	u8				mac_addr[ETH_ALEN];
+	u8				active;
 	struct cpsw_slave		*slaves;
 	struct cpdma_ctlr		*dma;
 	struct cpdma_chan		*txch, *rxch;
@@ -511,19 +512,24 @@  static irqreturn_t cpsw_interrupt(int irq, void *dev_id)
 {
 	struct cpsw_priv *priv = dev_id;
 
-	if (likely(netif_running(priv->ndev))) {
+	if (priv->active) {
 		cpsw_intr_disable(priv);
 		cpsw_disable_irq(priv);
 		napi_schedule(&priv->napi);
-	} else {
-		priv = cpsw_get_slave_priv(priv, 1);
-		if (likely(priv) && likely(netif_running(priv->ndev))) {
-			cpsw_intr_disable(priv);
-			cpsw_disable_irq(priv);
-			napi_schedule(&priv->napi);
-		}
+		return IRQ_HANDLED;
+	}
+
+	priv = cpsw_get_slave_priv(priv, 1);
+	if (!priv)
+		return IRQ_NONE;
+
+	if (priv->active) {
+		cpsw_intr_disable(priv);
+		cpsw_disable_irq(priv);
+		napi_schedule(&priv->napi);
+		return IRQ_HANDLED;
 	}
-	return IRQ_HANDLED;
+	return IRQ_NONE;
 }
 
 static int cpsw_poll(struct napi_struct *napi, int budget)
@@ -937,6 +943,7 @@  static int cpsw_ndo_open(struct net_device *ndev)
 		cpsw_set_coalesce(ndev, &coal);
 	}
 
+	priv->active = 1;
 	cpdma_ctlr_start(priv->dma);
 	cpsw_intr_enable(priv);
 	napi_enable(&priv->napi);
@@ -980,6 +987,7 @@  static int cpsw_ndo_stop(struct net_device *ndev)
 	pm_runtime_put_sync(&priv->pdev->dev);
 	if (priv->data.dual_emac)
 		priv->slaves[priv->emac_port].open_stat = false;
+	priv->active = 0;
 	return 0;
 }