[+cc Oliver, David]
Please update the subject line, e.g.,
PCI: Add sysfs "removable" attribute
On Fri, Apr 23, 2021 at 07:16:31PM -0700, Rajat Jain wrote:
> Export the already available info, to the userspace via the
> device core, so that userspace can implement whatever policies it
> wants to, for external removable devices.
I know it's not strictly part of *this* patch, but I think we should
connect the dots a little here, something like this:
PCI: Add sysfs "removable" attribute
A PCI device is "external_facing" if it's a Root Port with the ACPI
"ExternalFacingPort" property or if it has the DT "external-facing"
property. We consider everything downstream from such a device to
be removable.
Set pci_dev_type.supports_removable so the device core exposes the
"removable" file in sysfs, and tell the device core about removable
devices.
Wrap to fill 75 columns.
> Signed-off-by: Rajat Jain <rajatja@xxxxxxxxxx>
This looks like a good start. I think it would be useful to have a
more concrete example of how this information will be used. I know
that use would be in userspace, so an example probably would not be a
kernel patch. If you have user code published anywhere, that would
help. Or even a patch to an existing daemon. Or pointers to how
"removable" is used for USB devices.
> ---
> v2: Add documentation
>
> Documentation/ABI/testing/sysfs-devices-removable | 3 ++-
> drivers/pci/pci-sysfs.c | 1 +
> drivers/pci/probe.c | 12 ++++++++++++
> 3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable
> index e13dddd547b5..daac4f007619 100644
> --- a/Documentation/ABI/testing/sysfs-devices-removable
> +++ b/Documentation/ABI/testing/sysfs-devices-removable
> @@ -14,4 +14,5 @@ Description:
>
> Currently this is only supported by USB (which infers the
> information from a combination of hub descriptor bits and
> - platform-specific data such as ACPI).
> + platform-specific data such as ACPI) and PCI (which gets this
> + from ACPI / device tree).
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index f8afd54ca3e1..9302f0076e73 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1582,4 +1582,5 @@ static const struct attribute_group *pci_dev_attr_groups[] = {
>
> const struct device_type pci_dev_type = {
> .groups = pci_dev_attr_groups,
> + .supports_removable = true,
> };
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 953f15abc850..d1cceee62e1b 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1575,6 +1575,16 @@ static void set_pcie_untrusted(struct pci_dev *dev)
> dev->untrusted = true;
> }
>
> +static void set_pci_dev_removable(struct pci_dev *dev)
Maybe just "pci_set_removable()"? These "set_pci*" functions look a
little weird.
> +{
> + struct pci_dev *parent = pci_upstream_bridge(dev);
> + if (parent &&
> + (parent->external_facing || dev_is_removable(&parent->dev)))
> + dev_set_removable(&dev->dev, DEVICE_REMOVABLE);
> + else
> + dev_set_removable(&dev->dev, DEVICE_FIXED);
> +}
> +
> /**
> * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config?
> * @dev: PCI device
> @@ -1819,6 +1829,8 @@ int pci_setup_device(struct pci_dev *dev)
> /* "Unknown power state" */
> dev->current_state = PCI_UNKNOWN;
>
> + set_pci_dev_removable(dev);
So this *only* sets the "removable" attribute based on the
ExternalFacingPort or external-facing properties. I think Oliver and
David were hinting that maybe we should also set it for devices in
hotpluggable slots. What do you think?
I wonder if this (and similar hooks like set_pcie_port_type(),
set_pcie_untrusted(), set_pcie_thunderbolt(), etc) should go *after*
the early fixups so we could use fixups to work around issues?
> /* Early fixups, before probing the BARs */
> pci_fixup_device(pci_fixup_early, dev);
>
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>
