[PATCH 0/4] net: ethernet: ti: cpsw: correct common res usage

[PATCH 0/4] net: ethernet: ti: cpsw: correct common res usage

[Ivan Khoronzhuk]

This series is intended to remove unneeded redundancies connected with
common resource usage function.

Based on net-next/master
Tested on am572x idk

Ivan Khoronzhuk (4):
  net: ethernet: ti: cpsw: remove dual check from common res usage
    function
  net: ethernet: ti: cpsw: don't disable interrupts in ndo_open
  net: ethernet: ti: cpsw: don't duplicate ndev_running
  net: ethernet: ti: cpsw: don't duplicate common res in rx handler

 drivers/net/ethernet/ti/cpsw.c | 57 ++++++++++++++----------------------------
 1 file changed, 19 insertions(+), 38 deletions(-)

-- 
2.7.4

[PATCH 1/4] net: ethernet: ti: cpsw: remove dual check from common res usage function

[Ivan Khoronzhuk]

Common res usage is possible only in case an interface is
running. In case of not dual emac here can be only one interface,
so while ndo_open and switch mode, only one interface can be opened,
thus if open is called no any interface is running ... and no common
res are used. So remove check on dual emac, it will simplify
code/understanding and will match the name it's called.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 drivers/net/ethernet/ti/cpsw.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index f339268..d261024 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1240,9 +1240,6 @@ static int cpsw_common_res_usage_state(struct cpsw_common *cpsw)
 	u32 i;
 	u32 usage_count = 0;
 
-	if (!cpsw->data.dual_emac)
-		return 0;
-
 	for (i = 0; i < cpsw->data.slaves; i++)
 		if (cpsw->slaves[i].open_stat)
 			usage_count++;
-- 
2.7.4

[PATCH 2/4] net: ethernet: ti: cpsw: don't disable interrupts in ndo_open

[Ivan Khoronzhuk]

If any interface is running the interrupts are disabled anyway.
It make sense to disable interrupts if any of interfaces is running,
but in this place, obviously, it didn't have any effect. So, no need
in redundant check and interrupt disable.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 drivers/net/ethernet/ti/cpsw.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index d261024..40d7fc9 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1480,8 +1480,6 @@ static int cpsw_ndo_open(struct net_device *ndev)
 		return ret;
 	}
 
-	if (!cpsw_common_res_usage_state(cpsw))
-		cpsw_intr_disable(cpsw);
 	netif_carrier_off(ndev);
 
 	/* Notify the stack of the actual queue counts. */
-- 
2.7.4

[PATCH 3/4] net: ethernet: ti: cpsw: don't duplicate ndev_running

[Ivan Khoronzhuk]

No need to create additional vars to identify if interface is running.
So simplify code by removing redundant var and checking usage counter
instead.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 drivers/net/ethernet/ti/cpsw.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 40d7fc9..daae87f 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -357,7 +357,6 @@ struct cpsw_slave {
 	struct phy_device		*phy;
 	struct net_device		*ndev;
 	u32				port_vlan;
-	u32				open_stat;
 };
 
 static inline u32 slave_read(struct cpsw_slave *slave, u32 offset)
@@ -1241,7 +1240,7 @@ static int cpsw_common_res_usage_state(struct cpsw_common *cpsw)
 	u32 usage_count = 0;
 
 	for (i = 0; i < cpsw->data.slaves; i++)
-		if (cpsw->slaves[i].open_stat)
+		if (netif_running(cpsw->slaves[i].ndev))
 			usage_count++;
 
 	return usage_count;
@@ -1502,7 +1501,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
 		 CPSW_RTL_VERSION(reg));
 
 	/* initialize host and slave ports */
-	if (!cpsw_common_res_usage_state(cpsw))
+	if (cpsw_common_res_usage_state(cpsw) < 2)
 		cpsw_init_host_port(priv);
 	for_each_slave(priv, cpsw_slave_open, priv);
 
@@ -1513,7 +1512,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
 		cpsw_ale_add_vlan(cpsw->ale, cpsw->data.default_vlan,
 				  ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0);
 
