Re: [dpdk-dev] [PATCH v4 2/3] net/iavf: enable PCI bus master after reset

["Wang, Haiyue" <haiyue.wang@intel.com>]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, May 5, 2021 16:39
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev <dev@dpdk.org>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wang, Liang-min <liang-min.wang@intel.com>;
> Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Subject: Re: [PATCH v4 2/3] net/iavf: enable PCI bus master after reset
> 
> On Wed, May 5, 2021 at 4:56 AM Wang, Haiyue <haiyue.wang@intel.com> wrote:
> >
> > > -----Original Message-----
> > > From: David Marchand <david.marchand@redhat.com>
> > > Sent: Tuesday, May 4, 2021 19:32
> > > To: Wang, Haiyue <haiyue.wang@intel.com>
> > > Cc: dev <dev@dpdk.org>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wang, Liang-min <liang-
> min.wang@intel.com>;
> > > Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> > > Subject: Re: [PATCH v4 2/3] net/iavf: enable PCI bus master after reset
> > >
> > > On Tue, Apr 27, 2021 at 4:05 PM Haiyue Wang <haiyue.wang@intel.com> wrote:
> > > >
> > > > The VF reset can be triggerred by the PF reset event, in this case, the
> > > > PCI bus master will be cleared, then the VF is not allowed to issue any
> > > > Memory or I/O Requests.
> > > >
> > > > So after the reset event is detected, always enable the PCI bus master.
> > > >
> > > > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> > > > ---
> > > >  drivers/net/iavf/iavf_ethdev.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
> > > > index d523a0618..9a0a20a29 100644
> > > > --- a/drivers/net/iavf/iavf_ethdev.c
> > > > +++ b/drivers/net/iavf/iavf_ethdev.c
> > > > @@ -2255,6 +2255,9 @@ iavf_dev_close(struct rte_eth_dev *dev)
> > > >         rte_free(vf->aq_resp);
> > > >         vf->aq_resp = NULL;
> > > >
> > > > +       if (vf->vf_reset)
> > > > +               rte_pci_set_bus_master(pci_dev, true);
> > > > +
> > > >         vf->vf_reset = false;
> > >
> > > Not checking for the return code can leave the device in an invalid state.
> > > Then after this, calling the init code will fail.
> >
> > From the upper application's view, if this bus master fix can't recover
> > the device into valid state, then the device hotplug API should be used
> > to make the device fully recover. So I'd prefer to call bus master "try
> > best" to fix. If still have error, the system may be in bad state.
> 
> I find it odd to put something required for (re)initialising in a
> .dev_close ops.
> Maybe the place is more into .dev_reset if you find it confusing in .dev_init.
> But this is your driver, I'll let it to your judgement.
> 

In fact, the .dev_close is close to .dev_reset.

static int
iavf_dev_reset(struct rte_eth_dev *dev)
{
	int ret;

	ret = iavf_dev_uninit(dev); --> iavf_dev_close
	if (ret)
		return ret;

	return iavf_dev_init(dev);
}

> 
> On the other hand, what is the point of not failing early/propagating
> the rte_pci_set_bus_master error?
> The driver can't work without bus master enabled, correct?

From this point of view, correct. ;-)

Now I changed the vf_reset handling as bellow, if bus master failed, the
device will still be in VF reset state, so that other modules will return
immediately (the PMD has considered the VF reset state already).

if (vf->vf_reset &&
    !rte_pci_set_bus_master(pci_dev, true))
	vf->vf_reset = false;

> 
> 
> --
> David Marchand