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.laranje...@6wind.com
> > > >
> > > > Signed-off-by: Shahaf Shuler <shah...@mellanox.com>
> > > > Acked-by: Yongseok Koh <ys...@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
