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

["Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>]

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.

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

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.

Regards,

-- 
Nélio Laranjeiro
6WIND