[PATCH v3 1/2] driver core: Move the "removable" attribute from USB to core

[PATCH v3 1/2] driver core: Move the "removable" attribute from USB to core

[Rajat Jain]

Move the "removable" attribute from USB to core in order to allow it to be
supported by other subsystem / buses. Individual buses that want to support
this attribute can opt-in by setting the supports_removable flag, and then
populating the removable property of the device while enumerating it. The
UAPI (location, symantics etc) for the attribute remains unchanged.

Signed-off-by: Rajat Jain <rajatja@google.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
---
v3: - Minor commit log / comments updated.
    - use sysfs_emit()
    - Rename local variable name (state -> loc)
    - change supports_removable flag from bool to bitfield.
v2: Add documentation

 Documentation/ABI/testing/sysfs-bus-usb       | 11 -------
 .../ABI/testing/sysfs-devices-removable       | 17 ++++++++++
 drivers/base/core.c                           | 28 ++++++++++++++++
 drivers/usb/core/hub.c                        |  8 ++---
 drivers/usb/core/sysfs.c                      | 24 --------------
 drivers/usb/core/usb.c                        |  1 +
 include/linux/device.h                        | 32 +++++++++++++++++++
 include/linux/usb.h                           |  7 ----
 8 files changed, 82 insertions(+), 46 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-removable

diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
index bf2c1968525f..73eb23bc1f34 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -154,17 +154,6 @@ Description:
 		files hold a string value (enable or disable) indicating whether
 		or not USB3 hardware LPM U1 or U2 is enabled for the device.
 
-What:		/sys/bus/usb/devices/.../removable
-Date:		February 2012
-Contact:	Matthew Garrett <mjg@redhat.com>
-Description:
-		Some information about whether a given USB device is
-		physically fixed to the platform can be inferred from a
-		combination of hub descriptor bits and platform-specific data
-		such as ACPI. This file will read either "removable" or
-		"fixed" if the information is available, and "unknown"
-		otherwise.
-
 What:		/sys/bus/usb/devices/.../ltm_capable
 Date:		July 2012
 Contact:	Sarah Sharp <sarah.a.sharp@linux.intel.com>
diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable
new file mode 100644
index 000000000000..9dabcad7cdcd
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-removable
@@ -0,0 +1,17 @@
+What:		/sys/devices/.../removable
+Date:		Apr 2021
+Contact:	Matthew Garrett <mjg@redhat.com>,
+		Rajat Jain <rajatja@google.com>
+Description:
+		Information about whether a given device is physically fixed to
+		the platform. This is determined by the device's subsystem in a
+		bus / platform-specific way. This attribute is only present for
+		buses that can support determining such information:
+
+		"removable": The device is external / removable from the system.
+		"fixed":     The device is internal / fixed to the system.
+		"unknown":   The information is unavailable.
+
+		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).
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4a8bf8cda52b..9e6bf9e71a7e 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2404,6 +2404,25 @@ static ssize_t online_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(online);
 
+static ssize_t removable_show(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	const char *loc;
+
+	switch (dev->removable) {
+	case DEVICE_REMOVABLE:
+		loc = "removable";
+		break;
+	case DEVICE_FIXED:
+		loc = "fixed";
+		break;
+	default:
+		loc = "unknown";
+	}
+	return sysfs_emit(buf, "%s\n", loc);
+}
+static DEVICE_ATTR_RO(removable);
+
 int device_add_groups(struct device *dev, const struct attribute_group **groups)
 {
 	return sysfs_create_groups(&dev->kobj, groups);
@@ -2581,8 +2600,16 @@ static int device_add_attrs(struct device *dev)
 			goto err_remove_dev_online;
 	}
 
+	if (type && type->supports_removable) {
+		error = device_create_file(dev, &dev_attr_removable);
+		if (error)
+			goto err_remove_dev_waiting_for_supplier;
+	}
+
 	return 0;
 
+ err_remove_dev_waiting_for_supplier:
+	device_remove_file(dev, &dev_attr_waiting_for_supplier);
  err_remove_dev_online:
 	device_remove_file(dev, &dev_attr_online);
  err_remove_dev_groups:
@@ -2602,6 +2629,7 @@ static void device_remove_attrs(struct device *dev)
 	struct class *class = dev->class;
 	const struct device_type *type = dev->type;
 
+	device_remove_file(dev, &dev_attr_removable);
 	device_remove_file(dev, &dev_attr_waiting_for_supplier);
 	device_remove_file(dev, &dev_attr_online);
 	device_remove_groups(dev, dev->groups);
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index b2bc4b7c4289..7a3c28b14ca1 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2443,11 +2443,11 @@ static void set_usb_port_removable(struct usb_device *udev)
 	 */
 	switch (hub->ports[udev->portnum - 1]->connect_type) {
 	case USB_PORT_CONNECT_TYPE_HOT_PLUG:
-		udev->removable = USB_DEVICE_REMOVABLE;
+		dev_set_removable(&udev->dev, DEVICE_REMOVABLE);
 		return;
 	case USB_PORT_CONNECT_TYPE_HARD_WIRED:
 	case USB_PORT_NOT_USED:
-		udev->removable = USB_DEVICE_FIXED;
+		dev_set_removable(&udev->dev, DEVICE_FIXED);
 		return;
 	default:
 		break;
@@ -2472,9 +2472,9 @@ static void set_usb_port_removable(struct usb_device *udev)
 	}
 
 	if (removable)
-		udev->removable = USB_DEVICE_REMOVABLE;
+		dev_set_removable(&udev->dev, DEVICE_REMOVABLE);
 	else
-		udev->removable = USB_DEVICE_FIXED;
+		dev_set_removable(&udev->dev, DEVICE_FIXED);
 
 }
 
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index 5a168ba9fc51..fa2e49d432ff 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -301,29 +301,6 @@ static ssize_t urbnum_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(urbnum);
 
-static ssize_t removable_show(struct device *dev, struct device_attribute *attr,
-			      char *buf)
-{
-	struct usb_device *udev;
-	char *state;
-
-	udev = to_usb_device(dev);
-
-	switch (udev->removable) {
-	case USB_DEVICE_REMOVABLE:
-		state = "removable";
-		break;
-	case USB_DEVICE_FIXED:
-		state = "fixed";
-		break;
-	default:
-		state = "unknown";
-	}
-
-	return sprintf(buf, "%s\n", state);
-}
-static DEVICE_ATTR_RO(removable);
-
 static ssize_t ltm_capable_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
