Re: [PATCH] pci: provide bus reset attribute

[Alex Williamson <alex.williamson@redhat.com>]

On Fri, 25 Oct 2024 11:57:25 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> [+cc Alex, Amey, Raphael]
> 
> On Fri, Oct 25, 2024 at 07:50:21AM -0700, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > Attempting a bus reset on an end device only works if it's the only
> > function on or below that bus.
> > 
> > Provide an attribute on the pci_bus device that can perform the
> > secondary bus reset. This makes it possible for a user to safely reset
> > multiple devices in a single command using the secondary bus reset
> > method.  
> 
> I confess to being a little ambivalent or even hesitant about
> operations on the pci_bus (as opposed to on a pci_dev), but I can't
> really articulate a great reason, other than the fact that the "bus"
> is kind of abstract and from a hardware perspective, the *devices* are
> the only things we can control.
> 
> I assume this is useful in some scenario.  I guess this is root-only,
> so there's no real concern about whether all the devices are used by
> the same VM or in the same IOMMU group or anything?

I can understand your hesitation, but TBH I've wished for such a thing
in the past.  We can already twiddle the secondary bus reset bit using
setpci, but then we don't restore config space of the subordinate
devices and at best we need to remove and rescan those devices.

As Keith notes in his reply, we can also effectively trigger this same
thing through vfio-pci, so I think we're only making it a little easier
to accomplish through this sysfs attribute.  Yes, bad things can happen
if we were to reset a bus of running devices, but I don't know that
it's any more bad than resetting each of those devices individually.

I would agree that "reset" is not a great name since we're resetting
the subordinate devices and not the bridge device under which this
attribute would appear.  Seems there should also be an update to
Documentation/ABI/testing/sysfs-bus-pci in this patch too.  Thanks,

Alex

 
> > Signed-off-by: Keith Busch <kbusch@kernel.org>
> > ---
> >  drivers/pci/pci-sysfs.c | 23 +++++++++++++++++++++++
> >  drivers/pci/pci.c       |  2 +-
> >  drivers/pci/pci.h       |  1 +
> >  3 files changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 5d0f4db1cab78..616d64f12da4d 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -521,6 +521,28 @@ static ssize_t bus_rescan_store(struct device *dev,
> >  static struct device_attribute dev_attr_bus_rescan = __ATTR(rescan, 0200, NULL,
> >  							    bus_rescan_store);
> >  
> > +static ssize_t bus_reset_store(struct device *dev,
> > +				struct device_attribute *attr,
> > +				const char *buf, size_t count)
> > +{
> > +	struct pci_bus *bus = to_pci_bus(dev);
> > +	unsigned long val;
> > +
> > +	if (kstrtoul(buf, 0, &val) < 0)
> > +		return -EINVAL;
> > +
> > +	if (val) {
> > +		int ret = __pci_reset_bus(bus);
> > +
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return count;
> > +}
> > +static struct device_attribute dev_attr_bus_reset = __ATTR(reset, 0200, NULL,
> > +							   bus_reset_store);
> > +
> >  #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
> >  static ssize_t d3cold_allowed_store(struct device *dev,
> >  				    struct device_attribute *attr,
> > @@ -638,6 +660,7 @@ static struct attribute *pcie_dev_attrs[] = {
> >  
> >  static struct attribute *pcibus_attrs[] = {
> >  	&dev_attr_bus_rescan.attr,
> > +	&dev_attr_bus_reset.attr,
> >  	&dev_attr_cpuaffinity.attr,
> >  	&dev_attr_cpulistaffinity.attr,
> >  	NULL,
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 7d85c04fbba2a..338dfcd983f1e 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -5880,7 +5880,7 @@ EXPORT_SYMBOL_GPL(pci_probe_reset_bus);
> >   *
> >   * Same as above except return -EAGAIN if the bus cannot be locked
> >   */
> > -static int __pci_reset_bus(struct pci_bus *bus)
> > +int __pci_reset_bus(struct pci_bus *bus)
> >  {
> >  	int rc;
> >  
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 14d00ce45bfa9..1cdc2c9547a7e 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -104,6 +104,7 @@ bool pci_reset_supported(struct pci_dev *dev);
> >  void pci_init_reset_methods(struct pci_dev *dev);
> >  int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
> >  int pci_bus_error_reset(struct pci_dev *dev);
> > +int __pci_reset_bus(struct pci_bus *bus);
> >  
> >  struct pci_cap_saved_data {
> >  	u16		cap_nr;
> > -- 
> > 2.43.5
> >   
>