> -----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."); >