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: 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. 

The init code is mostly called from PCI VFIO/UIO device management which
has enabled bus master. If we put the bus master here, people may confuse.
Bind setting the bus master with 'vf->vf_reset' together looks better.

> 
> I'd rather move rte_pci_set_bus_master() (it is a noop if bus master
> is already enabled) in the init path and check for the return code

In fact, not real noop, since the reading PCI ops may fail. ;-)

> there?
> WDYT?
> 
> 
> --
> David Marchand