Re: [PATCH v7 1/8] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

[Bjorn Helgaas]

[+to Alex]

On Thu, Jun 24, 2021 at 08:58:09PM +0530, Amey Narkhede wrote:
> On 21/06/24 07:23AM, Bjorn Helgaas wrote:
> > On Tue, Jun 08, 2021 at 11:18:50AM +0530, Amey Narkhede wrote:
> > > Currently there is separate function pcie_has_flr() to probe if pcie flr is
> > > supported by the device which does not match the calling convention
> > > followed by reset methods which use second function argument to decide
> > > whether to probe or not.  Add new function pcie_reset_flr() that follows
> > > the calling convention of reset methods.
> >
> > > +/**
> > > + * pcie_reset_flr - initiate a PCIe function level reset
> > > + * @dev: device to reset
> > > + * @probe: If set, only check if the device can be reset this way.
> > > + *
> > > + * Initiate a function level reset on @dev.
> > > + */
> > > +int pcie_reset_flr(struct pci_dev *dev, int probe)
> > > +{
> > > +	u32 cap;
> > > +
> > > +	if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET)
> > > +		return -ENOTTY;
> > > +
> > > +	pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap);
> > > +	if (!(cap & PCI_EXP_DEVCAP_FLR))
> > > +		return -ENOTTY;
> > > +
> > > +	if (probe)
> > > +		return 0;
> > > +
> > > +	return pcie_flr(dev);
> > > +}
> >
> > Tangent: I've been told before, but I can't remember why we need the
> > "probe" interface.  Since we're looking at this area again, can we add
> > a comment to clarify this?
> >
> > Every time I read this, I wonder why we can't just get rid of the
> > probe and attempt a reset.  If it fails because it's not supported, we
> > could just try the next one in the list.
> 
> Part of the reason is to have same calling convention as other reset
> methods and other reason is devices that run in VMs where various
> capabilities can be hidden or have quirks for avoiding known troublesome
> combination of device features as Alex explained here
> https://lore.kernel.org/linux-pci/20210624151242.ybew2z5rseuusj7v@archlinux/T/#mb67c09a2ce08ce4787652e4c0e7b9e5adf1df57a
> 
> On the side note as you suggested earlier to cache flr capability
> earlier the PCI_EXP_DEVCAP reading code won't be there in next version
> so its just trivial check(dev->has_flr).

Sorry, I didn't make my question clear.  I'm not asking why we're
adding a "probe" argument to pcie_reset_flr() to make it consistent
with pci_af_flr(), pci_pm_reset(), pci_parent_bus_reset(), etc.  I
like making the interfaces consistent.

What I'm asking here is why the "probe" argument exists for *any* of
these interfaces and why pci_probe_reset_function() exists.

This is really more a question for Alex since it's a historical
question, not anything directly related to your series.  I'm not
proposing *removing* the "probe" argument; I know it exists for a
reason because I've asked about it before.  But I forgot the answer,
which makes me think a hint in the code would be useful.

Bjorn