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

[Alex Williamson]

On Thu, 24 Jun 2021 11:15:59 -0500
Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:

> [+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.

Heh [1]

That might be what you're recalling, but in that case I was adding
exported symbols that allowed probing bus vs slot reset because the
scope of affected devices is different.  My use case is testing whether
the user owns all the affected devices, so it's really not a
test-by-doing opportunity.

For these single-function scoped resets, as in the reply to [1]
pci_probe_reset_function() isn't exported and the only caller is
internal PCI code to determine whether to create the 'reset' sysfs
attribute.  Sure, as it exists today we could reset the device and test
whether it worked to get that value, that's what vfio-pci does now
before we give the device to the user, but the critical difference is
that in the vfio case we always want to flush any state that might be
leaked to the user and at device init time, doing so only invites
issues.

This series obviously expands the scope of probing, we don't just want
to know that there's at least one method available to us, but precisely
which ones.  It's rather impractical to try to reset a function a half
dozen different ways on boot for the possibility that the admin might
want to manipulate the reset order later.  And oh gosh, if we don't
cache the methods supported and re-test-by-doing when the attribute is
written, let's just not go there.  Thanks,

Alex

[1]https://lore.kernel.org/linux-pci/CAErSpo625CTnxZvy-gmy8VzxT4favF4s=_giU6nGey_N=VwK5A@xxxxxxxxxxxxxx/