Re: [dpdk-dev] [PATCH] net/mlx5: fix link status initialization

[Shahaf Shuler <shahafs@mellanox.com>]

Wednesday, April 4, 2018 3:11 PM, Nélio Laranjeiro:
> Subject: Re: [PATCH] net/mlx5: fix link status initialization
> 
> On Wed, Apr 04, 2018 at 09:58:33AM +0000, Shahaf Shuler wrote:
> > Wednesday, April 4, 2018 10:30 AM, Nélio Laranjeiro:
> > > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > >
> > > On Tue, Apr 03, 2018 at 07:48:17AM +0300, Shahaf Shuler wrote:
> > > > Following commit 7ba5320baa32 ("net/mlx5: fix link status
> > > > behavior")
> > > >
> > > > The initial link status is no longer set as part of the port start.
> > > > This may cause application to query the link as down while in fact
> > > > it was already up before the DPDK application start.
> > >
> > > There is something wrong in this explanation, the application should
> > > query the link using this same callback, why the PMD should call it?
> >
> > It is how ethdev is implemented. The application is doing nothing
> > wrong, it queries the link status using rte_eth_link_get_nowait
> >
> > When the application works with LSC interrupts the ethdev layer skips
> > the PMD callback and just update according to the link status exists
> > on device data.
> > It is because it assumes the link status on the device data is the
> > correct one since any link change is processed by the application.
> >
> > The issue is with the initial state of the link. If the link is
> > already up when the PMD starts there will be no callback for the
> > application.
> >
> > I think this logic is OK, and it is also a good practice to initialize
> > the link status to the actual state of the link as part of the port
> > probing.
> 
> The commit log should be re-worded to include this explanation.

Will add.

> 
> > > > Fixes: 7ba5320baa32 ("net/mlx5: fix link status behavior")
> > > > Cc: nelio.laranjeiro@6wind.com
> > > >
> > > > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > > > Acked-by: Yongseok Koh <yskoh@mellanox.com>
> > > > ---
> > > >  drivers/net/mlx5/mlx5.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> > > > index
> > > > 7d58d66bb9..f954ea2862 100644
> > > > --- a/drivers/net/mlx5/mlx5.c
> > > > +++ b/drivers/net/mlx5/mlx5.c
> > > > @@ -961,6 +961,7 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> > > __rte_unused,
> > > >  		DRV_LOG(DEBUG, "port %u forcing Ethernet interface up",
> > > >  			eth_dev->data->port_id);
> > > >  		mlx5_set_link_up(eth_dev);
> > > > +		mlx5_link_update(eth_dev, 1);
> > > >  		/* Store device configuration on private structure. */
> > > >  		priv->config = config;
> > > >  		continue;
> > > > --
> > > > 2.12.0
> 
> According to your analysis, this is only necessary when the LCS is configured
> in the device.  Why not adding this call to
> mlx5_dev_interrupt_handler_install() which is responsible to install the LCS
> callback.

I think it is good practice whether or not LSC is set. 
The link status should be initialized to the correct value after the probe.

> 
> Another point, the wait to complete flag is useless, if the link is up, the status
> and speed will be accurate, if not, it will receive an LSC event later.

Agree.

So how about keeping the code on the current place, just removing the wait_to_complete? 

> 
> Regards,
> 
> --
> Nélio Laranjeiro
> 6WIND