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

[Shahaf Shuler <shahafs@mellanox.com>]

Thursday, April 5, 2018 9:51 AM, Nélio Laranjeiro:
> Subject: Re: [PATCH] net/mlx5: fix link status initialization
> 
> On Thu, Apr 05, 2018 at 05:35:57AM +0000, Shahaf Shuler wrote:
> > 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:

[..]

> > >
> > > 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.
> 
> There is no guarantee the link will be accurate, at probe time the link may be
> up so internal information has a status up with a speed with this patch.
> The application probes a second port, in the mean time the link of the first
> port goes down, the interrupt is still not installed and the internal status
> becomes wrong (still up whereas the port is down).
> 
> Finally at start, the device installs the handler, but the link is still down
> whereas internally it is up, the application will call
> rte_eth_link_get_nowait() which will directly copy the wrong internal status
> to the application.

This is not correct.
Using Verbs, the async_fd on which link status interrupts are reported is created on probe. 
Even if the interrupt handler is not installed, interrupts still trigger on this fd. They will be processed when the interrupt handler will be installed as part of the port start. 
So in fact you have the whole trace on the link status changes waiting to be processed upon port start. 

Please try and see. 

> 
> There is also another situation, when the application stops the port, the
> interrupt is also removed, which means during this time, the internal status
> may be wrong as it won't be updated anymore.
> This comes to the same possible situation as above.

Same comment. 

> 
> > > 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?
> 
> The current place is not fixing the issue as there is still a possibility to have a
> wrong value.
> 
> Regards,
> 
> --
> Nélio Laranjeiro
> 6WIND