@@ -828,7 +805,6 @@ static struct attribute *dev_attrs[] = {
 	&dev_attr_avoid_reset_quirk.attr,
 	&dev_attr_authorized.attr,
 	&dev_attr_remove.attr,
-	&dev_attr_removable.attr,
 	&dev_attr_ltm_capable.attr,
 #ifdef CONFIG_OF
 	&dev_attr_devspec.attr,
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 62368c4ed37a..ce18e84528cf 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -569,6 +569,7 @@ struct device_type usb_device_type = {
 #ifdef CONFIG_PM
 	.pm =		&usb_device_pm_ops,
 #endif
+	.supports_removable = true,
 };
 
 
diff --git a/include/linux/device.h b/include/linux/device.h
index 38a2071cf776..7e87ab048307 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -93,6 +93,8 @@ struct device_type {
 	void (*release)(struct device *dev);
 
 	const struct dev_pm_ops *pm;
+
+	bool supports_removable:1; /* subsystem can classify removable/fixed */
 };
 
 /* interface for exporting device attributes */
@@ -350,6 +352,19 @@ enum dl_dev_state {
 	DL_DEV_UNBINDING,
 };
 
+/**
+ * enum device_removable - Whether the device is removable. The criteria for a
+ * device to be classified as removable is determined by its subsystem or bus.
+ * @DEVICE_REMOVABLE_UNKNOWN:  Device location is Unknown (default).
+ * @DEVICE_REMOVABLE: Device is removable by the user.
+ * @DEVICE_FIXED: Device is not removable by the user.
+ */
+enum device_removable {
+	DEVICE_REMOVABLE_UNKNOWN = 0,
+	DEVICE_REMOVABLE,
+	DEVICE_FIXED,
+};
+
 /**
  * struct dev_links_info - Device data related to device links.
  * @suppliers: List of links to supplier devices.
@@ -431,6 +446,9 @@ struct dev_links_info {
  * 		device (i.e. the bus driver that discovered the device).
  * @iommu_group: IOMMU group the device belongs to.
  * @iommu:	Per device generic IOMMU runtime data
+ * @removable:  Whether the device can be removed from the system. This
+ *              should be set by the subsystem / bus driver that discovered
+ *              the device.
  *
  * @offline_disabled: If set, the device is permanently online.
  * @offline:	Set after successful invocation of bus type's .offline().
@@ -544,6 +562,8 @@ struct device {
 	struct iommu_group	*iommu_group;
 	struct dev_iommu	*iommu;
 
+	enum device_removable	removable;
+
 	bool			offline_disabled:1;
 	bool			offline:1;
 	bool			of_node_reused:1;
@@ -782,6 +802,18 @@ static inline bool dev_has_sync_state(struct device *dev)
 	return false;
 }
 
+static inline void dev_set_removable(struct device *dev,
+				     enum device_removable removable)
+{
+	dev->removable = removable;
+}
+
+static inline bool dev_is_removable(struct device *dev)
+{
+	return dev && dev->type && dev->type->supports_removable
+	    && dev->removable == DEVICE_REMOVABLE;
+}
+
 /*
  * High level routines for use by the bus drivers
  */
diff --git a/include/linux/usb.h b/include/linux/usb.h
index eaae24217e8a..b9bd664f44a1 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -473,12 +473,6 @@ struct usb_dev_state;
 
 struct usb_tt;
 
-enum usb_device_removable {
-	USB_DEVICE_REMOVABLE_UNKNOWN = 0,
-	USB_DEVICE_REMOVABLE,
-	USB_DEVICE_FIXED,
-};
-
 enum usb_port_connect_type {
 	USB_PORT_CONNECT_TYPE_UNKNOWN = 0,
 	USB_PORT_CONNECT_TYPE_HOT_PLUG,
@@ -703,7 +697,6 @@ struct usb_device {
 #endif
 	struct wusb_dev *wusb_dev;
 	int slot_id;
-	enum usb_device_removable removable;
 	struct usb2_lpm_parameters l1_params;
 	struct usb3_lpm_parameters u1_params;
 	struct usb3_lpm_parameters u2_params;
-- 
2.31.1.607.g51e8a6a459-goog

[PATCH v3 2/2] PCI: Add sysfs "removable" attribute

[Rajat Jain]

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 by user.

We're mainly concerned with consumer platforms with user accessible
thunderbolt ports that are vulnerable to DMA attacks, and we expect those
ports to be identified as "ExternalFacingPort". Devices in traditional
hotplug slots can technically be removed, but the expectation is that
unless the port is marked with "ExternalFacingPort", such devices are less
accessible to user / may not be removed by end user, and thus not exposed
as "removable" to userspace.

Set pci_dev_type.supports_removable so the device core exposes the
"removable" file in sysfs, and tell the device core about removable
devices.

This can be used by userspace to implment any policies it wants to,
tailored specifically for user removable devices. Eg usage:
https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812
https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038
(code uses such an attribute to remove external PCI devicces or disable
features on them as needed by the policy desired)

Signed-off-by: Rajat Jain <rajatja@google.com>
---
v3: - commit log updated
    - Rename set_pci_dev_removable() -> pci_set_removable()
    - Call it after applying early PCI quirks.
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 9dabcad7cdcd..ec0b243f5db4 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 beb8d1f4fafe..38b3259ba333 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1541,4 +1541,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 3a62d09b8869..3515afeeaba8 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 pci_set_removable(struct pci_dev *dev)
+{
+	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
@@ -1822,6 +1832,8 @@ int pci_setup_device(struct pci_dev *dev)
 	/* Early fixups, before probing the BARs */
 	pci_fixup_device(pci_fixup_early, dev);
 
+	pci_set_removable(dev);
+
 	pci_info(dev, "[%04x:%04x] type %02x class %#08x\n",
 		 dev->vendor, dev->device, dev->hdr_type, dev->class);
 
-- 
2.31.1.607.g51e8a6a459-goog

Re: [PATCH v3 2/2] PCI: Add sysfs "removable" attribute

[Greg Kroah-Hartman]

On Wed, May 12, 2021 at 02:34:57PM -0700, Rajat Jain wrote:
> 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 by user.
> 
> We're mainly concerned with consumer platforms with user accessible
> thunderbolt ports that are vulnerable to DMA attacks, and we expect those
> ports to be identified as "ExternalFacingPort". Devices in traditional
> hotplug slots can technically be removed, but the expectation is that
> unless the port is marked with "ExternalFacingPort", such devices are less
> accessible to user / may not be removed by end user, and thus not exposed
> as "removable" to userspace.
> 
> Set pci_dev_type.supports_removable so the device core exposes the
> "removable" file in sysfs, and tell the device core about removable
> devices.
> 
> This can be used by userspace to implment any policies it wants to,
> tailored specifically for user removable devices. Eg usage:
> https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812
> https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038
> (code uses such an attribute to remove external PCI devicces or disable
> features on them as needed by the policy desired)
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v3: - commit log updated
>     - Rename set_pci_dev_removable() -> pci_set_removable()
>     - Call it after applying early PCI quirks.
> 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 9dabcad7cdcd..ec0b243f5db4 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 beb8d1f4fafe..38b3259ba333 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1541,4 +1541,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 3a62d09b8869..3515afeeaba8 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 pci_set_removable(struct pci_dev *dev)
> +{
> +	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);
> +}

Always run checkpatch.pl so you don't get grumpy maintainers telling you
to run checkpatch.pl :(

And why does external_facing come into play here?  I know you say it
above, but you should also put it here into the code for when we need to
look at it in a few months and wonder what in the world this is doing.

Also, are you SURE this is correct and will handle the hotpluggable PCI
devices in things like drawers and the like?

What is the goal here in exposing this information to userspace, who is
going to use it and what is it going to be used for?


> +
>  /**
>   * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config?
>   * @dev: PCI device
> @@ -1822,6 +1832,8 @@ int pci_setup_device(struct pci_dev *dev)
>  	/* Early fixups, before probing the BARs */
>  	pci_fixup_device(pci_fixup_early, dev);
>  
> +	pci_set_removable(dev);
> +
>  	pci_info(dev, "[%04x:%04x] type %02x class %#08x\n",
>  		 dev->vendor, dev->device, dev->hdr_type, dev->class);
>  
> -- 
> 2.31.1.607.g51e8a6a459-goog
> 

Re: [PATCH v3 2/2] PCI: Add sysfs "removable" attribute

[Rajat Jain]

Hello,

On Thu, May 13, 2021 at 6:58 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, May 12, 2021 at 02:34:57PM -0700, Rajat Jain wrote:
> > 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 by user.
> >
> > We're mainly concerned with consumer platforms with user accessible
> > thunderbolt ports that are vulnerable to DMA attacks, and we expect those
> > ports to be identified as "ExternalFacingPort". Devices in traditional
> > hotplug slots can technically be removed, but the expectation is that
> > unless the port is marked with "ExternalFacingPort", such devices are less
> > accessible to user / may not be removed by end user, and thus not exposed
> > as "removable" to userspace.
> >
> > Set pci_dev_type.supports_removable so the device core exposes the
> > "removable" file in sysfs, and tell the device core about removable
> > devices.
> >
> > This can be used by userspace to implment any policies it wants to,
> > tailored specifically for user removable devices. Eg usage:
> > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812
> > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038
> > (code uses such an attribute to remove external PCI devicces or disable
> > features on them as needed by the policy desired)
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> > v3: - commit log updated
> >     - Rename set_pci_dev_removable() -> pci_set_removable()
> >     - Call it after applying early PCI quirks.
> > 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 9dabcad7cdcd..ec0b243f5db4 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 beb8d1f4fafe..38b3259ba333 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -1541,4 +1541,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 3a62d09b8869..3515afeeaba8 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 pci_set_removable(struct pci_dev *dev)
> > +{
> > +     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);
> > +}
>
> Always run checkpatch.pl so you don't get grumpy maintainers telling you
> to run checkpatch.pl :(

Yes, I did (it gave me 0 errors and 0 warnings). Please let me know if
I need to fix something and I'll be happy to fix that.

>
> And why does external_facing come into play here?  I know you say it
> above, but you should also put it here into the code for when we need to
> look at it in a few months and wonder what in the world this is doing.

Ack, will do.

>
> Also, are you SURE this is correct and will handle the hotpluggable PCI
> devices in things like drawers and the like?

Yes, me and Bjorn discussed this in the v2 of this patch
(https://patchwork.kernel.org/project/linux-usb/patch/20210424021631.1972022-2-rajatja@google.com/),
and yes, this can take care of the hot-pluggable trays if the firmware
marks the slots external-facing.

>
> What is the goal here in exposing this information to userspace, who is
> going to use it and what is it going to be used for?

The goal here is to implement policies regarding usage of external PCI
devices, in userspace. ChromeOS is using it for things like:
- Remove external PCI devices when a user logs out.
- Don't allow new external PCI devices while the screen is locked.
- collect metrics about usage of external PCI devices (how many users
actually use it etc).
- disable certain features (that are deemed to be dangerous) for
external PCI network cards.
- etc.

Thanks,

Rajat

>
>
> > +
> >  /**
> >   * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config?
> >   * @dev: PCI device
> > @@ -1822,6 +1832,8 @@ int pci_setup_device(struct pci_dev *dev)
> >       /* Early fixups, before probing the BARs */
> >       pci_fixup_device(pci_fixup_early, dev);
> >
> > +     pci_set_removable(dev);
> > +
> >       pci_info(dev, "[%04x:%04x] type %02x class %#08x\n",
> >                dev->vendor, dev->device, dev->hdr_type, dev->class);
> >
> > --
> > 2.31.1.607.g51e8a6a459-goog
> >

Re: [PATCH v3 2/2] PCI: Add sysfs "removable" attribute

[Greg Kroah-Hartman]

On Thu, May 13, 2021 at 09:39:58AM -0700, Rajat Jain wrote:
> Hello,
> 
> On Thu, May 13, 2021 at 6:58 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, May 12, 2021 at 02:34:57PM -0700, Rajat Jain wrote:
> > > 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 by user.
> > >
> > > We're mainly concerned with consumer platforms with user accessible
> > > thunderbolt ports that are vulnerable to DMA attacks, and we expect those
> > > ports to be identified as "ExternalFacingPort". Devices in traditional
> > > hotplug slots can technically be removed, but the expectation is that
> > > unless the port is marked with "ExternalFacingPort", such devices are less
> > > accessible to user / may not be removed by end user, and thus not exposed
> > > as "removable" to userspace.
> > >
> > > Set pci_dev_type.supports_removable so the device core exposes the
> > > "removable" file in sysfs, and tell the device core about removable
> > > devices.
> > >
> > > This can be used by userspace to implment any policies it wants to,
> > > tailored specifically for user removable devices. Eg usage:
> > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812
> > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038
> > > (code uses such an attribute to remove external PCI devicces or disable
> > > features on them as needed by the policy desired)
> > >
> > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > ---
> > > v3: - commit log updated
> > >     - Rename set_pci_dev_removable() -> pci_set_removable()
> > >     - Call it after applying early PCI quirks.
> > > 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 9dabcad7cdcd..ec0b243f5db4 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 beb8d1f4fafe..38b3259ba333 100644
> > > --- a/drivers/pci/pci-sysfs.c
> > > +++ b/drivers/pci/pci-sysfs.c
> > > @@ -1541,4 +1541,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 3a62d09b8869..3515afeeaba8 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 pci_set_removable(struct pci_dev *dev)
> > > +{
> > > +     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);
> > > +}
> >
> > Always run checkpatch.pl so you don't get grumpy maintainers telling you
> > to run checkpatch.pl :(
> 
> Yes, I did (it gave me 0 errors and 0 warnings). Please let me know if
> I need to fix something and I'll be happy to fix that.
> 
> >
> > And why does external_facing come into play here?  I know you say it
> > above, but you should also put it here into the code for when we need to
> > look at it in a few months and wonder what in the world this is doing.
> 
> Ack, will do.
> 
> >
> > Also, are you SURE this is correct and will handle the hotpluggable PCI
> > devices in things like drawers and the like?
> 
> Yes, me and Bjorn discussed this in the v2 of this patch
> (https://patchwork.kernel.org/project/linux-usb/patch/20210424021631.1972022-2-rajatja@google.com/),
> and yes, this can take care of the hot-pluggable trays if the firmware
> marks the slots external-facing.

Ok, I'll trust you two :)

> > What is the goal here in exposing this information to userspace, who is
> > going to use it and what is it going to be used for?
> 
> The goal here is to implement policies regarding usage of external PCI
> devices, in userspace. ChromeOS is using it for things like:
> - Remove external PCI devices when a user logs out.

remove them how?  disconnect the device from the system through what
method?

> - Don't allow new external PCI devices while the screen is locked.

Don't allow how?  Don't allow the binding of a driver to a device, or
the device to be discovered at all?  What controls this?

> - collect metrics about usage of external PCI devices (how many users
> actually use it etc).
> - disable certain features (that are deemed to be dangerous) for
> external PCI network cards.

What is a "dangerous" network feature, RDMA?

thanks,

greg k-h

Re: [PATCH v3 2/2] PCI: Add sysfs "removable" attribute

[Rajat Jain]

On Thu, May 13, 2021 at 10:42 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, May 13, 2021 at 09:39:58AM -0700, Rajat Jain wrote:
> > Hello,
> >
> > On Thu, May 13, 2021 at 6:58 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, May 12, 2021 at 02:34:57PM -0700, Rajat Jain wrote:
> > > > 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 by user.
> > > >
> > > > We're mainly concerned with consumer platforms with user accessible
> > > > thunderbolt ports that are vulnerable to DMA attacks, and we expect those
> > > > ports to be identified as "ExternalFacingPort". Devices in traditional
> > > > hotplug slots can technically be removed, but the expectation is that
> > > > unless the port is marked with "ExternalFacingPort", such devices are less
> > > > accessible to user / may not be removed by end user, and thus not exposed
> > > > as "removable" to userspace.
> > > >
> > > > Set pci_dev_type.supports_removable so the device core exposes the
> > > > "removable" file in sysfs, and tell the device core about removable
> > > > devices.
> > > >
> > > > This can be used by userspace to implment any policies it wants to,
> > > > tailored specifically for user removable devices. Eg usage:
> > > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812
> > > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038
> > > > (code uses such an attribute to remove external PCI devicces or disable
> > > > features on them as needed by the policy desired)
> > > >
> > > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > > ---
> > > > v3: - commit log updated
> > > >     - Rename set_pci_dev_removable() -> pci_set_removable()
> > > >     - Call it after applying early PCI quirks.
> > > > 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 9dabcad7cdcd..ec0b243f5db4 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 beb8d1f4fafe..38b3259ba333 100644
> > > > --- a/drivers/pci/pci-sysfs.c
> > > > +++ b/drivers/pci/pci-sysfs.c
> > > > @@ -1541,4 +1541,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 3a62d09b8869..3515afeeaba8 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 pci_set_removable(struct pci_dev *dev)
> > > > +{
> > > > +     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);
> > > > +}
> > >
> > > Always run checkpatch.pl so you don't get grumpy maintainers telling you
> > > to run checkpatch.pl :(
> >
> > Yes, I did (it gave me 0 errors and 0 warnings). Please let me know if
> > I need to fix something and I'll be happy to fix that.
> >
> > >
> > > And why does external_facing come into play here?  I know you say it
> > > above, but you should also put it here into the code for when we need to
> > > look at it in a few months and wonder what in the world this is doing.
> >
> > Ack, will do.
> >
> > >
> > > Also, are you SURE this is correct and will handle the hotpluggable PCI
> > > devices in things like drawers and the like?
> >
> > Yes, me and Bjorn discussed this in the v2 of this patch
> > (https://patchwork.kernel.org/project/linux-usb/patch/20210424021631.1972022-2-rajatja@google.com/),
> > and yes, this can take care of the hot-pluggable trays if the firmware
> > marks the slots external-facing.
>
> Ok, I'll trust you two :)
>
> > > What is the goal here in exposing this information to userspace, who is
> > > going to use it and what is it going to be used for?
> >
> > The goal here is to implement policies regarding usage of external PCI
> > devices, in userspace. ChromeOS is using it for things like:
> > - Remove external PCI devices when a user logs out.
>
> remove them how?  disconnect the device from the system through what
> method?

echo 1 > /sys/bus/pci/devices/<device>/remove

>
> > - Don't allow new external PCI devices while the screen is locked.
>
> Don't allow how?  Don't allow the binding of a driver to a device, or
> the device to be discovered at all?  What controls this?

Actually Sorry, this was a wrong recollection.

>
> > - collect metrics about usage of external PCI devices (how many users
> > actually use it etc).
> > - disable certain features (that are deemed to be dangerous) for
> > external PCI network cards.
>
> What is a "dangerous" network feature, RDMA?

For now, we disable offloading of receive path generic / segmentation
/ checksum features to the external PCI hardware, based on our
security team's recommendations.

Thanks,

Rajat

>
> thanks,
>
> greg k-h

Re: [PATCH v3 2/2] PCI: Add sysfs "removable" attribute

[Rajat Jain]

Hi

On Wed, May 12, 2021 at 2:35 PM Rajat Jain <rajatja@google.com> wrote:
>
> 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 by user.
>
> We're mainly concerned with consumer platforms with user accessible
> thunderbolt ports that are vulnerable to DMA attacks, and we expect those
> ports to be identified as "ExternalFacingPort". Devices in traditional
> hotplug slots can technically be removed, but the expectation is that
> unless the port is marked with "ExternalFacingPort", such devices are less
> accessible to user / may not be removed by end user, and thus not exposed
> as "removable" to userspace.
>
> Set pci_dev_type.supports_removable so the device core exposes the
> "removable" file in sysfs, and tell the device core about removable
> devices.
>
> This can be used by userspace to implment any policies it wants to,
> tailored specifically for user removable devices. Eg usage:
> https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812
> https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038
> (code uses such an attribute to remove external PCI devicces or disable
> features on them as needed by the policy desired)
>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v3: - commit log updated
>     - Rename set_pci_dev_removable() -> pci_set_removable()
>     - Call it after applying early PCI quirks.
> 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 9dabcad7cdcd..ec0b243f5db4 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 beb8d1f4fafe..38b3259ba333 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1541,4 +1541,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 3a62d09b8869..3515afeeaba8 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 pci_set_removable(struct pci_dev *dev)
> +{
> +       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);
> +}

Copying comments from Krzysztof from another thread:

[Krzysztof] We were also wondering if we should only set DEVICE_REMOVABLE for
devices known to be behind an external-facing port, and let everything
else be set to "unknown" (or whatever the default would be).

[Rajat]: I think I'm fine with this proposal if Bjorn & PCI community
also sees this as a better idea. Essentially the question here is,
would it be better for the non-removable PCI devices to be shown as
"fixed" or "unknown"?

Thanks,

Rajat

> +
>  /**
>   * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config?
>   * @dev: PCI device
> @@ -1822,6 +1832,8 @@ int pci_setup_device(struct pci_dev *dev)
>         /* Early fixups, before probing the BARs */
>         pci_fixup_device(pci_fixup_early, dev);
>
> +       pci_set_removable(dev);
> +
>         pci_info(dev, "[%04x:%04x] type %02x class %#08x\n",
>                  dev->vendor, dev->device, dev->hdr_type, dev->class);
>
> --
> 2.31.1.607.g51e8a6a459-goog
>

Re: [PATCH v3 2/2] PCI: Add sysfs "removable" attribute

[Bjorn Helgaas]

On Thu, May 13, 2021 at 11:02:10AM -0700, Rajat Jain wrote:
> On Wed, May 12, 2021 at 2:35 PM Rajat Jain <rajatja@google.com> wrote:
> >
> > 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 by user.
> >
> > We're mainly concerned with consumer platforms with user accessible
> > thunderbolt ports that are vulnerable to DMA attacks, and we expect those
> > ports to be identified as "ExternalFacingPort". Devices in traditional
> > hotplug slots can technically be removed, but the expectation is that
> > unless the port is marked with "ExternalFacingPort", such devices are less
> > accessible to user / may not be removed by end user, and thus not exposed
> > as "removable" to userspace.

s/thunderbolt/Thunderbolt/ since I think it's a trademark
s/identified as/identified by firmware as/

> > Set pci_dev_type.supports_removable so the device core exposes the
> > "removable" file in sysfs, and tell the device core about removable
> > devices.
> >
> > This can be used by userspace to implment any policies it wants to,
> > tailored specifically for user removable devices. Eg usage:
> > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812
> > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038
> > (code uses such an attribute to remove external PCI devicces or disable
> > features on them as needed by the policy desired)

s/implment/implement/
s/devicces/devices/

Or maybe something like:

  This can be used to implement userspace policies tailored for
  user-removable devices.

Not sure exactly what "remove external PCI devices" means.  You're
talking about the *code* doing something, so I don't think it means
physically unplugging the device from the system.  Maybe preventing a
driver from binding to it or something similar?

I hesitate slightly to rely on URLs like googlesource.com in commit
logs because we don't know how long they will remain valid.  But I
guess there's no real alternative here, since this code probably
hasn't been posted to any public mailing lists like the ones archived
at https://lore.kernel.org/lists.html, right?

> > Signed-off-by: Rajat Jain <rajatja@google.com>

> > +static void pci_set_removable(struct pci_dev *dev)
> > +{
> > +       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);
> > +}
> 
> Copying comments from Krzysztof from another thread:
> 
> [Krzysztof] We were also wondering if we should only set DEVICE_REMOVABLE for
> devices known to be behind an external-facing port, and let everything
> else be set to "unknown" (or whatever the default would be).
> 
> [Rajat]: I think I'm fine with this proposal if Bjorn & PCI community
> also sees this as a better idea. Essentially the question here is,
> would it be better for the non-removable PCI devices to be shown as
> "fixed" or "unknown"?

I think I would rather see this as:

  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);

In other words, assume only that everything below an "external-facing"
device is removable.

In the absence of an "external-facing" property, we don't know
anything about the connection, and I'd rather use the default
(probably "unknown") instead of assuming "fixed."

I don't think we have anything that depends on "fixed," so I don't
think there's value in setting it.

(Note the blank line between local variables and the "if"; maybe
that's what Greg hinted at?)

Bjorn

Re: [PATCH v3 2/2] PCI: Add sysfs "removable" attribute

[Rajat Jain]

On Thu, May 13, 2021 at 1:05 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, May 13, 2021 at 11:02:10AM -0700, Rajat Jain wrote:
> > On Wed, May 12, 2021 at 2:35 PM Rajat Jain <rajatja@google.com> wrote:
> > >
> > > 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 by user.
> > >
> > > We're mainly concerned with consumer platforms with user accessible
> > > thunderbolt ports that are vulnerable to DMA attacks, and we expect those
> > > ports to be identified as "ExternalFacingPort". Devices in traditional
> > > hotplug slots can technically be removed, but the expectation is that
> > > unless the port is marked with "ExternalFacingPort", such devices are less
> > > accessible to user / may not be removed by end user, and thus not exposed
> > > as "removable" to userspace.
>
> s/thunderbolt/Thunderbolt/ since I think it's a trademark
> s/identified as/identified by firmware as/

Ack, will do.

>
> > > Set pci_dev_type.supports_removable so the device core exposes the
> > > "removable" file in sysfs, and tell the device core about removable
> > > devices.
> > >
> > > This can be used by userspace to implment any policies it wants to,
> > > tailored specifically for user removable devices. Eg usage:
> > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812
> > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038
> > > (code uses such an attribute to remove external PCI devicces or disable
> > > features on them as needed by the policy desired)
>
> s/implment/implement/
> s/devicces/devices/
>
> Or maybe something like:
>
>   This can be used to implement userspace policies tailored for
>   user-removable devices.

Ack, will do.

>
> Not sure exactly what "remove external PCI devices" means.  You're
> talking about the *code* doing something, so I don't think it means
> physically unplugging the device from the system.  Maybe preventing a
> driver from binding to it or something similar?

echo 1 > /sys/bus/pci/devices/<device>/remove

>
> I hesitate slightly to rely on URLs like googlesource.com in commit
> logs because we don't know how long they will remain valid.  But I
> guess there's no real alternative here, since this code probably
> hasn't been posted to any public mailing lists like the ones archived
> at https://lore.kernel.org/lists.html, right?

Yes, chromium reviews (userspace code that shall use the new
attribute) happen over gerrit, and so the publicly available links
would be googlesource.com.

>
> > > Signed-off-by: Rajat Jain <rajatja@google.com>
>
> > > +static void pci_set_removable(struct pci_dev *dev)
> > > +{
> > > +       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);
> > > +}
> >
> > Copying comments from Krzysztof from another thread:
> >
> > [Krzysztof] We were also wondering if we should only set DEVICE_REMOVABLE for
> > devices known to be behind an external-facing port, and let everything
> > else be set to "unknown" (or whatever the default would be).
> >
> > [Rajat]: I think I'm fine with this proposal if Bjorn & PCI community
> > also sees this as a better idea. Essentially the question here is,
> > would it be better for the non-removable PCI devices to be shown as
> > "fixed" or "unknown"?
>
> I think I would rather see this as:
>
>   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);
>
> In other words, assume only that everything below an "external-facing"
> device is removable.
>
> In the absence of an "external-facing" property, we don't know
> anything about the connection, and I'd rather use the default
> (probably "unknown") instead of assuming "fixed."

Ack, will do.

One question: Under Greg's latest suggestion, the decision to show
this attribute does not have to be bus wide / device_type wide i.e.
subsystem can choose for this attribute to show up only under certain
devices. So if it is more preferable, I can have this attribute show
under ONLY PCI devices that attach below "external-facing" PCI devices
(and any other PCI devices will not have this attribute show up at
all). I guess this sounds better than having "unknown" show up on the
rest of the devices that are not removable?

>
> I don't think we have anything that depends on "fixed," so I don't
> think there's value in setting it.
>
> (Note the blank line between local variables and the "if"; maybe
> that's what Greg hinted at?)

Ack, will remove the blank line (didn't know that blank lines between
variables and code is not preferred).

Thanks,
Rajat

>
> Bjorn

Re: [PATCH v3 2/2] PCI: Add sysfs "removable" attribute

[Bjorn Helgaas]

On Thu, May 13, 2021 at 01:34:23PM -0700, Rajat Jain wrote:
> On Thu, May 13, 2021 at 1:05 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Thu, May 13, 2021 at 11:02:10AM -0700, Rajat Jain wrote:
> > > On Wed, May 12, 2021 at 2:35 PM Rajat Jain <rajatja@google.com> wrote:
> > > >
> > > > 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 by user.
> > > >
> > > > We're mainly concerned with consumer platforms with user accessible
> > > > thunderbolt ports that are vulnerable to DMA attacks, and we expect those
> > > > ports to be identified as "ExternalFacingPort". Devices in traditional
> > > > hotplug slots can technically be removed, but the expectation is that
> > > > unless the port is marked with "ExternalFacingPort", such devices are less
> > > > accessible to user / may not be removed by end user, and thus not exposed
> > > > as "removable" to userspace.
> >
> > s/thunderbolt/Thunderbolt/ since I think it's a trademark
> > s/identified as/identified by firmware as/
> 
> Ack, will do.
> 
> >
> > > > Set pci_dev_type.supports_removable so the device core exposes the
> > > > "removable" file in sysfs, and tell the device core about removable
> > > > devices.
> > > >
> > > > This can be used by userspace to implment any policies it wants to,
> > > > tailored specifically for user removable devices. Eg usage:
> > > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812
> > > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038
> > > > (code uses such an attribute to remove external PCI devicces or disable
> > > > features on them as needed by the policy desired)
> >
> > s/implment/implement/
> > s/devicces/devices/
> >
> > Or maybe something like:
> >
> >   This can be used to implement userspace policies tailored for
> >   user-removable devices.
> 
> Ack, will do.
> 
> >
> > Not sure exactly what "remove external PCI devices" means.  You're
> > talking about the *code* doing something, so I don't think it means
> > physically unplugging the device from the system.  Maybe preventing a
> > driver from binding to it or something similar?
> 
> echo 1 > /sys/bus/pci/devices/<device>/remove
> 
> >
> > I hesitate slightly to rely on URLs like googlesource.com in commit
> > logs because we don't know how long they will remain valid.  But I
> > guess there's no real alternative here, since this code probably
> > hasn't been posted to any public mailing lists like the ones archived
> > at https://lore.kernel.org/lists.html, right?
> 
> Yes, chromium reviews (userspace code that shall use the new
> attribute) happen over gerrit, and so the publicly available links
> would be googlesource.com.
> 
> >
> > > > Signed-off-by: Rajat Jain <rajatja@google.com>
> >
> > > > +static void pci_set_removable(struct pci_dev *dev)
> > > > +{
> > > > +       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);
> > > > +}
> > >
> > > Copying comments from Krzysztof from another thread:
> > >
> > > [Krzysztof] We were also wondering if we should only set DEVICE_REMOVABLE for
> > > devices known to be behind an external-facing port, and let everything
> > > else be set to "unknown" (or whatever the default would be).
> > >
> > > [Rajat]: I think I'm fine with this proposal if Bjorn & PCI community
> > > also sees this as a better idea. Essentially the question here is,
> > > would it be better for the non-removable PCI devices to be shown as
> > > "fixed" or "unknown"?
> >
> > I think I would rather see this as:
> >
> >   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);
> >
> > In other words, assume only that everything below an "external-facing"
> > device is removable.
> >
> > In the absence of an "external-facing" property, we don't know
> > anything about the connection, and I'd rather use the default
> > (probably "unknown") instead of assuming "fixed."
> 
> Ack, will do.
> 
> One question: Under Greg's latest suggestion, the decision to show
> this attribute does not have to be bus wide / device_type wide i.e.
> subsystem can choose for this attribute to show up only under certain
> devices. So if it is more preferable, I can have this attribute show
> under ONLY PCI devices that attach below "external-facing" PCI devices
> (and any other PCI devices will not have this attribute show up at
> all). I guess this sounds better than having "unknown" show up on the
> rest of the devices that are not removable?

If you can make the file appear only for removable devices, that
sounds even better.

> > I don't think we have anything that depends on "fixed," so I don't
> > think there's value in setting it.
> >
> > (Note the blank line between local variables and the "if"; maybe
> > that's what Greg hinted at?)
> 
> Ack, will remove the blank line (didn't know that blank lines between
> variables and code is not preferred).

The blank line *is* preferred, but your patch didn't include one.

Bjorn

Re: [PATCH v3 1/2] driver core: Move the "removable" attribute from USB to core

[Greg Kroah-Hartman]

On Wed, May 12, 2021 at 02:34:56PM -0700, Rajat Jain wrote:
> Move the "removable" attribute from USB to core in order to allow it to be
> supported by other subsystem / buses. Individual buses that want to support
> this attribute can opt-in by setting the supports_removable flag, and then
> populating the removable property of the device while enumerating it. The
> UAPI (location, symantics etc) for the attribute remains unchanged.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
> v3: - Minor commit log / comments updated.
>     - use sysfs_emit()
>     - Rename local variable name (state -> loc)
>     - change supports_removable flag from bool to bitfield.
> v2: Add documentation
> 
>  Documentation/ABI/testing/sysfs-bus-usb       | 11 -------
>  .../ABI/testing/sysfs-devices-removable       | 17 ++++++++++
>  drivers/base/core.c                           | 28 ++++++++++++++++
>  drivers/usb/core/hub.c                        |  8 ++---
>  drivers/usb/core/sysfs.c                      | 24 --------------
>  drivers/usb/core/usb.c                        |  1 +
>  include/linux/device.h                        | 32 +++++++++++++++++++
>  include/linux/usb.h                           |  7 ----
>  8 files changed, 82 insertions(+), 46 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-devices-removable
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
> index bf2c1968525f..73eb23bc1f34 100644
> --- a/Documentation/ABI/testing/sysfs-bus-usb
> +++ b/Documentation/ABI/testing/sysfs-bus-usb
> @@ -154,17 +154,6 @@ Description:
>  		files hold a string value (enable or disable) indicating whether
>  		or not USB3 hardware LPM U1 or U2 is enabled for the device.
>  
> -What:		/sys/bus/usb/devices/.../removable
> -Date:		February 2012
> -Contact:	Matthew Garrett <mjg@redhat.com>
> -Description:
> -		Some information about whether a given USB device is
> -		physically fixed to the platform can be inferred from a
> -		combination of hub descriptor bits and platform-specific data
> -		such as ACPI. This file will read either "removable" or
> -		"fixed" if the information is available, and "unknown"
> -		otherwise.
> -
>  What:		/sys/bus/usb/devices/.../ltm_capable
>  Date:		July 2012
>  Contact:	Sarah Sharp <sarah.a.sharp@linux.intel.com>
> diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable
> new file mode 100644
> index 000000000000..9dabcad7cdcd
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-devices-removable
> @@ -0,0 +1,17 @@
> +What:		/sys/devices/.../removable

This should be "/sys/bus/devices/.../removable" perhaps?  Or not?  Is
this moving in the existing USB cases?

> +Date:		Apr 2021
> +Contact:	Matthew Garrett <mjg@redhat.com>,

This email address no longer works, so perhaps just use your own?

> +		Rajat Jain <rajatja@google.com>
> +Description:
> +		Information about whether a given device is physically fixed to
> +		the platform. This is determined by the device's subsystem in a
> +		bus / platform-specific way. This attribute is only present for
> +		buses that can support determining such information:
> +
> +		"removable": The device is external / removable from the system.
> +		"fixed":     The device is internal / fixed to the system.
> +		"unknown":   The information is unavailable.
> +
> +		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).
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 4a8bf8cda52b..9e6bf9e71a7e 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2404,6 +2404,25 @@ static ssize_t online_store(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RW(online);
>  
> +static ssize_t removable_show(struct device *dev, struct device_attribute *attr,
> +			      char *buf)
> +{
> +	const char *loc;
> +
> +	switch (dev->removable) {
> +	case DEVICE_REMOVABLE:
> +		loc = "removable";
> +		break;
> +	case DEVICE_FIXED:
> +		loc = "fixed";
> +		break;
> +	default:
> +		loc = "unknown";
> +	}
> +	return sysfs_emit(buf, "%s\n", loc);
> +}
> +static DEVICE_ATTR_RO(removable);
> +
>  int device_add_groups(struct device *dev, const struct attribute_group **groups)
>  {
>  	return sysfs_create_groups(&dev->kobj, groups);
> @@ -2581,8 +2600,16 @@ static int device_add_attrs(struct device *dev)
>  			goto err_remove_dev_online;
>  	}
>  
> +	if (type && type->supports_removable) {
> +		error = device_create_file(dev, &dev_attr_removable);
> +		if (error)
> +			goto err_remove_dev_waiting_for_supplier;
> +	}
> +
>  	return 0;
>  
> + err_remove_dev_waiting_for_supplier:
> +	device_remove_file(dev, &dev_attr_waiting_for_supplier);
>   err_remove_dev_online:
>  	device_remove_file(dev, &dev_attr_online);
>   err_remove_dev_groups:
> @@ -2602,6 +2629,7 @@ static void device_remove_attrs(struct device *dev)
>  	struct class *class = dev->class;
>  	const struct device_type *type = dev->type;
>  
> +	device_remove_file(dev, &dev_attr_removable);
>  	device_remove_file(dev, &dev_attr_waiting_for_supplier);
>  	device_remove_file(dev, &dev_attr_online);
>  	device_remove_groups(dev, dev->groups);
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index b2bc4b7c4289..7a3c28b14ca1 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2443,11 +2443,11 @@ static void set_usb_port_removable(struct usb_device *udev)
>  	 */
>  	switch (hub->ports[udev->portnum - 1]->connect_type) {
>  	case USB_PORT_CONNECT_TYPE_HOT_PLUG:
> -		udev->removable = USB_DEVICE_REMOVABLE;
> +		dev_set_removable(&udev->dev, DEVICE_REMOVABLE);
>  		return;
>  	case USB_PORT_CONNECT_TYPE_HARD_WIRED:
>  	case USB_PORT_NOT_USED:
> -		udev->removable = USB_DEVICE_FIXED;
> +		dev_set_removable(&udev->dev, DEVICE_FIXED);
>  		return;
>  	default:
>  		break;
> @@ -2472,9 +2472,9 @@ static void set_usb_port_removable(struct usb_device *udev)
>  	}
>  
>  	if (removable)
> -		udev->removable = USB_DEVICE_REMOVABLE;
> +		dev_set_removable(&udev->dev, DEVICE_REMOVABLE);
>  	else
> -		udev->removable = USB_DEVICE_FIXED;
> +		dev_set_removable(&udev->dev, DEVICE_FIXED);
>  
>  }
>  
> diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
> index 5a168ba9fc51..fa2e49d432ff 100644
> --- a/drivers/usb/core/sysfs.c
> +++ b/drivers/usb/core/sysfs.c
> @@ -301,29 +301,6 @@ static ssize_t urbnum_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(urbnum);
>  
> -static ssize_t removable_show(struct device *dev, struct device_attribute *attr,
> -			      char *buf)
> -{
> -	struct usb_device *udev;
> -	char *state;
> -
> -	udev = to_usb_device(dev);
> -
> -	switch (udev->removable) {
> -	case USB_DEVICE_REMOVABLE:
> -		state = "removable";
> -		break;
> -	case USB_DEVICE_FIXED:
> -		state = "fixed";
> -		break;
> -	default:
> -		state = "unknown";
> -	}
> -
> -	return sprintf(buf, "%s\n", state);
> -}
> -static DEVICE_ATTR_RO(removable);
> -
>  static ssize_t ltm_capable_show(struct device *dev,
>  				struct device_attribute *attr, char *buf)
>  {
> @@ -828,7 +805,6 @@ static struct attribute *dev_attrs[] = {
>  	&dev_attr_avoid_reset_quirk.attr,
>  	&dev_attr_authorized.attr,
>  	&dev_attr_remove.attr,
> -	&dev_attr_removable.attr,
>  	&dev_attr_ltm_capable.attr,
>  #ifdef CONFIG_OF
>  	&dev_attr_devspec.attr,
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 62368c4ed37a..ce18e84528cf 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -569,6 +569,7 @@ struct device_type usb_device_type = {
>  #ifdef CONFIG_PM
>  	.pm =		&usb_device_pm_ops,
>  #endif
> +	.supports_removable = true,
>  };
>  
>  
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 38a2071cf776..7e87ab048307 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -93,6 +93,8 @@ struct device_type {
>  	void (*release)(struct device *dev);
>  
>  	const struct dev_pm_ops *pm;
> +
> +	bool supports_removable:1; /* subsystem can classify removable/fixed */

Why isn't this a bus type?  Shouldn't it go there and not in the device
type?

>  };
>  
>  /* interface for exporting device attributes */
> @@ -350,6 +352,19 @@ enum dl_dev_state {
>  	DL_DEV_UNBINDING,
>  };
>  
> +/**
> + * enum device_removable - Whether the device is removable. The criteria for a
> + * device to be classified as removable is determined by its subsystem or bus.
> + * @DEVICE_REMOVABLE_UNKNOWN:  Device location is Unknown (default).
> + * @DEVICE_REMOVABLE: Device is removable by the user.
> + * @DEVICE_FIXED: Device is not removable by the user.
> + */
> +enum device_removable {
> +	DEVICE_REMOVABLE_UNKNOWN = 0,
> +	DEVICE_REMOVABLE,
> +	DEVICE_FIXED,
> +};
> +
>  /**
>   * struct dev_links_info - Device data related to device links.
>   * @suppliers: List of links to supplier devices.
> @@ -431,6 +446,9 @@ struct dev_links_info {
>   * 		device (i.e. the bus driver that discovered the device).
>   * @iommu_group: IOMMU group the device belongs to.
>   * @iommu:	Per device generic IOMMU runtime data
> + * @removable:  Whether the device can be removed from the system. This
> + *              should be set by the subsystem / bus driver that discovered
> + *              the device.
>   *
>   * @offline_disabled: If set, the device is permanently online.
>   * @offline:	Set after successful invocation of bus type's .offline().
> @@ -544,6 +562,8 @@ struct device {
>  	struct iommu_group	*iommu_group;
>  	struct dev_iommu	*iommu;
>  
> +	enum device_removable	removable;
> +
>  	bool			offline_disabled:1;
>  	bool			offline:1;
>  	bool			of_node_reused:1;
> @@ -782,6 +802,18 @@ static inline bool dev_has_sync_state(struct device *dev)
>  	return false;
>  }
>  
> +static inline void dev_set_removable(struct device *dev,
> +				     enum device_removable removable)
> +{
> +	dev->removable = removable;
> +}
> +
> +static inline bool dev_is_removable(struct device *dev)
> +{
> +	return dev && dev->type && dev->type->supports_removable
> +	    && dev->removable == DEVICE_REMOVABLE;

Again, shouldn't this be a bus type, and not a device type?

Where are you going to have devices of different types on a bus that do,
or do not, allow this attribute?

thanks,

greg k-h

Re: [PATCH v3 1/2] driver core: Move the "removable" attribute from USB to core

[Rajat Jain]

Hello,

On Thu, May 13, 2021 at 6:55 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, May 12, 2021 at 02:34:56PM -0700, Rajat Jain wrote:
> > Move the "removable" attribute from USB to core in order to allow it to be
> > supported by other subsystem / buses. Individual buses that want to support
> > this attribute can opt-in by setting the supports_removable flag, and then
> > populating the removable property of the device while enumerating it. The
> > UAPI (location, symantics etc) for the attribute remains unchanged.
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > Acked-by: Alan Stern <stern@rowland.harvard.edu>
> > Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> > v3: - Minor commit log / comments updated.
> >     - use sysfs_emit()
> >     - Rename local variable name (state -> loc)
> >     - change supports_removable flag from bool to bitfield.
> > v2: Add documentation
> >
> >  Documentation/ABI/testing/sysfs-bus-usb       | 11 -------
> >  .../ABI/testing/sysfs-devices-removable       | 17 ++++++++++
> >  drivers/base/core.c                           | 28 ++++++++++++++++
> >  drivers/usb/core/hub.c                        |  8 ++---
> >  drivers/usb/core/sysfs.c                      | 24 --------------
> >  drivers/usb/core/usb.c                        |  1 +
> >  include/linux/device.h                        | 32 +++++++++++++++++++
> >  include/linux/usb.h                           |  7 ----
> >  8 files changed, 82 insertions(+), 46 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-devices-removable
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
> > index bf2c1968525f..73eb23bc1f34 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-usb
> > +++ b/Documentation/ABI/testing/sysfs-bus-usb
> > @@ -154,17 +154,6 @@ Description:
> >               files hold a string value (enable or disable) indicating whether
> >               or not USB3 hardware LPM U1 or U2 is enabled for the device.
> >
> > -What:                /sys/bus/usb/devices/.../removable
> > -Date:                February 2012
> > -Contact:     Matthew Garrett <mjg@redhat.com>
> > -Description:
> > -             Some information about whether a given USB device is
> > -             physically fixed to the platform can be inferred from a
> > -             combination of hub descriptor bits and platform-specific data
> > -             such as ACPI. This file will read either "removable" or
> > -             "fixed" if the information is available, and "unknown"
> > -             otherwise.
> > -
> >  What:                /sys/bus/usb/devices/.../ltm_capable
> >  Date:                July 2012
> >  Contact:     Sarah Sharp <sarah.a.sharp@linux.intel.com>
> > diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable
> > new file mode 100644
> > index 000000000000..9dabcad7cdcd
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-devices-removable
> > @@ -0,0 +1,17 @@
> > +What:                /sys/devices/.../removable
>
> This should be "/sys/bus/devices/.../removable" perhaps?  Or not?  Is
> this moving in the existing USB cases?
>
> > +Date:                Apr 2021
> > +Contact:     Matthew Garrett <mjg@redhat.com>,
>
> This email address no longer works, so perhaps just use your own?

Ack, will do.

>
> > +             Rajat Jain <rajatja@google.com>
> > +Description:
> > +             Information about whether a given device is physically fixed to
> > +             the platform. This is determined by the device's subsystem in a
> > +             bus / platform-specific way. This attribute is only present for
> > +             buses that can support determining such information:
> > +
> > +             "removable": The device is external / removable from the system.
> > +             "fixed":     The device is internal / fixed to the system.
> > +             "unknown":   The information is unavailable.
> > +
> > +             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).
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 4a8bf8cda52b..9e6bf9e71a7e 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -2404,6 +2404,25 @@ static ssize_t online_store(struct device *dev, struct device_attribute *attr,
> >  }
> >  static DEVICE_ATTR_RW(online);
> >
> > +static ssize_t removable_show(struct device *dev, struct device_attribute *attr,
> > +                           char *buf)
> > +{
> > +     const char *loc;
> > +
> > +     switch (dev->removable) {
> > +     case DEVICE_REMOVABLE:
> > +             loc = "removable";
> > +             break;
> > +     case DEVICE_FIXED:
> > +             loc = "fixed";
> > +             break;
> > +     default:
> > +             loc = "unknown";
> > +     }
> > +     return sysfs_emit(buf, "%s\n", loc);
> > +}
> > +static DEVICE_ATTR_RO(removable);
> > +
> >  int device_add_groups(struct device *dev, const struct attribute_group **groups)
> >  {
> >       return sysfs_create_groups(&dev->kobj, groups);
> > @@ -2581,8 +2600,16 @@ static int device_add_attrs(struct device *dev)
> >                       goto err_remove_dev_online;
> >       }
> >
> > +     if (type && type->supports_removable) {
> > +             error = device_create_file(dev, &dev_attr_removable);
> > +             if (error)
> > +                     goto err_remove_dev_waiting_for_supplier;
> > +     }
> > +
> >       return 0;
> >
> > + err_remove_dev_waiting_for_supplier:
> > +     device_remove_file(dev, &dev_attr_waiting_for_supplier);
> >   err_remove_dev_online:
> >       device_remove_file(dev, &dev_attr_online);
> >   err_remove_dev_groups:
> > @@ -2602,6 +2629,7 @@ static void device_remove_attrs(struct device *dev)
> >       struct class *class = dev->class;
> >       const struct device_type *type = dev->type;
> >
> > +     device_remove_file(dev, &dev_attr_removable);
> >       device_remove_file(dev, &dev_attr_waiting_for_supplier);
> >       device_remove_file(dev, &dev_attr_online);
> >       device_remove_groups(dev, dev->groups);
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index b2bc4b7c4289..7a3c28b14ca1 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -2443,11 +2443,11 @@ static void set_usb_port_removable(struct usb_device *udev)
> >        */
> >       switch (hub->ports[udev->portnum - 1]->connect_type) {
> >       case USB_PORT_CONNECT_TYPE_HOT_PLUG:
> > -             udev->removable = USB_DEVICE_REMOVABLE;
> > +             dev_set_removable(&udev->dev, DEVICE_REMOVABLE);
> >               return;
> >       case USB_PORT_CONNECT_TYPE_HARD_WIRED:
> >       case USB_PORT_NOT_USED:
> > -             udev->removable = USB_DEVICE_FIXED;
> > +             dev_set_removable(&udev->dev, DEVICE_FIXED);
> >               return;
> >       default:
> >               break;
> > @@ -2472,9 +2472,9 @@ static void set_usb_port_removable(struct usb_device *udev)
> >       }
> >
> >       if (removable)
> > -             udev->removable = USB_DEVICE_REMOVABLE;
> > +             dev_set_removable(&udev->dev, DEVICE_REMOVABLE);
> >       else
> > -             udev->removable = USB_DEVICE_FIXED;
> > +             dev_set_removable(&udev->dev, DEVICE_FIXED);
> >
> >  }
> >
> > diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
> > index 5a168ba9fc51..fa2e49d432ff 100644
> > --- a/drivers/usb/core/sysfs.c
> > +++ b/drivers/usb/core/sysfs.c
> > @@ -301,29 +301,6 @@ static ssize_t urbnum_show(struct device *dev, struct device_attribute *attr,
> >  }
> >  static DEVICE_ATTR_RO(urbnum);
> >
> > -static ssize_t removable_show(struct device *dev, struct device_attribute *attr,
> > -                           char *buf)
> > -{
> > -     struct usb_device *udev;
> > -     char *state;
> > -
> > -     udev = to_usb_device(dev);
> > -
> > -     switch (udev->removable) {
> > -     case USB_DEVICE_REMOVABLE:
> > -             state = "removable";
> > -             break;
> > -     case USB_DEVICE_FIXED:
> > -             state = "fixed";
> > -             break;
> > -     default:
> > -             state = "unknown";
> > -     }
> > -
> > -     return sprintf(buf, "%s\n", state);
> > -}
> > -static DEVICE_ATTR_RO(removable);
> > -
> >  static ssize_t ltm_capable_show(struct device *dev,
> >                               struct device_attribute *attr, char *buf)
> >  {
> > @@ -828,7 +805,6 @@ static struct attribute *dev_attrs[] = {
> >       &dev_attr_avoid_reset_quirk.attr,
> >       &dev_attr_authorized.attr,
> >       &dev_attr_remove.attr,
> > -     &dev_attr_removable.attr,
> >       &dev_attr_ltm_capable.attr,
> >  #ifdef CONFIG_OF
> >       &dev_attr_devspec.attr,
> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index 62368c4ed37a..ce18e84528cf 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -569,6 +569,7 @@ struct device_type usb_device_type = {
> >  #ifdef CONFIG_PM
> >       .pm =           &usb_device_pm_ops,
> >  #endif
> > +     .supports_removable = true,
> >  };
> >
> >
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 38a2071cf776..7e87ab048307 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -93,6 +93,8 @@ struct device_type {
> >       void (*release)(struct device *dev);
> >
> >       const struct dev_pm_ops *pm;
> > +
> > +     bool supports_removable:1; /* subsystem can classify removable/fixed */
>
> Why isn't this a bus type?  Shouldn't it go there and not in the device
> type?

Please see below.

>
> >  };
> >
> >  /* interface for exporting device attributes */
> > @@ -350,6 +352,19 @@ enum dl_dev_state {
> >       DL_DEV_UNBINDING,
> >  };
> >
> > +/**
> > + * enum device_removable - Whether the device is removable. The criteria for a
> > + * device to be classified as removable is determined by its subsystem or bus.
> > + * @DEVICE_REMOVABLE_UNKNOWN:  Device location is Unknown (default).
> > + * @DEVICE_REMOVABLE: Device is removable by the user.
> > + * @DEVICE_FIXED: Device is not removable by the user.
> > + */
> > +enum device_removable {
> > +     DEVICE_REMOVABLE_UNKNOWN = 0,
> > +     DEVICE_REMOVABLE,
> > +     DEVICE_FIXED,
> > +};
> > +
> >  /**
> >   * struct dev_links_info - Device data related to device links.
> >   * @suppliers: List of links to supplier devices.
> > @@ -431,6 +446,9 @@ struct dev_links_info {
> >   *           device (i.e. the bus driver that discovered the device).
> >   * @iommu_group: IOMMU group the device belongs to.
> >   * @iommu:   Per device generic IOMMU runtime data
> > + * @removable:  Whether the device can be removed from the system. This
> > + *              should be set by the subsystem / bus driver that discovered
> > + *              the device.
> >   *
> >   * @offline_disabled: If set, the device is permanently online.
> >   * @offline: Set after successful invocation of bus type's .offline().
> > @@ -544,6 +562,8 @@ struct device {
> >       struct iommu_group      *iommu_group;
> >       struct dev_iommu        *iommu;
> >
> > +     enum device_removable   removable;
> > +
> >       bool                    offline_disabled:1;
> >       bool                    offline:1;
> >       bool                    of_node_reused:1;
> > @@ -782,6 +802,18 @@ static inline bool dev_has_sync_state(struct device *dev)
> >       return false;
> >  }
> >
> > +static inline void dev_set_removable(struct device *dev,
> > +                                  enum device_removable removable)
> > +{
> > +     dev->removable = removable;
> > +}
> > +
> > +static inline bool dev_is_removable(struct device *dev)
> > +{
> > +     return dev && dev->type && dev->type->supports_removable
> > +         && dev->removable == DEVICE_REMOVABLE;
>
> Again, shouldn't this be a bus type, and not a device type?
>
> Where are you going to have devices of different types on a bus that do,
> or do not, allow this attribute?

USB. Presently, both the usb_device_type and usb_if_device_type sit on
the usb_bus_type but "removable" only applies to usb_device_type (the
attribute shows up only under usb_devices and not under
usb_interfaces).

Thus, I put the supports_removable flag in device_type instead of bus_type.

Thanks,

Rajat

>
> thanks,
>
> greg k-h

Re: [PATCH v3 1/2] driver core: Move the "removable" attribute from USB to core

[Greg Kroah-Hartman]

On Thu, May 13, 2021 at 09:26:26AM -0700, Rajat Jain wrote:
> > >  static ssize_t ltm_capable_show(struct device *dev,
> > >                               struct device_attribute *attr, char *buf)
> > >  {
> > > @@ -828,7 +805,6 @@ static struct attribute *dev_attrs[] = {
> > >       &dev_attr_avoid_reset_quirk.attr,
> > >       &dev_attr_authorized.attr,
> > >       &dev_attr_remove.attr,
> > > -     &dev_attr_removable.attr,
> > >       &dev_attr_ltm_capable.attr,
> > >  #ifdef CONFIG_OF
> > >       &dev_attr_devspec.attr,
> > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > > index 62368c4ed37a..ce18e84528cf 100644
> > > --- a/drivers/usb/core/usb.c
> > > +++ b/drivers/usb/core/usb.c
> > > @@ -569,6 +569,7 @@ struct device_type usb_device_type = {
> > >  #ifdef CONFIG_PM
> > >       .pm =           &usb_device_pm_ops,
> > >  #endif
> > > +     .supports_removable = true,
> > >  };
> > >
> > >
> > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > index 38a2071cf776..7e87ab048307 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -93,6 +93,8 @@ struct device_type {
> > >       void (*release)(struct device *dev);
> > >
> > >       const struct dev_pm_ops *pm;
> > > +
> > > +     bool supports_removable:1; /* subsystem can classify removable/fixed */
> >
> > Why isn't this a bus type?  Shouldn't it go there and not in the device
> > type?
> 
> Please see below.
> 
> >
> > >  };
> > >
> > >  /* interface for exporting device attributes */
> > > @@ -350,6 +352,19 @@ enum dl_dev_state {
> > >       DL_DEV_UNBINDING,
> > >  };
> > >
> > > +/**
> > > + * enum device_removable - Whether the device is removable. The criteria for a
> > > + * device to be classified as removable is determined by its subsystem or bus.
> > > + * @DEVICE_REMOVABLE_UNKNOWN:  Device location is Unknown (default).
> > > + * @DEVICE_REMOVABLE: Device is removable by the user.
> > > + * @DEVICE_FIXED: Device is not removable by the user.
> > > + */
> > > +enum device_removable {
> > > +     DEVICE_REMOVABLE_UNKNOWN = 0,
> > > +     DEVICE_REMOVABLE,
> > > +     DEVICE_FIXED,
> > > +};
> > > +
> > >  /**
> > >   * struct dev_links_info - Device data related to device links.
> > >   * @suppliers: List of links to supplier devices.
> > > @@ -431,6 +446,9 @@ struct dev_links_info {
> > >   *           device (i.e. the bus driver that discovered the device).
> > >   * @iommu_group: IOMMU group the device belongs to.
> > >   * @iommu:   Per device generic IOMMU runtime data
> > > + * @removable:  Whether the device can be removed from the system. This
> > > + *              should be set by the subsystem / bus driver that discovered
> > > + *              the device.
> > >   *
> > >   * @offline_disabled: If set, the device is permanently online.
> > >   * @offline: Set after successful invocation of bus type's .offline().
> > > @@ -544,6 +562,8 @@ struct device {
> > >       struct iommu_group      *iommu_group;
> > >       struct dev_iommu        *iommu;
> > >
> > > +     enum device_removable   removable;
> > > +
> > >       bool                    offline_disabled:1;
> > >       bool                    offline:1;
> > >       bool                    of_node_reused:1;
> > > @@ -782,6 +802,18 @@ static inline bool dev_has_sync_state(struct device *dev)
> > >       return false;
> > >  }
> > >
> > > +static inline void dev_set_removable(struct device *dev,
> > > +                                  enum device_removable removable)
> > > +{
> > > +     dev->removable = removable;
> > > +}
> > > +
> > > +static inline bool dev_is_removable(struct device *dev)
> > > +{
> > > +     return dev && dev->type && dev->type->supports_removable
> > > +         && dev->removable == DEVICE_REMOVABLE;
> >
> > Again, shouldn't this be a bus type, and not a device type?
> >
> > Where are you going to have devices of different types on a bus that do,
> > or do not, allow this attribute?
> 
> USB. Presently, both the usb_device_type and usb_if_device_type sit on
> the usb_bus_type but "removable" only applies to usb_device_type (the
> attribute shows up only under usb_devices and not under
> usb_interfaces).

Ah.  Messy.

Then should this be a per-device value and don't worry about the type at
all?  You have a field in the device already, why not make it:

enum device_removable {
	DEVICE_REMOVABLE_NOT_SUPPORTED = 0,	/* must be 0 */
	DEVICE_REMOVABLE_UNKNOWN,
	DEVICE_REMOVABLE,
	DEVICE_FIXED,
};

And then only show the file if a device does NOT have the value set to
DEVICE_REMOVABLE_NOT_SUPPORTED?

If a bus/subsystem/whatever wants the device to have a sysfs file and
value there, then it needs to set it to anything other than 0 and it
will be created?

That gets you out of the device bus/type mess and it is localized to
only the device itself, allowing for any device to do this?  Should make
the logic everywhere a bit simpler as well.

thanks,

greg k-h

Re: [PATCH v3 1/2] driver core: Move the "removable" attribute from USB to core

[Rajat Jain]

On Thu, May 13, 2021 at 9:40 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, May 13, 2021 at 09:26:26AM -0700, Rajat Jain wrote:
> > > >  static ssize_t ltm_capable_show(struct device *dev,
> > > >                               struct device_attribute *attr, char *buf)
> > > >  {
> > > > @@ -828,7 +805,6 @@ static struct attribute *dev_attrs[] = {
> > > >       &dev_attr_avoid_reset_quirk.attr,
> > > >       &dev_attr_authorized.attr,
> > > >       &dev_attr_remove.attr,
> > > > -     &dev_attr_removable.attr,
> > > >       &dev_attr_ltm_capable.attr,
> > > >  #ifdef CONFIG_OF
> > > >       &dev_attr_devspec.attr,
> > > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > > > index 62368c4ed37a..ce18e84528cf 100644
> > > > --- a/drivers/usb/core/usb.c
> > > > +++ b/drivers/usb/core/usb.c
> > > > @@ -569,6 +569,7 @@ struct device_type usb_device_type = {
> > > >  #ifdef CONFIG_PM
> > > >       .pm =           &usb_device_pm_ops,
> > > >  #endif
> > > > +     .supports_removable = true,
> > > >  };
> > > >
> > > >
> > > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > > index 38a2071cf776..7e87ab048307 100644
> > > > --- a/include/linux/device.h
> > > > +++ b/include/linux/device.h
> > > > @@ -93,6 +93,8 @@ struct device_type {
> > > >       void (*release)(struct device *dev);
> > > >
> > > >       const struct dev_pm_ops *pm;
> > > > +
> > > > +     bool supports_removable:1; /* subsystem can classify removable/fixed */
> > >
> > > Why isn't this a bus type?  Shouldn't it go there and not in the device
> > > type?
> >
> > Please see below.
> >
> > >
> > > >  };
> > > >
> > > >  /* interface for exporting device attributes */
> > > > @@ -350,6 +352,19 @@ enum dl_dev_state {
> > > >       DL_DEV_UNBINDING,
> > > >  };
> > > >
> > > > +/**
> > > > + * enum device_removable - Whether the device is removable. The criteria for a
> > > > + * device to be classified as removable is determined by its subsystem or bus.
> > > > + * @DEVICE_REMOVABLE_UNKNOWN:  Device location is Unknown (default).
> > > > + * @DEVICE_REMOVABLE: Device is removable by the user.
> > > > + * @DEVICE_FIXED: Device is not removable by the user.
> > > > + */
> > > > +enum device_removable {
> > > > +     DEVICE_REMOVABLE_UNKNOWN = 0,
> > > > +     DEVICE_REMOVABLE,
> > > > +     DEVICE_FIXED,
> > > > +};
> > > > +
> > > >  /**
> > > >   * struct dev_links_info - Device data related to device links.
> > > >   * @suppliers: List of links to supplier devices.
> > > > @@ -431,6 +446,9 @@ struct dev_links_info {
> > > >   *           device (i.e. the bus driver that discovered the device).
> > > >   * @iommu_group: IOMMU group the device belongs to.
> > > >   * @iommu:   Per device generic IOMMU runtime data
> > > > + * @removable:  Whether the device can be removed from the system. This
> > > > + *              should be set by the subsystem / bus driver that discovered
> > > > + *              the device.
> > > >   *
> > > >   * @offline_disabled: If set, the device is permanently online.
> > > >   * @offline: Set after successful invocation of bus type's .offline().
> > > > @@ -544,6 +562,8 @@ struct device {
> > > >       struct iommu_group      *iommu_group;
> > > >       struct dev_iommu        *iommu;
> > > >
> > > > +     enum device_removable   removable;
> > > > +
> > > >       bool                    offline_disabled:1;
> > > >       bool                    offline:1;
> > > >       bool                    of_node_reused:1;
> > > > @@ -782,6 +802,18 @@ static inline bool dev_has_sync_state(struct device *dev)
> > > >       return false;
> > > >  }
> > > >
> > > > +static inline void dev_set_removable(struct device *dev,
> > > > +                                  enum device_removable removable)
> > > > +{
> > > > +     dev->removable = removable;
> > > > +}
> > > > +
> > > > +static inline bool dev_is_removable(struct device *dev)
> > > > +{
> > > > +     return dev && dev->type && dev->type->supports_removable
> > > > +         && dev->removable == DEVICE_REMOVABLE;
> > >
> > > Again, shouldn't this be a bus type, and not a device type?
> > >
> > > Where are you going to have devices of different types on a bus that do,
> > > or do not, allow this attribute?
> >
> > USB. Presently, both the usb_device_type and usb_if_device_type sit on
> > the usb_bus_type but "removable" only applies to usb_device_type (the
> > attribute shows up only under usb_devices and not under
> > usb_interfaces).
>
> Ah.  Messy.
>
> Then should this be a per-device value and don't worry about the type at
> all?  You have a field in the device already, why not make it:
>
> enum device_removable {
>         DEVICE_REMOVABLE_NOT_SUPPORTED = 0,     /* must be 0 */
>         DEVICE_REMOVABLE_UNKNOWN,
>         DEVICE_REMOVABLE,
>         DEVICE_FIXED,
> };
>
> And then only show the file if a device does NOT have the value set to
> DEVICE_REMOVABLE_NOT_SUPPORTED?
>
> If a bus/subsystem/whatever wants the device to have a sysfs file and
> value there, then it needs to set it to anything other than 0 and it
> will be created?

Ack, will do.

Thanks,

Rajat


>
> That gets you out of the device bus/type mess and it is localized to
> only the device itself, allowing for any device to do this?  Should make
> the logic everywhere a bit simpler as well.
>
> thanks,
>
> greg k-h

Re: [PATCH v3 1/2] driver core: Move the "removable" attribute from USB to core

[Rajat Jain]

Hello,

On Thu, May 13, 2021 at 6:55 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, May 12, 2021 at 02:34:56PM -0700, Rajat Jain wrote:
> > Move the "removable" attribute from USB to core in order to allow it to be
> > supported by other subsystem / buses. Individual buses that want to support
> > this attribute can opt-in by setting the supports_removable flag, and then
> > populating the removable property of the device while enumerating it. The
> > UAPI (location, symantics etc) for the attribute remains unchanged.
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > Acked-by: Alan Stern <stern@rowland.harvard.edu>
> > Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> > v3: - Minor commit log / comments updated.
> >     - use sysfs_emit()
> >     - Rename local variable name (state -> loc)
> >     - change supports_removable flag from bool to bitfield.
> > v2: Add documentation
> >
> >  Documentation/ABI/testing/sysfs-bus-usb       | 11 -------
> >  .../ABI/testing/sysfs-devices-removable       | 17 ++++++++++
> >  drivers/base/core.c                           | 28 ++++++++++++++++
> >  drivers/usb/core/hub.c                        |  8 ++---
> >  drivers/usb/core/sysfs.c                      | 24 --------------
> >  drivers/usb/core/usb.c                        |  1 +
> >  include/linux/device.h                        | 32 +++++++++++++++++++
> >  include/linux/usb.h                           |  7 ----
> >  8 files changed, 82 insertions(+), 46 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-devices-removable
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
> > index bf2c1968525f..73eb23bc1f34 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-usb
> > +++ b/Documentation/ABI/testing/sysfs-bus-usb
> > @@ -154,17 +154,6 @@ Description:
> >               files hold a string value (enable or disable) indicating whether
> >               or not USB3 hardware LPM U1 or U2 is enabled for the device.
> >
> > -What:                /sys/bus/usb/devices/.../removable
> > -Date:                February 2012
> > -Contact:     Matthew Garrett <mjg@redhat.com>
> > -Description:
> > -             Some information about whether a given USB device is
> > -             physically fixed to the platform can be inferred from a
> > -             combination of hub descriptor bits and platform-specific data
> > -             such as ACPI. This file will read either "removable" or
> > -             "fixed" if the information is available, and "unknown"
> > -             otherwise.
> > -
> >  What:                /sys/bus/usb/devices/.../ltm_capable
> >  Date:                July 2012
> >  Contact:     Sarah Sharp <sarah.a.sharp@linux.intel.com>
> > diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable
> > new file mode 100644
> > index 000000000000..9dabcad7cdcd
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-devices-removable
> > @@ -0,0 +1,17 @@
> > +What:                /sys/devices/.../removable
>
> This should be "/sys/bus/devices/.../removable" perhaps?  Or not?  Is
> this moving in the existing USB cases?

Sorry, I forgot to respond to this comment earlier today.

There is no /sys/bus/devices/ so I guess you had really meant
"/sys/bus/.../devices/.../removable". Given the latest suggestion on
the patch, where each device shall be free to show / hide this
attribute, I think the current  /sys/devices/.../removable looks
correct. Please let me know if you'd still like to change this to
something else, and I'd be happy to change.

Thanks,

Rajat


>
> > +Date:                Apr 2021
> > +Contact:     Matthew Garrett <mjg@redhat.com>,
>
> This email address no longer works, so perhaps just use your own?
>
> > +             Rajat Jain <rajatja@google.com>
> > +Description:
> > +             Information about whether a given device is physically fixed to
> > +             the platform. This is determined by the device's subsystem in a
> > +             bus / platform-specific way. This attribute is only present for
> > +             buses that can support determining such information:
> > +
> > +             "removable": The device is external / removable from the system.
> > +             "fixed":     The device is internal / fixed to the system.
> > +             "unknown":   The information is unavailable.
> > +
> > +             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).
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 4a8bf8cda52b..9e6bf9e71a7e 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -2404,6 +2404,25 @@ static ssize_t online_store(struct device *dev, struct device_attribute *attr,
> >  }
> >  static DEVICE_ATTR_RW(online);
> >
> > +static ssize_t removable_show(struct device *dev, struct device_attribute *attr,
> > +                           char *buf)
> > +{
> > +     const char *loc;
> > +
> > +     switch (dev->removable) {
> > +     case DEVICE_REMOVABLE:
> > +             loc = "removable";
> > +             break;
> > +     case DEVICE_FIXED:
> > +             loc = "fixed";
> > +             break;
> > +     default:
> > +             loc = "unknown";
> > +     }
> > +     return sysfs_emit(buf, "%s\n", loc);
> > +}
> > +static DEVICE_ATTR_RO(removable);
> > +
> >  int device_add_groups(struct device *dev, const struct attribute_group **groups)
> >  {
> >       return sysfs_create_groups(&dev->kobj, groups);
> > @@ -2581,8 +2600,16 @@ static int device_add_attrs(struct device *dev)
> >                       goto err_remove_dev_online;
> >       }
> >
> > +     if (type && type->supports_removable) {
> > +             error = device_create_file(dev, &dev_attr_removable);
> > +             if (error)
> > +                     goto err_remove_dev_waiting_for_supplier;
> > +     }
> > +
> >       return 0;
> >
> > + err_remove_dev_waiting_for_supplier:
> > +     device_remove_file(dev, &dev_attr_waiting_for_supplier);
> >   err_remove_dev_online:
> >       device_remove_file(dev, &dev_attr_online);
> >   err_remove_dev_groups:
> > @@ -2602,6 +2629,7 @@ static void device_remove_attrs(struct device *dev)
> >       struct class *class = dev->class;
> >       const struct device_type *type = dev->type;
> >
> > +     device_remove_file(dev, &dev_attr_removable);
> >       device_remove_file(dev, &dev_attr_waiting_for_supplier);
> >       device_remove_file(dev, &dev_attr_online);
> >       device_remove_groups(dev, dev->groups);
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index b2bc4b7c4289..7a3c28b14ca1 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -2443,11 +2443,11 @@ static void set_usb_port_removable(struct usb_device *udev)
> >        */
> >       switch (hub->ports[udev->portnum - 1]->connect_type) {
> >       case USB_PORT_CONNECT_TYPE_HOT_PLUG:
> > -             udev->removable = USB_DEVICE_REMOVABLE;
> > +             dev_set_removable(&udev->dev, DEVICE_REMOVABLE);
> >               return;
> >       case USB_PORT_CONNECT_TYPE_HARD_WIRED:
> >       case USB_PORT_NOT_USED:
> > -             udev->removable = USB_DEVICE_FIXED;
> > +             dev_set_removable(&udev->dev, DEVICE_FIXED);
> >               return;
> >       default:
> >               break;
> > @@ -2472,9 +2472,9 @@ static void set_usb_port_removable(struct usb_device *udev)
> >       }
> >
> >       if (removable)
> > -             udev->removable = USB_DEVICE_REMOVABLE;
> > +             dev_set_removable(&udev->dev, DEVICE_REMOVABLE);
> >       else
> > -             udev->removable = USB_DEVICE_FIXED;
> > +             dev_set_removable(&udev->dev, DEVICE_FIXED);
> >
> >  }
> >
> > diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
> > index 5a168ba9fc51..fa2e49d432ff 100644
> > --- a/drivers/usb/core/sysfs.c
> > +++ b/drivers/usb/core/sysfs.c
> > @@ -301,29 +301,6 @@ static ssize_t urbnum_show(struct device *dev, struct device_attribute *attr,
> >  }
> >  static DEVICE_ATTR_RO(urbnum);
> >
> > -static ssize_t removable_show(struct device *dev, struct device_attribute *attr,
> > -                           char *buf)
> > -{
> > -     struct usb_device *udev;
> > -     char *state;
> > -
> > -     udev = to_usb_device(dev);
> > -
> > -     switch (udev->removable) {
> > -     case USB_DEVICE_REMOVABLE:
> > -             state = "removable";
> > -             break;
> > -     case USB_DEVICE_FIXED:
> > -             state = "fixed";
> > -             break;
> > -     default:
> > -             state = "unknown";
> > -     }
> > -
> > -     return sprintf(buf, "%s\n", state);
> > -}
> > -static DEVICE_ATTR_RO(removable);
> > -
> >  static ssize_t ltm_capable_show(struct device *dev,
> >                               struct device_attribute *attr, char *buf)
> >  {
> > @@ -828,7 +805,6 @@ static struct attribute *dev_attrs[] = {
> >       &dev_attr_avoid_reset_quirk.attr,
> >       &dev_attr_authorized.attr,
> >       &dev_attr_remove.attr,
> > -     &dev_attr_removable.attr,
> >       &dev_attr_ltm_capable.attr,
> >  #ifdef CONFIG_OF
> >       &dev_attr_devspec.attr,
> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index 62368c4ed37a..ce18e84528cf 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -569,6 +569,7 @@ struct device_type usb_device_type = {
> >  #ifdef CONFIG_PM
> >       .pm =           &usb_device_pm_ops,
> >  #endif
> > +     .supports_removable = true,
> >  };
> >
> >
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 38a2071cf776..7e87ab048307 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -93,6 +93,8 @@ struct device_type {
> >       void (*release)(struct device *dev);
> >
> >       const struct dev_pm_ops *pm;
> > +
> > +     bool supports_removable:1; /* subsystem can classify removable/fixed */
>
> Why isn't this a bus type?  Shouldn't it go there and not in the device
> type?
>
> >  };
> >
> >  /* interface for exporting device attributes */
> > @@ -350,6 +352,19 @@ enum dl_dev_state {
> >       DL_DEV_UNBINDING,
> >  };
> >
> > +/**
> > + * enum device_removable - Whether the device is removable. The criteria for a
> > + * device to be classified as removable is determined by its subsystem or bus.
> > + * @DEVICE_REMOVABLE_UNKNOWN:  Device location is Unknown (default).
> > + * @DEVICE_REMOVABLE: Device is removable by the user.
> > + * @DEVICE_FIXED: Device is not removable by the user.
> > + */
> > +enum device_removable {
> > +     DEVICE_REMOVABLE_UNKNOWN = 0,
> > +     DEVICE_REMOVABLE,
> > +     DEVICE_FIXED,
> > +};
> > +
> >  /**
> >   * struct dev_links_info - Device data related to device links.
> >   * @suppliers: List of links to supplier devices.
> > @@ -431,6 +446,9 @@ struct dev_links_info {
> >   *           device (i.e. the bus driver that discovered the device).
> >   * @iommu_group: IOMMU group the device belongs to.
> >   * @iommu:   Per device generic IOMMU runtime data
> > + * @removable:  Whether the device can be removed from the system. This
> > + *              should be set by the subsystem / bus driver that discovered
> > + *              the device.
> >   *
> >   * @offline_disabled: If set, the device is permanently online.
> >   * @offline: Set after successful invocation of bus type's .offline().
> > @@ -544,6 +562,8 @@ struct device {
> >       struct iommu_group      *iommu_group;
> >       struct dev_iommu        *iommu;
> >
> > +     enum device_removable   removable;
> > +
> >       bool                    offline_disabled:1;
> >       bool                    offline:1;
> >       bool                    of_node_reused:1;
> > @@ -782,6 +802,18 @@ static inline bool dev_has_sync_state(struct device *dev)
> >       return false;
> >  }
> >
> > +static inline void dev_set_removable(struct device *dev,
> > +                                  enum device_removable removable)
> > +{
> > +     dev->removable = removable;
> > +}
> > +
> > +static inline bool dev_is_removable(struct device *dev)
> > +{
> > +     return dev && dev->type && dev->type->supports_removable
> > +         && dev->removable == DEVICE_REMOVABLE;
>
> Again, shouldn't this be a bus type, and not a device type?
>
> Where are you going to have devices of different types on a bus that do,
> or do not, allow this attribute?
>
> thanks,
>
> greg k-h