On Mon, 1 Mar 2021 14:28:17 -0600 Bjorn Helgaas <helg...@kernel.org> wrote:
> [+cc Alex, reset expert] > > On Mon, Mar 01, 2021 at 06:12:21PM +0100, Pali Rohár wrote: > > Hello! > > > > PCIe card can be reset via in-band Hot Reset signal which can be > > triggered by PCIe bridge via Secondary Bus Reset bit in PCI config > > space. > > > > Kernel already exports sysfs node "reset" for triggering Functional > > Reset of particular function of PCI device. But in some cases Functional > > Reset is not enough and Hot Reset is required. > > > > Following RFC patch exports sysfs node "reset_bus" for PCI bridges which > > triggers Secondary Bus Reset and therefore for PCIe bridges it resets > > connected PCIe card. > > > > What do you think about it? > > > > Currently there is userspace script which can trigger PCIe Hot Reset by > > modifying PCI config space from userspace: > > > > https://alexforencich.com/wiki/en/pcie/hot-reset-linux > > > > But because kernel already provides way how to trigger Functional Reset > > it could provide also way how to trigger PCIe Hot Reset. What that script does and what this does, or what the existing reset attribute does, are very different. The script finds the upstream bridge for a given device, removes the device (ignoring that more than one device might be affected by the bus reset), uses setpci to trigger a secondary bus reset, then rescans devices. The below only triggers the secondary bus reset, neither saving and restoring affected device state like the existing function level reset attribute, nor removing and rescanning as the script does. It simply leaves an entire hierarchy of PCI devices entirely un-programmed yet still has struct pci_devs attached to them for untold future misery. In fact, for the case of a single device affected by the bus reset, as intended by the script, the existing reset attribute will already do that if the device supports no other reset mechanism. There's actually a running LFX mentorship project that aims to allow the user to control the type of reset performed by the existing reset attribute such that a user could force the bus reset behavior over other reset methods. There might be some justification for an attribute that actually implements the referenced script correctly, perhaps in kernel we could avoid races with bus rescans, but simply triggering an SBR to quietly de-program all downstream devices with no state restore or device rescan is not it. Any affected device would be unusable. Was this tested? Thanks, Alex > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > > index 50fcb62d59b5..f5e11c589498 100644 > > --- a/drivers/pci/pci-sysfs.c > > +++ b/drivers/pci/pci-sysfs.c > > @@ -1321,6 +1321,30 @@ static ssize_t reset_store(struct device *dev, > > struct device_attribute *attr, > > > > static DEVICE_ATTR(reset, 0200, NULL, reset_store); > > > > +static ssize_t reset_bus_store(struct device *dev, struct device_attribute > > *attr, > > + const char *buf, size_t count) > > +{ > > + struct pci_dev *pdev = to_pci_dev(dev); > > + unsigned long val; > > + ssize_t result = kstrtoul(buf, 0, &val); > > + > > + if (result < 0) > > + return result; > > + > > + if (val != 1) > > + return -EINVAL; > > + > > + pm_runtime_get_sync(dev); > > + result = pci_bridge_secondary_bus_reset(pdev); > > + pm_runtime_put(dev); > > + if (result < 0) > > + return result; > > + > > + return count; > > +} > > + > > +static DEVICE_ATTR(reset_bus, 0200, NULL, reset_bus_store); > > + > > static int pci_create_capabilities_sysfs(struct pci_dev *dev) > > { > > int retval; > > @@ -1332,8 +1356,15 @@ static int pci_create_capabilities_sysfs(struct > > pci_dev *dev) > > if (retval) > > goto error; > > } > > + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) { > > + retval = device_create_file(&dev->dev, &dev_attr_reset_bus); > > + if (retval) > > + goto error_reset_bus; > > + } > > return 0; > > > > +error_reset_bus: > > + device_remove_file(&dev->dev, &dev_attr_reset); > > error: > > pcie_vpd_remove_sysfs_dev_files(dev); > > return retval; > > @@ -1414,6 +1445,8 @@ static void pci_remove_capabilities_sysfs(struct > > pci_dev *dev) > > device_remove_file(&dev->dev, &dev_attr_reset); > > dev->reset_fn = 0; > > } > > + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) > > + device_remove_file(&dev->dev, &dev_attr_reset_bus); > > } > > > > /** >
