From: Bjorn Helgaas <helgaas@kernel.org>
To: Rajat Jain <rajatja@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
"Rafael J. Wysocki" <rafael@kernel.org>,
Bjorn Helgaas <bhelgaas@google.com>,
Alan Stern <stern@rowland.harvard.edu>,
linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
linux-usb@vger.kernel.org, Rajat Jain <rajatxjain@gmail.com>,
Jesse Barnes <jsbarnes@google.com>,
Dmitry Torokhov <dtor@google.com>,
Oliver Neukum <oneukum@suse.com>,
David Laight <David.Laight@ACULAB.COM>
Subject: Re: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
Date: Tue, 11 May 2021 16:30:47 -0500 [thread overview]
Message-ID: <20210511213047.GA2417208@bjorn-Precision-5520> (raw)
In-Reply-To: <20210424021631.1972022-2-rajatja@google.com>
[+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@google.com>
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
>
next prev parent reply other threads:[~2021-05-11 21:30 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-24 2:16 [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core Rajat Jain
2021-04-24 2:16 ` [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices Rajat Jain
2021-04-26 9:17 ` Oliver Neukum
2021-04-26 11:49 ` Rafael J. Wysocki
2021-04-26 13:01 ` David Laight
2021-04-26 19:47 ` Rajat Jain
2021-04-27 11:59 ` Oliver Neukum
2021-04-27 12:59 ` David Laight
2021-04-28 6:56 ` Oliver Neukum
2021-04-28 12:21 ` Rafael J. Wysocki
2021-04-29 9:03 ` Oliver Neukum
2021-04-29 9:57 ` Rafael J. Wysocki
2021-04-29 16:59 ` Rajat Jain
2021-05-11 21:30 ` Bjorn Helgaas [this message]
2021-05-11 22:15 ` Rajat Jain
2021-05-11 23:02 ` Bjorn Helgaas
2021-05-12 0:02 ` Rajat Jain
2021-05-12 22:28 ` Rajat Jain
2021-04-25 14:58 ` [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core Alan Stern
2021-05-11 19:22 ` Bjorn Helgaas
2021-05-11 21:36 ` Rajat Jain
2021-05-12 1:00 ` Krzysztof Wilczyński
2021-05-12 1:20 ` Rajat Jain
2021-05-12 22:27 ` Rajat Jain
2021-05-12 23:32 ` Krzysztof Wilczyński
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210511213047.GA2417208@bjorn-Precision-5520 \
--to=helgaas@kernel.org \
--cc=David.Laight@ACULAB.COM \
--cc=bhelgaas@google.com \
--cc=dtor@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=jsbarnes@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=oneukum@suse.com \
--cc=rafael@kernel.org \
--cc=rajatja@google.com \
--cc=rajatxjain@gmail.com \
--cc=stern@rowland.harvard.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.