Re: [PATCHv2 1/2] pci: provide bus reset attribute

[Keith Busch <kbusch@kernel.org>]

On Tue, Nov 12, 2024 at 05:16:23PM -0600, Bjorn Helgaas wrote:
> On Fri, Oct 25, 2024 at 03:27:54PM -0700, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > Resetting a bus from an end device only works if it's the only function
> > on or below that bus.
> 
> Can we clarify this wording?  "Resetting a bus from an end device"?

What I mean is, if you find a device you want to reset in the sysfs
hierarchy, and find its reset method:

  /sys/bus/pci/devices/<D:B:D.f>/reset_method

If you request "bus", it only works if it is the only function on that bus.

I agree my commit message there is a bit unclear, though. I was a bit to
deep in that code when I wrote it.
 
> I guess this has something to do with pci_parent_bus_reset(dev), which
> declines to call pci_bridge_secondary_bus_reset() if there are any
> other devices on the same bus as "dev"? 

Yep, exactly.

> pci_parent_bus_reset() is only called from pci_reset_bus_function(),
> which is used by the "bus" and "cxl_bus" reset methods.
> 
> So I guess the point is something like:
> 
>   The "bus" and "cxl_bus" reset methods reset a device by asserting
>   Secondary Bus Reset on the bridge leading to the device.
>   pci_parent_bus_reset() only allows that if the device is the only
>   device below the bridge.
> 
>   Add a sysfs attribute on bridges that can assert Secondary Bus Reset
>   regardless of how many devices are below the bridge.  This makes it
>   possible for users to reset multiple devices in a single command.
> 
> I omitted "safely" because this doesn't do any checking to ensure
> safety, so I don't know in what sense it is safe.

By "safe", what I mean is the device is locked by the kernel, config
space is saved and restored on either side of the reset, and the
attached driver (if any) is notified this action is about to happen and
when it completes so it do whatever quiescent and restoring actions it
needs for a bus reset.

I can't say this is universally "safe" since it's a bit optomistic to
assume everything affected by this action is going to work as the pci
driver expects, but the alternatives (setpci) don't coordinate anything
with the kernel, so it's "safe" relative to that :)

> It seems like this is partly just a convenience, to reset several
> devices at once.
>
> But I think it is *also* a new way to reset devices that we might not
> be able to reset otherwise, e.g., if there's more than one device on
> the bus, and they don't support ACPI, FLR, or PM reset, there
> previously was no reset method that worked at all.  Right?

Exactly! I have multi-function devices in a switch hierarchy where this
unfortunately is really the only way to do it. They don't support
resetting individual functions, so SBR is the only way we have to
reliably reset the device.