-	if (!cpsw_common_res_usage_state(cpsw)) {
+	if (cpsw_common_res_usage_state(cpsw) < 2) {
 		/* disable priority elevation */
 		__raw_writel(0, &cpsw->regs->ptype);
 
@@ -1556,9 +1555,6 @@ static int cpsw_ndo_open(struct net_device *ndev)
 	cpdma_ctlr_start(cpsw->dma);
 	cpsw_intr_enable(cpsw);
 
-	if (cpsw->data.dual_emac)
-		cpsw->slaves[priv->emac_port].open_stat = true;
-
 	return 0;
 
 err_cleanup:
@@ -1578,7 +1574,7 @@ static int cpsw_ndo_stop(struct net_device *ndev)
 	netif_tx_stop_all_queues(priv->ndev);
 	netif_carrier_off(priv->ndev);
 
-	if (cpsw_common_res_usage_state(cpsw) <= 1) {
+	if (!cpsw_common_res_usage_state(cpsw)) {
 		napi_disable(&cpsw->napi_rx);
 		napi_disable(&cpsw->napi_tx);
 		cpts_unregister(cpsw->cpts);
@@ -1592,8 +1588,6 @@ static int cpsw_ndo_stop(struct net_device *ndev)
 		cpsw_split_res(ndev);
 
 	pm_runtime_put_sync(cpsw->dev);
-	if (cpsw->data.dual_emac)
-		cpsw->slaves[priv->emac_port].open_stat = false;
 	return 0;
 }
 
-- 
2.7.4

[PATCH 4/4] net: ethernet: ti: cpsw: don't duplicate common res in rx handler

[Ivan Khoronzhuk]

No need to duplicate the same function in rx handler to get info
if any interface is running.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 drivers/net/ethernet/ti/cpsw.c | 40 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index daae87f..458298d 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -671,6 +671,18 @@ static void cpsw_intr_disable(struct cpsw_common *cpsw)
 	return;
 }
 
+static int cpsw_common_res_usage_state(struct cpsw_common *cpsw)
+{
+	u32 i;
+	u32 usage_count = 0;
+
+	for (i = 0; i < cpsw->data.slaves; i++)
+		if (netif_running(cpsw->slaves[i].ndev))
+			usage_count++;
+
+	return usage_count;
+}
+
 static void cpsw_tx_handler(void *token, int len, int status)
 {
 	struct netdev_queue	*txq;
@@ -703,18 +715,10 @@ static void cpsw_rx_handler(void *token, int len, int status)
 	cpsw_dual_emac_src_port_detect(cpsw, status, ndev, skb);
 
 	if (unlikely(status < 0) || unlikely(!netif_running(ndev))) {
-		bool ndev_status = false;
-		struct cpsw_slave *slave = cpsw->slaves;
-		int n;
-
-		if (cpsw->data.dual_emac) {
-			/* In dual emac mode check for all interfaces */
-			for (n = cpsw->data.slaves; n; n--, slave++)
-				if (netif_running(slave->ndev))
-					ndev_status = true;
-		}
-
-		if (ndev_status && (status >= 0)) {
+		/* In dual emac mode check for all interfaces */
+		if (cpsw->data.dual_emac &&
+		    cpsw_common_res_usage_state(cpsw) &&
+		    (status >= 0)) {
 			/* The packet received is for the interface which
 			 * is already down and the other interface is up
 			 * and running, instead of freeing which results
@@ -1234,18 +1238,6 @@ static void cpsw_get_ethtool_stats(struct net_device *ndev,
 	}
 }
 
-static int cpsw_common_res_usage_state(struct cpsw_common *cpsw)
-{
-	u32 i;
-	u32 usage_count = 0;
-
-	for (i = 0; i < cpsw->data.slaves; i++)
-		if (netif_running(cpsw->slaves[i].ndev))
-			usage_count++;
-
-	return usage_count;
-}
-
 static inline int cpsw_tx_packet_submit(struct cpsw_priv *priv,
 					struct sk_buff *skb,
 					struct cpdma_chan *txch)
-- 
2.7.4

Re: [PATCH 0/4] net: ethernet: ti: cpsw: correct common res usage

[Grygorii Strashko]

Hi Ivan,

On 01/08/2017 10:40 AM, Ivan Khoronzhuk wrote:
> This series is intended to remove unneeded redundancies connected with
> common resource usage function.
>
> Based on net-next/master
> Tested on am572x idk
>
> Ivan Khoronzhuk (4):
>   net: ethernet: ti: cpsw: remove dual check from common res usage
>     function
>   net: ethernet: ti: cpsw: don't disable interrupts in ndo_open
>   net: ethernet: ti: cpsw: don't duplicate ndev_running
>   net: ethernet: ti: cpsw: don't duplicate common res in rx handler
>

thanks for the patches - I'll need some time to review them.

>  drivers/net/ethernet/ti/cpsw.c | 57 ++++++++++++++----------------------------
>  1 file changed, 19 insertions(+), 38 deletions(-)
>

-- 
regards,
-grygorii

Re: [PATCH 3/4] net: ethernet: ti: cpsw: don't duplicate ndev_running

[Grygorii Strashko]

On 01/08/2017 10:41 AM, Ivan Khoronzhuk wrote:
> No need to create additional vars to identify if interface is running.
> So simplify code by removing redundant var and checking usage counter
> instead.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>  drivers/net/ethernet/ti/cpsw.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 40d7fc9..daae87f 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -357,7 +357,6 @@ struct cpsw_slave {
>  	struct phy_device		*phy;
>  	struct net_device		*ndev;
>  	u32				port_vlan;
> -	u32				open_stat;
>  };
>  
>  static inline u32 slave_read(struct cpsw_slave *slave, u32 offset)
> @@ -1241,7 +1240,7 @@ static int cpsw_common_res_usage_state(struct cpsw_common *cpsw)
>  	u32 usage_count = 0;
>  
>  	for (i = 0; i < cpsw->data.slaves; i++)
> -		if (cpsw->slaves[i].open_stat)
> +		if (netif_running(cpsw->slaves[i].ndev))
>  			usage_count++;

Not sure this will work as you expected, but may be I've missed smth :(

code in static int __dev_open(struct net_device *dev)
..
	set_bit(__LINK_STATE_START, &dev->state);

	if (ops->ndo_validate_addr)
		ret = ops->ndo_validate_addr(dev);

	if (!ret && ops->ndo_open)
		ret = ops->ndo_open(dev);

	netpoll_poll_enable(dev);

	if (ret)
		clear_bit(__LINK_STATE_START, &dev->state);
..

so, netif_running(ndev) will start returning true before calling ops->ndo_open(dev);

>  
>  	return usage_count;
> @@ -1502,7 +1501,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
>  		 CPSW_RTL_VERSION(reg));
>  
>  	/* initialize host and slave ports */
> -	if (!cpsw_common_res_usage_state(cpsw))
> +	if (cpsw_common_res_usage_state(cpsw) < 2)

Ah. You've changed the condition here.

I think it might be reasonable to hide this inside cpsw_common_res_usage_state()
and seems it can be renamed to smth like cpsw_is_running().


>  		cpsw_init_host_port(priv);
>  	for_each_slave(priv, cpsw_slave_open, priv);
>  
> @@ -1513,7 +1512,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
>  		cpsw_ale_add_vlan(cpsw->ale, cpsw->data.default_vlan,
>  				  ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0);
>  
> -	if (!cpsw_common_res_usage_state(cpsw)) {
> +	if (cpsw_common_res_usage_state(cpsw) < 2) {
>  		/* disable priority elevation */
>  		__raw_writel(0, &cpsw->regs->ptype);
>  
> @@ -1556,9 +1555,6 @@ static int cpsw_ndo_open(struct net_device *ndev)
>  	cpdma_ctlr_start(cpsw->dma);
>  	cpsw_intr_enable(cpsw);
>  
> -	if (cpsw->data.dual_emac)
> -		cpsw->slaves[priv->emac_port].open_stat = true;
> -
>  	return 0;
>  
>  err_cleanup:
> @@ -1578,7 +1574,7 @@ static int cpsw_ndo_stop(struct net_device *ndev)
>  	netif_tx_stop_all_queues(priv->ndev);
>  	netif_carrier_off(priv->ndev);
>  
> -	if (cpsw_common_res_usage_state(cpsw) <= 1) {
> +	if (!cpsw_common_res_usage_state(cpsw)) {

and here __LINK_STATE_START will be cleared before calling ops->ndo_stop(dev);
So, from one side netif_running(ndev) usage will simplify cpsw_common_res_usage_state() internals,
but from another side - it will make places where it's used even more entangled :( as for me,
because when cpsw_common_res_usage_state() will return 1 in cpsw_ndo_open() it will mean
"no interfaces is really running yet", but the same value 1 in cpsw_ndo_stop()
will mean "there are still one is running".

>  		napi_disable(&cpsw->napi_rx);
>  		napi_disable(&cpsw->napi_tx);
>  		cpts_unregister(cpsw->cpts);
> @@ -1592,8 +1588,6 @@ static int cpsw_ndo_stop(struct net_device *ndev)
>  		cpsw_split_res(ndev);
>  
>  	pm_runtime_put_sync(cpsw->dev);
> -	if (cpsw->data.dual_emac)
> -		cpsw->slaves[priv->emac_port].open_stat = false;
>  	return 0;
>  }
>  
> 

-- 
regards,
-grygorii

Re: [PATCH 3/4] net: ethernet: ti: cpsw: don't duplicate ndev_running

[Ivan Khoronzhuk]

On Mon, Jan 09, 2017 at 11:25:38AM -0600, Grygorii Strashko wrote:
> 
> 
> On 01/08/2017 10:41 AM, Ivan Khoronzhuk wrote:
> > No need to create additional vars to identify if interface is running.
> > So simplify code by removing redundant var and checking usage counter
> > instead.
> > 
> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> > ---
> >  drivers/net/ethernet/ti/cpsw.c | 14 ++++----------
> >  1 file changed, 4 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> > index 40d7fc9..daae87f 100644
> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -357,7 +357,6 @@ struct cpsw_slave {
> >  	struct phy_device		*phy;
> >  	struct net_device		*ndev;
> >  	u32				port_vlan;
> > -	u32				open_stat;
> >  };
> >  
> >  static inline u32 slave_read(struct cpsw_slave *slave, u32 offset)
> > @@ -1241,7 +1240,7 @@ static int cpsw_common_res_usage_state(struct cpsw_common *cpsw)
> >  	u32 usage_count = 0;
> >  
> >  	for (i = 0; i < cpsw->data.slaves; i++)
> > -		if (cpsw->slaves[i].open_stat)
> > +		if (netif_running(cpsw->slaves[i].ndev))
> >  			usage_count++;
> 
> Not sure this will work as you expected, but may be I've missed smth :(
I've changed conditions, will work.

> 
> code in static int __dev_open(struct net_device *dev)
> ..
> 	set_bit(__LINK_STATE_START, &dev->state);
> 
> 	if (ops->ndo_validate_addr)
> 		ret = ops->ndo_validate_addr(dev);
> 
> 	if (!ret && ops->ndo_open)
> 		ret = ops->ndo_open(dev);
> 
> 	netpoll_poll_enable(dev);
> 
> 	if (ret)
> 		clear_bit(__LINK_STATE_START, &dev->state);
> ..
> 
> so, netif_running(ndev) will start returning true before calling ops->ndo_open(dev);
Yes, It's done bearing it in mind of course.

> 
> >  
> >  	return usage_count;
> > @@ -1502,7 +1501,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
> >  		 CPSW_RTL_VERSION(reg));
> >  
> >  	/* initialize host and slave ports */
> > -	if (!cpsw_common_res_usage_state(cpsw))
> > +	if (cpsw_common_res_usage_state(cpsw) < 2)
> 
> Ah. You've changed the condition here.
> 
> I think it might be reasonable to hide this inside cpsw_common_res_usage_state()
> and seems it can be renamed to smth like cpsw_is_running().
It probably needs to be renamed to smth a little different,
like cpsw_get_usage_count ...or cpsw_get_open_ndev_count

> 
> 
> >  		cpsw_init_host_port(priv);
> >  	for_each_slave(priv, cpsw_slave_open, priv);
> >  
> > @@ -1513,7 +1512,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
> >  		cpsw_ale_add_vlan(cpsw->ale, cpsw->data.default_vlan,
> >  				  ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0);
> >  
> > -	if (!cpsw_common_res_usage_state(cpsw)) {
> > +	if (cpsw_common_res_usage_state(cpsw) < 2) {
> >  		/* disable priority elevation */
> >  		__raw_writel(0, &cpsw->regs->ptype);
> >  
> > @@ -1556,9 +1555,6 @@ static int cpsw_ndo_open(struct net_device *ndev)
> >  	cpdma_ctlr_start(cpsw->dma);
> >  	cpsw_intr_enable(cpsw);
> >  
> > -	if (cpsw->data.dual_emac)
> > -		cpsw->slaves[priv->emac_port].open_stat = true;
> > -
> >  	return 0;
> >  
> >  err_cleanup:
> > @@ -1578,7 +1574,7 @@ static int cpsw_ndo_stop(struct net_device *ndev)
> >  	netif_tx_stop_all_queues(priv->ndev);
> >  	netif_carrier_off(priv->ndev);
> >  
> > -	if (cpsw_common_res_usage_state(cpsw) <= 1) {
> > +	if (!cpsw_common_res_usage_state(cpsw)) {
> 
> and here __LINK_STATE_START will be cleared before calling ops->ndo_stop(dev);
Actually it's changed because of it.

> So, from one side netif_running(ndev) usage will simplify cpsw_common_res_usage_state() internals,
> but from another side - it will make places where it's used even more entangled :( as for me,
> because when cpsw_common_res_usage_state() will return 1 in cpsw_ndo_open() it will mean
> "no interfaces is really running yet", but the same value 1 in cpsw_ndo_stop()
why not? no interfaces running, except the one excuting ndo_open now.
It's more clear then duplicating it and using two different ways in
different places for identifing running devices. Current way more
close to some testing code, not final version. Just to be consistent
better to change it.

Yes, it returns different results when it's called from ndo_close and
ndo_open. Maybe name for the function is not very close to an action
it's doing, it declares more intention, and even not for every case.
What about to rename it to some cpsw_get_open_ndev_count and add
comments in several places explaining what it actually do.

> will mean "there are still one is running".
> 
> >  		napi_disable(&cpsw->napi_rx);
> >  		napi_disable(&cpsw->napi_tx);
> >  		cpts_unregister(cpsw->cpts);
> > @@ -1592,8 +1588,6 @@ static int cpsw_ndo_stop(struct net_device *ndev)
> >  		cpsw_split_res(ndev);
> >  
> >  	pm_runtime_put_sync(cpsw->dev);
> > -	if (cpsw->data.dual_emac)
> > -		cpsw->slaves[priv->emac_port].open_stat = false;
> >  	return 0;
> >  }
> >  
> > 
> 
> -- 
> regards,
> -grygorii

Re: [PATCH 3/4] net: ethernet: ti: cpsw: don't duplicate ndev_running

[Grygorii Strashko]

On 01/10/2017 07:56 PM, Ivan Khoronzhuk wrote:
> On Mon, Jan 09, 2017 at 11:25:38AM -0600, Grygorii Strashko wrote:
>>
>>
>> On 01/08/2017 10:41 AM, Ivan Khoronzhuk wrote:
>>> No need to create additional vars to identify if interface is running.
>>> So simplify code by removing redundant var and checking usage counter
>>> instead.
>>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>>> ---
>>>  drivers/net/ethernet/ti/cpsw.c | 14 ++++----------
>>>  1 file changed, 4 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>>> index 40d7fc9..daae87f 100644
>>> --- a/drivers/net/ethernet/ti/cpsw.c
>>> +++ b/drivers/net/ethernet/ti/cpsw.c
>>> @@ -357,7 +357,6 @@ struct cpsw_slave {
>>>  	struct phy_device		*phy;
>>>  	struct net_device		*ndev;
>>>  	u32				port_vlan;
>>> -	u32				open_stat;
>>>  };
>>>  
>>>  static inline u32 slave_read(struct cpsw_slave *slave, u32 offset)
>>> @@ -1241,7 +1240,7 @@ static int cpsw_common_res_usage_state(struct cpsw_common *cpsw)
>>>  	u32 usage_count = 0;
>>>  
>>>  	for (i = 0; i < cpsw->data.slaves; i++)
>>> -		if (cpsw->slaves[i].open_stat)
>>> +		if (netif_running(cpsw->slaves[i].ndev))
>>>  			usage_count++;
>>
>> Not sure this will work as you expected, but may be I've missed smth :(
> I've changed conditions, will work.
> 
>>
>> code in static int __dev_open(struct net_device *dev)
>> ..
>> 	set_bit(__LINK_STATE_START, &dev->state);
>>
>> 	if (ops->ndo_validate_addr)
>> 		ret = ops->ndo_validate_addr(dev);
>>
>> 	if (!ret && ops->ndo_open)
>> 		ret = ops->ndo_open(dev);
>>
>> 	netpoll_poll_enable(dev);
>>
>> 	if (ret)
>> 		clear_bit(__LINK_STATE_START, &dev->state);
>> ..
>>
>> so, netif_running(ndev) will start returning true before calling ops->ndo_open(dev);
> Yes, It's done bearing it in mind of course.
> 
>>
>>>  
>>>  	return usage_count;
>>> @@ -1502,7 +1501,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
>>>  		 CPSW_RTL_VERSION(reg));
>>>  
>>>  	/* initialize host and slave ports */
>>> -	if (!cpsw_common_res_usage_state(cpsw))
>>> +	if (cpsw_common_res_usage_state(cpsw) < 2)
>>
>> Ah. You've changed the condition here.
>>
>> I think it might be reasonable to hide this inside cpsw_common_res_usage_state()
>> and seems it can be renamed to smth like cpsw_is_running().
> It probably needs to be renamed to smth a little different,
> like cpsw_get_usage_count ...or cpsw_get_open_ndev_count

cpsw_get_usage_count () sounds good

> 
>>
>>
>>>  		cpsw_init_host_port(priv);
>>>  	for_each_slave(priv, cpsw_slave_open, priv);
>>>  
>>> @@ -1513,7 +1512,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
>>>  		cpsw_ale_add_vlan(cpsw->ale, cpsw->data.default_vlan,
>>>  				  ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0);
>>>  
>>> -	if (!cpsw_common_res_usage_state(cpsw)) {
>>> +	if (cpsw_common_res_usage_state(cpsw) < 2) {
>>>  		/* disable priority elevation */
>>>  		__raw_writel(0, &cpsw->regs->ptype);
>>>  
>>> @@ -1556,9 +1555,6 @@ static int cpsw_ndo_open(struct net_device *ndev)
>>>  	cpdma_ctlr_start(cpsw->dma);
>>>  	cpsw_intr_enable(cpsw);
>>>  
>>> -	if (cpsw->data.dual_emac)
>>> -		cpsw->slaves[priv->emac_port].open_stat = true;
>>> -
>>>  	return 0;
>>>  
>>>  err_cleanup:
>>> @@ -1578,7 +1574,7 @@ static int cpsw_ndo_stop(struct net_device *ndev)
>>>  	netif_tx_stop_all_queues(priv->ndev);
>>>  	netif_carrier_off(priv->ndev);
>>>  
>>> -	if (cpsw_common_res_usage_state(cpsw) <= 1) {
>>> +	if (!cpsw_common_res_usage_state(cpsw)) {
>>
>> and here __LINK_STATE_START will be cleared before calling ops->ndo_stop(dev);
> Actually it's changed because of it.
> 
>> So, from one side netif_running(ndev) usage will simplify cpsw_common_res_usage_state() internals,
>> but from another side - it will make places where it's used even more entangled :( as for me,
>> because when cpsw_common_res_usage_state() will return 1 in cpsw_ndo_open() it will mean
>> "no interfaces is really running yet", but the same value 1 in cpsw_ndo_stop()
> why not? no interfaces running, except the one excuting ndo_open now.
> It's more clear then duplicating it and using two different ways in
> different places for identifing running devices. Current way more
> close to some testing code, not final version. Just to be consistent
> better to change it.
> 
> Yes, it returns different results when it's called from ndo_close and
> ndo_open. Maybe name for the function is not very close to an action
> it's doing, it declares more intention, and even not for every case.
> What about to rename it to some cpsw_get_open_ndev_count and add
> comments in several places explaining what it actually do.

yes. please. comments are required at least.

its actually a question why __LINK_STATE_START is managed this way in ./net/core/dev.c

__dev_open()
	set_bit(__LINK_STATE_START, &dev->state); <---- before .ndo_open()

	if (!ret && ops->ndo_open)
		ret = ops->ndo_open(dev);

	<---- shouldn't set_bit(__LINK_STATE_START, &dev->state) be after calling .ndo_open() ??

__dev_close_many()

	clear_bit(__LINK_STATE_START, &dev->state); <-stop sequence is differ from open.

	if (ops->ndo_stop)
			ops->ndo_stop(dev);


-- 
regards,
-grygorii

Re: [PATCH 3/4] net: ethernet: ti: cpsw: don't duplicate ndev_running

[Ivan Khoronzhuk]

On Thu, Jan 12, 2017 at 11:34:47AM -0600, Grygorii Strashko wrote:

Hi Grygorii,
Sorry for late reply.

> 
> 
> On 01/10/2017 07:56 PM, Ivan Khoronzhuk wrote:
> > On Mon, Jan 09, 2017 at 11:25:38AM -0600, Grygorii Strashko wrote:
> >>
> >>
> >> On 01/08/2017 10:41 AM, Ivan Khoronzhuk wrote:
> >>> No need to create additional vars to identify if interface is running.
> >>> So simplify code by removing redundant var and checking usage counter
> >>> instead.
> >>>
> >>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >>> ---
> >>>  drivers/net/ethernet/ti/cpsw.c | 14 ++++----------
> >>>  1 file changed, 4 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> >>> index 40d7fc9..daae87f 100644
> >>> --- a/drivers/net/ethernet/ti/cpsw.c
> >>> +++ b/drivers/net/ethernet/ti/cpsw.c
> >>> @@ -357,7 +357,6 @@ struct cpsw_slave {
> >>>  	struct phy_device		*phy;
> >>>  	struct net_device		*ndev;
> >>>  	u32				port_vlan;
> >>> -	u32				open_stat;
> >>>  };
> >>>  
> >>>  static inline u32 slave_read(struct cpsw_slave *slave, u32 offset)
> >>> @@ -1241,7 +1240,7 @@ static int cpsw_common_res_usage_state(struct cpsw_common *cpsw)
> >>>  	u32 usage_count = 0;
> >>>  
> >>>  	for (i = 0; i < cpsw->data.slaves; i++)
> >>> -		if (cpsw->slaves[i].open_stat)
> >>> +		if (netif_running(cpsw->slaves[i].ndev))
> >>>  			usage_count++;
> >>
> >> Not sure this will work as you expected, but may be I've missed smth :(
> > I've changed conditions, will work.
> > 
> >>
> >> code in static int __dev_open(struct net_device *dev)
> >> ..
> >> 	set_bit(__LINK_STATE_START, &dev->state);
> >>
> >> 	if (ops->ndo_validate_addr)
> >> 		ret = ops->ndo_validate_addr(dev);
> >>
> >> 	if (!ret && ops->ndo_open)
> >> 		ret = ops->ndo_open(dev);
> >>
> >> 	netpoll_poll_enable(dev);
> >>
> >> 	if (ret)
> >> 		clear_bit(__LINK_STATE_START, &dev->state);
> >> ..
> >>
> >> so, netif_running(ndev) will start returning true before calling ops->ndo_open(dev);
> > Yes, It's done bearing it in mind of course.
> > 
> >>
> >>>  
> >>>  	return usage_count;
> >>> @@ -1502,7 +1501,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
> >>>  		 CPSW_RTL_VERSION(reg));
> >>>  
> >>>  	/* initialize host and slave ports */
> >>> -	if (!cpsw_common_res_usage_state(cpsw))
> >>> +	if (cpsw_common_res_usage_state(cpsw) < 2)
> >>
> >> Ah. You've changed the condition here.
> >>
> >> I think it might be reasonable to hide this inside cpsw_common_res_usage_state()
> >> and seems it can be renamed to smth like cpsw_is_running().
> > It probably needs to be renamed to smth a little different,
> > like cpsw_get_usage_count ...or cpsw_get_open_ndev_count
> 
> cpsw_get_usage_count () sounds good
Like it more also. Will change it.

> 
> > 
> >>
> >>
> >>>  		cpsw_init_host_port(priv);
> >>>  	for_each_slave(priv, cpsw_slave_open, priv);
> >>>  
> >>> @@ -1513,7 +1512,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
> >>>  		cpsw_ale_add_vlan(cpsw->ale, cpsw->data.default_vlan,
> >>>  				  ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0);
> >>>  
> >>> -	if (!cpsw_common_res_usage_state(cpsw)) {
> >>> +	if (cpsw_common_res_usage_state(cpsw) < 2) {
> >>>  		/* disable priority elevation */
> >>>  		__raw_writel(0, &cpsw->regs->ptype);
> >>>  
> >>> @@ -1556,9 +1555,6 @@ static int cpsw_ndo_open(struct net_device *ndev)
> >>>  	cpdma_ctlr_start(cpsw->dma);
> >>>  	cpsw_intr_enable(cpsw);
> >>>  
> >>> -	if (cpsw->data.dual_emac)
> >>> -		cpsw->slaves[priv->emac_port].open_stat = true;
> >>> -
> >>>  	return 0;
> >>>  
> >>>  err_cleanup:
> >>> @@ -1578,7 +1574,7 @@ static int cpsw_ndo_stop(struct net_device *ndev)
> >>>  	netif_tx_stop_all_queues(priv->ndev);
> >>>  	netif_carrier_off(priv->ndev);
> >>>  
> >>> -	if (cpsw_common_res_usage_state(cpsw) <= 1) {
> >>> +	if (!cpsw_common_res_usage_state(cpsw)) {
> >>
> >> and here __LINK_STATE_START will be cleared before calling ops->ndo_stop(dev);
> > Actually it's changed because of it.
> > 
> >> So, from one side netif_running(ndev) usage will simplify cpsw_common_res_usage_state() internals,
> >> but from another side - it will make places where it's used even more entangled :( as for me,
> >> because when cpsw_common_res_usage_state() will return 1 in cpsw_ndo_open() it will mean
> >> "no interfaces is really running yet", but the same value 1 in cpsw_ndo_stop()
> > why not? no interfaces running, except the one excuting ndo_open now.
> > It's more clear then duplicating it and using two different ways in
> > different places for identifing running devices. Current way more
> > close to some testing code, not final version. Just to be consistent
> > better to change it.
> > 
> > Yes, it returns different results when it's called from ndo_close and
> > ndo_open. Maybe name for the function is not very close to an action
> > it's doing, it declares more intention, and even not for every case.
> > What about to rename it to some cpsw_get_open_ndev_count and add
> > comments in several places explaining what it actually do.
> 
> yes. please. comments are required at least.
> 
> its actually a question why __LINK_STATE_START is managed this way in ./net/core/dev.c
> 
> __dev_open()
> 	set_bit(__LINK_STATE_START, &dev->state); <---- before .ndo_open()
> 
> 	if (!ret && ops->ndo_open)
> 		ret = ops->ndo_open(dev);
> 
> 	<---- shouldn't set_bit(__LINK_STATE_START, &dev->state) be after calling .ndo_open() ??
By logic yes, but in another way it looks like it 's done intentionally.
Some code can be based on it, some that can be executed while .ndo_open
and after. And it should act the same in both cases. In case of cpsw, at least
cpsw_adjust_link() can be called while .ndo_open and also after, but in both
cases it's supposed that flag is set. In case of cpsw_adjust_link() there is no
way to predict when it will be called, so here only one way - set
__LINK_STATE_START before .ndo_open(), result - flag is set in any case already.
Maybe that is one of the reasons of such sequence.
Changing the logic can bring a lot of headache, don't want to touch it here.

> 
> __dev_close_many()
> 
> 	clear_bit(__LINK_STATE_START, &dev->state); <-stop sequence is differ from open.
Yes, and it doens't have postponed tasks like .ndo_open.

> 
> 	if (ops->ndo_stop)
> 			ops->ndo_stop(dev);
> 
> 
> -- 
> regards,
> -grygorii