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@xxxxxxxxxx>
> >>> ---
> >>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
