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

[Amey Narkhede]

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

Thanks,
Amey