Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1553907 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] net: ethernet: ti: cpsw: correct common res usage Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> - 2017-01-08 17:50 +0100
    [PATCH 3/4] net: ethernet: ti: cpsw: don't duplicate ndev_running Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> - 2017-01-08 17:50 +0100
      Re: [PATCH 3/4] net: ethernet: ti: cpsw: don't duplicate ndev_running Grygorii Strashko <grygorii.strashko@ti.com> - 2017-01-09 18:30 +0100
        Re: [PATCH 3/4] net: ethernet: ti: cpsw: don't duplicate ndev_running Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> - 2017-01-11 03:00 +0100
          Re: [PATCH 3/4] net: ethernet: ti: cpsw: don't duplicate ndev_running Grygorii Strashko <grygorii.strashko@ti.com> - 2017-01-12 18:40 +0100
    [PATCH 4/4] net: ethernet: ti: cpsw: don't duplicate common res in rx handler Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> - 2017-01-08 17:50 +0100
    Re: [PATCH 0/4] net: ethernet: ti: cpsw: correct common res usage Grygorii Strashko <grygorii.strashko@ti.com> - 2017-01-09 18:20 +0100

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1554513

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web