[dpdk-dev] [PATCH v6 0/4] support reset of VF link

[Lu, Wenzhuo]

> -----Original Message-----
> From: Luca Boccassi [mailto:lboccass at Brocade.com]
> Sent: Monday, July 11, 2016 11:43 PM
> To: Lu, Wenzhuo
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6 0/4] support reset of VF link
> 
> On Mon, 2016-07-11 at 13:02 +0100, Luca Boccassi wrote:
> > On Mon, 2016-07-11 at 01:32 +0000, Lu, Wenzhuo wrote:
> > > >
> > > > Unfortunately I found one issue: if PF is down, and then the VF on
> > > > the guest is down as well (ip link down) and then goes back up
> > > > before the PF, then calling rte_eth_dev_reset will return 0
> > > > (success), even though the PF is still down and it should fail. This is with
> ixgbe. Any idea what could be the problem?
> > > I've found this interesting thing. I believe it’s the HW difference between igb
> and ixgbe. When the link is down, ixgbe VF can be reset successfully but igb VF
> cannot. The expression is the  registers of the ixgbe VF can be accessed when
> the PF link is down but igb VF cannot.
> > > It means, on ixgbe, when PF link is down, we reset the VF link. Then PF link is
> up, we receive the message again and reset the VF link again.
> >
> > What message do you refer to here? I am seeing the RESET callback only
> > when the PF goes down, not when it goes up.
> >
> > At the moment, with ixgbe, this happens:
> >
> > PF down -> reset notification, rte_eth_dev_reset keeps failing -> VF
> > down -> VF up -> rte_eth_dev_reset in a loop/timer succeeds -> PF up
> > -> VF link has no-carrier, and traffic does NOT go through
> >
> > The problem is that there is just no way of being notified that PF is
> > up, and if rte_eth_dev_reset succeeds I have no way of knowing that I
> > need to run it again.
> 
> I was now able to solve this use case, by having the rte_eth_dev_reset
> implementations return -EAGAIN if the dev is not up. This way I know, in the
> application, that I have to try again. What do you think?
> 
> IMHO it makes sense, as the reset does not actually succeeds, and the caller
> should try again. The diff is very trivial, and attached for reference.
Yes, I think the change is reasonable. Sorry, I didn’t realize you're talking about the code you have changed. Maybe we're not on the same page when discussing before :)

> 
> --
> Kind regards,
> Luca Boccassi
> 
> 
> Make rte_eth_dev_reset return EAGAIN if VF down
> 
> If VF is down the reset will not happen, so the driver should return
> EAGAIN to signal the application that it needs to call again
> rte_eth_dev_reset.
> 
> Signed-off-by: Luca Boccassi <lboccass at brocade.com
> ---
>  drivers/net/e1000/igb_ethdev.c    |    2 +-
>  drivers/net/i40e/i40e_ethdev_vf.c |    2 +-
>  drivers/net/ixgbe/ixgbe_ethdev.c  |    2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -6235,7 +6235,7 @@ ixgbevf_dev_reset(struct rte_eth_dev *de
> 
>  	/* Nothing needs to be done if the device is not started. */
>  	if (!dev->data->dev_started)
> -		 return 0;
> +		 return -EAGAIN;
> 
>  	PMD_DRV_LOG(DEBUG, "Link up/down event detected.");
> 
> --- a/drivers/net/i40e/i40e_ethdev_vf.c
> +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> @@ -1504,7 +1504,7 @@ i40evf_handle_vf_reset(struct rte_eth_de
>  		 I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> 
>  	if (!dev->data->dev_started)
> -		 return 0;
> +		 return -EAGAIN;
> 
>  	adapter->reset_number = 1;
>  	i40e_vf_reset_dev(dev);
> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -2609,7 +2609,7 @@ igbvf_dev_reset(struct rte_eth_dev *dev,
> 
>  	/* Nothing needs to be done if the device is not started. */
>  	if (!dev->data->dev_started)
> -		 return 0;
> +		 return -EAGAIN;
> 
>  	PMD_DRV_LOG(DEBUG, "Link up/down event detected.");
>