Message-ID: <20170118013023.GA6247@khorivan>
Date:   Wed, 18 Jan 2017 03:30:24 +0200
From:   Ivan Khoronzhuk <ivan.khoronzhuk@...aro.org>
To:     Grygorii Strashko <grygorii.strashko@...com>
Cc:     netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>, mugunthanvnm@...com,
        linux-omap@...r.kernel.org, linux-kernel@...r.kernel.org,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH 3/4] net: ethernet: ti: cpsw: don't duplicate ndev_running

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@...aro.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

Powered by blists - more mailing lists