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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).