On Wed, May 5, 2021 at 4:56 AM Wang, Haiyue <haiyue.w...@intel.com> wrote: > > > -----Original Message----- > > From: David Marchand <david.march...@redhat.com> > > Sent: Tuesday, May 4, 2021 19:32 > > To: Wang, Haiyue <haiyue.w...@intel.com> > > Cc: dev <dev@dpdk.org>; Zhang, Qi Z <qi.z.zh...@intel.com>; Wang, Liang-min > > <liang-min.w...@intel.com>; > > Wu, Jingjing <jingjing...@intel.com>; Xing, Beilei <beilei.x...@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.w...@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.w...@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. 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? -- David Marchand
