| Message ID | 20241025222755.3756162-1-kbusch@meta.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv2,1/2] pci: provide bus reset attribute | expand |
On Fri, 25 Oct 2024 15:27:54 -0700 Keith Busch <kbusch@meta.com> 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. > > Provide an attribute on the pci_dev bridge 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 > action. > > Signed-off-by: Keith Busch <kbusch@kernel.org> > --- > v1->v2: > > Moved the attribute from the pci_bus to the bridge's pci_dev > > And renamed it to "reset_subordinate" to distinguish from other > existing device "reset" attributes. > > Added documentation. > > Follow up patch to warn if the action was potentially harmful. > > Documentation/ABI/testing/sysfs-bus-pci | 11 +++++++++++ > drivers/pci/pci-sysfs.c | 23 +++++++++++++++++++++++ > drivers/pci/pci.c | 2 +- > drivers/pci/pci.h | 1 + > 4 files changed, 36 insertions(+), 1 deletion(-) Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
On 24/10/25 03:27pm, 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. > > Provide an attribute on the pci_dev bridge 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 > action. > > Signed-off-by: Keith Busch <kbusch@kernel.org> Reviewed-by: Amey Narkhede <ameynarkhede03@gmail.com>
On Fri, Oct 25, 2024 at 03:27:54PM -0700, Keith Busch wrote: > Resetting a bus from an end device only works if it's the only function > on or below that bus. > > Provide an attribute on the pci_dev bridge 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 > action. Hi Bjorn, just want to check in. Do you have concerns remaining for this feature?
Hello, > > Resetting a bus from an end device only works if it's the only function > > on or below that bus. > > > > Provide an attribute on the pci_dev bridge 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 > > action. > > Hi Bjorn, just want to check in. Do you have concerns remaining for this > feature? Would you have anything against if we put this new bus reset sysfs object access behind the following test? if (!capable(CAP_SYS_ADMIN)) return -EPERM; This is irregardless of what the permissions on the sysfs objects from the DAC point of view are set to. Checking CAP_SYS_ADMIN capability, to improve our default security stance, on a number of important sysfs objects (e.g., reset, remove, etc.) we have was something I discussed in the past with Bjorn, but never got around to sending a patch to add this check. Thoughts? Krzysztof
On Tue, Nov 05, 2024 at 06:53:32AM +0900, Krzysztof Wilczy´nski wrote: > Would you have anything against if we put this new bus reset sysfs object > access behind the following test? > > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > > This is irregardless of what the permissions on the sysfs objects from the > DAC point of view are set to. > > Checking CAP_SYS_ADMIN capability, to improve our default security stance, > on a number of important sysfs objects (e.g., reset, remove, etc.) we have > was something I discussed in the past with Bjorn, but never got around to > sending a patch to add this check. > > Thoughts? Sure, I'm okay that. We are using DEVICE_ATTR_WO file attribute which says should make it writable only by an admin, but totally fine with adding this explicit check here too.
On 24-11-04 14:58:38, Keith Busch wrote: > On Tue, Nov 05, 2024 at 06:53:32AM +0900, Krzysztof Wilczy´nski wrote: > > Would you have anything against if we put this new bus reset sysfs object > > access behind the following test? > > > > if (!capable(CAP_SYS_ADMIN)) > > return -EPERM; > > > > This is irregardless of what the permissions on the sysfs objects from the > > DAC point of view are set to. > > > > Checking CAP_SYS_ADMIN capability, to improve our default security stance, > > on a number of important sysfs objects (e.g., reset, remove, etc.) we have > > was something I discussed in the past with Bjorn, but never got around to > > sending a patch to add this check. > > > > Thoughts? > > Sure, I'm okay that. We are using DEVICE_ATTR_WO file attribute which > says should make it writable only by an admin, but totally fine with > adding this explicit check here too. Thank you! Depending on whether Bjorn will have any feedback to might prompt a new version of the patches to be sent, if there won't be one, then I will add this extra check directly on the branch. Krzysztof
diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci index 7f63c7e977735..5da6a14dc326b 100644 --- a/Documentation/ABI/testing/sysfs-bus-pci +++ b/Documentation/ABI/testing/sysfs-bus-pci @@ -163,6 +163,17 @@ Description: will be present in sysfs. Writing 1 to this file will perform reset. +What: /sys/bus/pci/devices/.../reset_subordinate +Date: October 2024 +Contact: linux-pci@vger.kernel.org +Description: + This is visible only for bridge devices. If you want to reset + all devices attached through the subordinate bus of a specific + bridge device, writing 1 to this will try to do it. This will + affect all devices attached to the system through this bridge + similiar to writing 1 to their individual "reset" file, so use + with caution. + What: /sys/bus/pci/devices/.../vpd Date: February 2008 Contact: Ben Hutchings <bwh@kernel.org> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index 5d0f4db1cab78..480a99e50612b 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 reset_subordinate_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct pci_dev *pdev = to_pci_dev(dev); + struct pci_bus *bus = pdev->subordinate; + 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 DEVICE_ATTR_WO(reset_subordinate); + #if defined(CONFIG_PM) && defined(CONFIG_ACPI) static ssize_t d3cold_allowed_store(struct device *dev, struct device_attribute *attr, @@ -625,6 +647,7 @@ static struct attribute *pci_dev_attrs[] = { static struct attribute *pci_bridge_attrs[] = { &dev_attr_subordinate_bus_number.attr, &dev_attr_secondary_bus_number.attr, + &dev_attr_reset_subordinate.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;