[dpdk-dev] [PATCH v1] net/i40e: avoid invalid operations after reset

Zhang, Qi Z qi.z.zhang at intel.com
Tue Dec 11 07:58:31 CET 2018



> -----Original Message-----
> From: Yan, Zhirun
> Sent: Tuesday, December 11, 2018 1:32 PM
> To: Zhang, Qi Z <qi.z.zhang at intel.com>; dev at dpdk.org; Wang, Haiyue
> <haiyue.wang at intel.com>
> Subject: RE: [PATCH v1] net/i40e: avoid invalid operations after reset
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Monday, December 10, 2018 9:13 PM
> > To: Yan, Zhirun <zhirun.yan at intel.com>; dev at dpdk.org; Wang, Haiyue
> > <haiyue.wang at intel.com>
> > Subject: RE: [PATCH v1] net/i40e: avoid invalid operations after reset
> >
> >
> >
> > > -----Original Message-----
> > > From: Yan, Zhirun
> > > Sent: Tuesday, December 11, 2018 12:05 AM
> > > To: dev at dpdk.org; Wang, Haiyue <haiyue.wang at intel.com>; Zhang, Qi Z
> > > <qi.z.zhang at intel.com>
> > > Cc: Yan, Zhirun <zhirun.yan at intel.com>
> > > Subject: [PATCH v1] net/i40e: avoid invalid operations after reset
> > >
> > > if reset but not reinit adminq, some operations in
> > > i40evf_dev_close() like
> > > i40evf_dev_promiscuous_disable() and
> > > i40evf_dev_allmulticast_disable() will result in failures.
> > >
> > > Fixes: cae18d2b0fb4 ("net/i40e: add workaround promiscuous disable")
> > >
> > > Signed-off-by: Zhirun Yan <zhirun.yan at intel.com>
> > > Signed-off-by: Haiyue Wang <haiyue.wang at intel.com>
> > > ---
> > >  drivers/net/i40e/i40e_ethdev_vf.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> > > b/drivers/net/i40e/i40e_ethdev_vf.c
> > > index ae55b9b18..a1f4a729f 100644
> > > --- a/drivers/net/i40e/i40e_ethdev_vf.c
> > > +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> > > @@ -2245,6 +2245,7 @@ static void
> > >  i40evf_dev_close(struct rte_eth_dev *dev)  {
> > >  	struct i40e_hw *hw =
> > > I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > > +	struct i40e_vf *vf =
> > > I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > >
> > >  	i40evf_dev_stop(dev);
> > >  	i40e_dev_free_queues(dev);
> > > @@ -2253,10 +2254,12 @@ i40evf_dev_close(struct rte_eth_dev *dev)
> > >  	 * it is a workaround solution when work with kernel driver
> > >  	 * and it is not the normal way
> > >  	 */
> > > -	i40evf_dev_promiscuous_disable(dev);
> > > -	i40evf_dev_allmulticast_disable(dev);
> > > +	if (!vf->vf_reset) {
> >
> > I didn't see when vf_reset will be cleared to false? Is it another bug?
> >
> After reset VF, vf->vf_reset will be cleared to false after calling
> i40evf_request_queues().
> This patch is used to fix request_queues.

If the patch is going to fix another new patch, then it should be part of that :)

But my suggestion is we could keep this patch as independent, 
because strictly even without the request queue patch, the issue is still valid, since there will be other cases that pf will reset VF.

But the problem here is your fix rely on that vf_reset flag should always be updated correctly, but actually it does not, it is not cleared always, that means once vf reset happened, 
in following dev_close, you will always skip those adminq command.

>From my view, there should be a fix to clear the vf_reset and vf->pend_msg, and i40evf_check_vf_reset_done looks like the right place to do this. 
ideally your "request queues" patch should not touch these flag

> 
> >
> > > +		i40evf_dev_promiscuous_disable(dev);
> > > +		i40evf_dev_allmulticast_disable(dev);
> > > +		i40evf_reset_vf(hw);
> > > +	}
> >
> > > -	i40evf_reset_vf(hw);
> > >  	i40e_shutdown_adminq(hw);
> > >  	i40evf_disable_irq0(hw);
> > >  	rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
> > > --
> > > 2.17.1



More information about the dev mailing list