All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core
@ 2021-04-24  2:16 Rajat Jain
  2021-04-24  2:16 ` [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices Rajat Jain
                   ` (3 more replies)
  0 siblings, 4 replies; 25+ messages in thread
From: Rajat Jain @ 2021-04-24  2:16 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J. Wysocki, Bjorn Helgaas, Alan Stern,
	Rajat Jain, linux-kernel, linux-pci, linux-usb, helgaas
  Cc: rajatxjain, jsbarnes, dtor

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 ABI for the attribute remains
unchanged.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
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                        | 36 +++++++++++++++++++
 include/linux/usb.h                           |  7 ----
 8 files changed, 86 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..e13dddd547b5
--- /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 f29839382f81..b8ae4cc52805 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2327,6 +2327,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 *state;
+
+	switch (dev->removable) {
+	case DEVICE_REMOVABLE:
+		state = "removable";
+		break;
+	case DEVICE_FIXED:
+		state = "fixed";
+		break;
+	default:
+		state = "unknown";
+	}
+	return sprintf(buf, "%s\n", state);
+}
+static DEVICE_ATTR_RO(removable);
+
 int device_add_groups(struct device *dev, const struct attribute_group **groups)
 {
 	return sysfs_create_groups(&dev->kobj, groups);
@@ -2504,8 +2523,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:
@@ -2525,6 +2552,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 7f71218cc1e5..500e5648de04 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2442,11 +2442,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;
@@ -2471,9 +2471,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 d85699bee671..e8ff3afdf7af 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -298,29 +298,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)
 {
@@ -825,7 +802,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 a566bb494e24..5a0f73a28196 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -523,6 +523,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 ba660731bd25..d6442b811607 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -93,6 +93,12 @@ struct device_type {
 	void (*release)(struct device *dev);
 
 	const struct dev_pm_ops *pm;
+
+	/*
+	 * Determines whether the subsystem supports classifying the devices of
+	 * this type into removable vs fixed.
+	 */
+	bool supports_removable;
 };
 
 /* interface for exporting device attributes */
@@ -350,6 +356,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 +450,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().
@@ -541,6 +563,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;
@@ -778,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 d6a41841b93e..0bbb9e8b18c7 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,
@@ -701,7 +695,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.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-04-24  2:16 [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core Rajat Jain
@ 2021-04-24  2:16 ` Rajat Jain
  2021-04-26  9:17   ` Oliver Neukum
  2021-05-11 21:30   ` Bjorn Helgaas
  2021-04-25 14:58 ` [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core Alan Stern
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 25+ messages in thread
From: Rajat Jain @ 2021-04-24  2:16 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J. Wysocki, Bjorn Helgaas, Alan Stern,
	Rajat Jain, linux-kernel, linux-pci, linux-usb, helgaas
  Cc: rajatxjain, jsbarnes, dtor

Export the already available info, to the userspace via the
device core, so that userspace can implement whatever policies it
wants to, for external removable devices.

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

 Documentation/ABI/testing/sysfs-devices-removable |  3 ++-
 drivers/pci/pci-sysfs.c                           |  1 +
 drivers/pci/probe.c                               | 12 ++++++++++++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable
index e13dddd547b5..daac4f007619 100644
--- a/Documentation/ABI/testing/sysfs-devices-removable
+++ b/Documentation/ABI/testing/sysfs-devices-removable
@@ -14,4 +14,5 @@ Description:
 
 		Currently this is only supported by USB (which infers the
 		information from a combination of hub descriptor bits and
-		platform-specific data such as ACPI).
+		platform-specific data such as ACPI) and PCI (which gets this
+		from ACPI / device tree).
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index f8afd54ca3e1..9302f0076e73 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1582,4 +1582,5 @@ static const struct attribute_group *pci_dev_attr_groups[] = {
 
 const struct device_type pci_dev_type = {
 	.groups = pci_dev_attr_groups,
+	.supports_removable = true,
 };
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 953f15abc850..d1cceee62e1b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1575,6 +1575,16 @@ static void set_pcie_untrusted(struct pci_dev *dev)
 		dev->untrusted = true;
 }
 
+static void set_pci_dev_removable(struct pci_dev *dev)
+{
+	struct pci_dev *parent = pci_upstream_bridge(dev);
+	if (parent &&
+	    (parent->external_facing || dev_is_removable(&parent->dev)))
+		dev_set_removable(&dev->dev, DEVICE_REMOVABLE);
+	else
+		dev_set_removable(&dev->dev, DEVICE_FIXED);
+}
+
 /**
  * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config?
  * @dev: PCI device
@@ -1819,6 +1829,8 @@ int pci_setup_device(struct pci_dev *dev)
 	/* "Unknown power state" */
 	dev->current_state = PCI_UNKNOWN;
 
+	set_pci_dev_removable(dev);
+
 	/* Early fixups, before probing the BARs */
 	pci_fixup_device(pci_fixup_early, dev);
 
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core
  2021-04-24  2:16 [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core Rajat Jain
  2021-04-24  2:16 ` [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices Rajat Jain
@ 2021-04-25 14:58 ` Alan Stern
  2021-05-11 19:22 ` Bjorn Helgaas
  2021-05-12  1:00 ` Krzysztof Wilczyński
  3 siblings, 0 replies; 25+ messages in thread
From: Alan Stern @ 2021-04-25 14:58 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Bjorn Helgaas,
	linux-kernel, linux-pci, linux-usb, helgaas, rajatxjain,
	jsbarnes, dtor

On Fri, Apr 23, 2021 at 07:16:30PM -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 ABI for the attribute remains
> unchanged.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v2: Add documentation

For the parts affecting the USB core:

Acked-by: Alan Stern <stern@rowland.harvard.edu>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-04-24  2:16 ` [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices Rajat Jain
@ 2021-04-26  9:17   ` Oliver Neukum
  2021-04-26 11:49     ` Rafael J. Wysocki
  2021-05-11 21:30   ` Bjorn Helgaas
  1 sibling, 1 reply; 25+ messages in thread
From: Oliver Neukum @ 2021-04-26  9:17 UTC (permalink / raw)
  To: Rajat Jain, Greg Kroah-Hartman, Rafael J. Wysocki, Bjorn Helgaas,
	Alan Stern, linux-kernel, linux-pci, linux-usb, helgaas
  Cc: rajatxjain, jsbarnes, dtor

Am Freitag, den 23.04.2021, 19:16 -0700 schrieb Rajat Jain:
> Export the already available info, to the userspace via the
> device core, so that userspace can implement whatever policies it
> wants to, for external removable devices.

Hi,

is there a way to tell apart whether a device can undergo regular
surprise removal? Do we want that?

	Regards
		Oliver



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-04-26  9:17   ` Oliver Neukum
@ 2021-04-26 11:49     ` Rafael J. Wysocki
  2021-04-26 13:01       ` David Laight
  0 siblings, 1 reply; 25+ messages in thread
From: Rafael J. Wysocki @ 2021-04-26 11:49 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Rajat Jain, Greg Kroah-Hartman, Rafael J. Wysocki, Bjorn Helgaas,
	Alan Stern, Linux Kernel Mailing List, Linux PCI,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Bjorn Helgaas, Rajat Jain, Jesse Barnes, Dmitry Torokhov

On Mon, Apr 26, 2021 at 11:17 AM Oliver Neukum <oneukum@suse.com> wrote:
>
> Am Freitag, den 23.04.2021, 19:16 -0700 schrieb Rajat Jain:
> > Export the already available info, to the userspace via the
> > device core, so that userspace can implement whatever policies it
> > wants to, for external removable devices.
>
> Hi,
>
> is there a way to tell apart whether a device can undergo regular
> surprise removal?

PCI devices located under a removable parent can undergo surprise
removal.  The ones on a Thunderbolt chain too.

> Do we want that?

Do you mean surprise removal?  Yes, we do.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-04-26 11:49     ` Rafael J. Wysocki
@ 2021-04-26 13:01       ` David Laight
  2021-04-26 19:47         ` Rajat Jain
  2021-04-27 11:59         ` Oliver Neukum
  0 siblings, 2 replies; 25+ messages in thread
From: David Laight @ 2021-04-26 13:01 UTC (permalink / raw)
  To: 'Rafael J. Wysocki', Oliver Neukum
  Cc: Rajat Jain, Greg Kroah-Hartman, Bjorn Helgaas, Alan Stern,
	Linux Kernel Mailing List, Linux PCI,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Bjorn Helgaas, Rajat Jain, Jesse Barnes, Dmitry Torokhov

From: Rafael J. Wysocki
> Sent: 26 April 2021 12:49
> 
> On Mon, Apr 26, 2021 at 11:17 AM Oliver Neukum <oneukum@suse.com> wrote:
> >
> > Am Freitag, den 23.04.2021, 19:16 -0700 schrieb Rajat Jain:
> > > Export the already available info, to the userspace via the
> > > device core, so that userspace can implement whatever policies it
> > > wants to, for external removable devices.
> >
> > Hi,
> >
> > is there a way to tell apart whether a device can undergo regular
> > surprise removal?
> 
> PCI devices located under a removable parent can undergo surprise
> removal.  The ones on a Thunderbolt chain too.
> 
> > Do we want that?
> 
> Do you mean surprise removal?  Yes, we do.

Always been true - think of cardbus (PCI pcmcia) cards with
PCI bridges to external PCI expansion chassis containing
additional PCI slots.
The cardbus card is hot removable.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-04-26 13:01       ` David Laight
@ 2021-04-26 19:47         ` Rajat Jain
  2021-04-27 11:59         ` Oliver Neukum
  1 sibling, 0 replies; 25+ messages in thread
From: Rajat Jain @ 2021-04-26 19:47 UTC (permalink / raw)
  To: David Laight
  Cc: Rafael J. Wysocki, Oliver Neukum, Greg Kroah-Hartman,
	Bjorn Helgaas, Alan Stern, Linux Kernel Mailing List, Linux PCI,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Bjorn Helgaas, Rajat Jain, Jesse Barnes, Dmitry Torokhov

On Mon, Apr 26, 2021 at 6:01 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Rafael J. Wysocki
> > Sent: 26 April 2021 12:49
> >
> > On Mon, Apr 26, 2021 at 11:17 AM Oliver Neukum <oneukum@suse.com> wrote:
> > >
> > > Am Freitag, den 23.04.2021, 19:16 -0700 schrieb Rajat Jain:
> > > > Export the already available info, to the userspace via the
> > > > device core, so that userspace can implement whatever policies it
> > > > wants to, for external removable devices.
> > >
> > > Hi,
> > >
> > > is there a way to tell apart whether a device can undergo regular
> > > surprise removal?
> >
> > PCI devices located under a removable parent can undergo surprise
> > removal.  The ones on a Thunderbolt chain too.
> >
> > > Do we want that?
> >
> > Do you mean surprise removal?  Yes, we do.
>
> Always been true - think of cardbus (PCI pcmcia) cards with
> PCI bridges to external PCI expansion chassis containing
> additional PCI slots.
> The cardbus card is hot removable.

Hi Oliver / Folks, please let me know if there is a suggestion for me
here, or if there is still a question for me to answer.

Thanks,

Rajat

PS: To give some background about this change, we'd like to implement
some policies around disabling user plugged devices when a user logs
out, and collect statistics around use of such devices.

>
>         David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-04-26 13:01       ` David Laight
  2021-04-26 19:47         ` Rajat Jain
@ 2021-04-27 11:59         ` Oliver Neukum
  2021-04-27 12:59           ` David Laight
  1 sibling, 1 reply; 25+ messages in thread
From: Oliver Neukum @ 2021-04-27 11:59 UTC (permalink / raw)
  To: David Laight, 'Rafael J. Wysocki'
  Cc: Rajat Jain, Greg Kroah-Hartman, Bjorn Helgaas, Alan Stern,
	Linux Kernel Mailing List, Linux PCI,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Bjorn Helgaas, Rajat Jain, Jesse Barnes, Dmitry Torokhov

Am Montag, den 26.04.2021, 13:01 +0000 schrieb David Laight:
> From: Rafael J. Wysocki
> > Sent: 26 April 2021 12:49
> > 
> > On Mon, Apr 26, 2021 at 11:17 AM Oliver Neukum <oneukum@suse.com> wrote:
> > > Am Freitag, den 23.04.2021, 19:16 -0700 schrieb Rajat Jain:
> > > > Export the already available info, to the userspace via the
> > > > device core, so that userspace can implement whatever policies it
> > > > wants to, for external removable devices.
> > > 
> > > Hi,
> > > 
> > > is there a way to tell apart whether a device can undergo regular
> > > surprise removal?
> > 
> > PCI devices located under a removable parent can undergo surprise
> > removal.  The ones on a Thunderbolt chain too.
> > 
> > > Do we want that?
> > 
> > Do you mean surprise removal?  Yes, we do.
> 
> Always been true - think of cardbus (PCI pcmcia) cards with
> PCI bridges to external PCI expansion chassis containing
> additional PCI slots.
> The cardbus card is hot removable.

Hi,

that is true for those options, but not for the style
of PCI hotplug which requires you to push a button and wait
for the blinking light.

	REgards
		Oliver



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-04-27 11:59         ` Oliver Neukum
@ 2021-04-27 12:59           ` David Laight
  2021-04-28  6:56             ` Oliver Neukum
  0 siblings, 1 reply; 25+ messages in thread
From: David Laight @ 2021-04-27 12:59 UTC (permalink / raw)
  To: 'Oliver Neukum', 'Rafael J. Wysocki'
  Cc: Rajat Jain, Greg Kroah-Hartman, Bjorn Helgaas, Alan Stern,
	Linux Kernel Mailing List, Linux PCI,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Bjorn Helgaas, Rajat Jain, Jesse Barnes, Dmitry Torokhov

From: Oliver Neukum
> Sent: 27 April 2021 13:00
> 
> Am Montag, den 26.04.2021, 13:01 +0000 schrieb David Laight:
> > From: Rafael J. Wysocki
> > > Sent: 26 April 2021 12:49
> > >
> > > On Mon, Apr 26, 2021 at 11:17 AM Oliver Neukum <oneukum@suse.com> wrote:
> > > > Am Freitag, den 23.04.2021, 19:16 -0700 schrieb Rajat Jain:
> > > > > Export the already available info, to the userspace via the
> > > > > device core, so that userspace can implement whatever policies it
> > > > > wants to, for external removable devices.
> > > >
> > > > Hi,
> > > >
> > > > is there a way to tell apart whether a device can undergo regular
> > > > surprise removal?
> > >
> > > PCI devices located under a removable parent can undergo surprise
> > > removal.  The ones on a Thunderbolt chain too.
> > >
> > > > Do we want that?
> > >
> > > Do you mean surprise removal?  Yes, we do.
> >
> > Always been true - think of cardbus (PCI pcmcia) cards with
> > PCI bridges to external PCI expansion chassis containing
> > additional PCI slots.
> > The cardbus card is hot removable.
> 
> Hi,
> 
> that is true for those options, but not for the style
> of PCI hotplug which requires you to push a button and wait
> for the blinking light.

True, I remember some of those PCI hotplug chassis from 25 years ago.
ISTR we did get the removal events working (SVR4/Unixware) but I
don't remember the relevant chassis ever being sold.
In spite of the marketing hype I suspect it was only ever possible
to remove a completely working board and replace it with an
exactly equivalent one.

In any case those chassis are not 'surprise removal'.

More modern drivers are less likely to crash (and burn?) when
a PCI read returns ~0u.
But I suspect an awful lot really don't handle surprise removal
very well at all.

How many ensure that a ~0u response from every PCI(e) wont
cause some kind of grief?
(We've been there due to a buggy fpga not responding to non-config
cycles.)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-04-27 12:59           ` David Laight
@ 2021-04-28  6:56             ` Oliver Neukum
  2021-04-28 12:21               ` Rafael J. Wysocki
  0 siblings, 1 reply; 25+ messages in thread
From: Oliver Neukum @ 2021-04-28  6:56 UTC (permalink / raw)
  To: David Laight, 'Rafael J. Wysocki'
  Cc: Rajat Jain, Greg Kroah-Hartman, Bjorn Helgaas, Alan Stern,
	Linux Kernel Mailing List, Linux PCI,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Bjorn Helgaas, Rajat Jain, Jesse Barnes, Dmitry Torokhov

Am Dienstag, den 27.04.2021, 12:59 +0000 schrieb David Laight:
> From: Oliver Neukum
> > Sent: 27 April 2021 13:00

> > that is true for those options, but not for the style
> > of PCI hotplug which requires you to push a button and wait
> > for the blinking light.
> 
> True, I remember some of those PCI hotplug chassis from 25 years ago.
> ISTR we did get the removal events working (SVR4/Unixware) but I
> don't remember the relevant chassis ever being sold.
> In spite of the marketing hype I suspect it was only ever possible
> to remove a completely working board and replace it with an
> exactly equivalent one.
> 
> In any case those chassis are not 'surprise removal'.
> 
> More modern drivers are less likely to crash (and burn?) when
> a PCI read returns ~0u.
> But I suspect an awful lot really don't handle surprise removal
> very well at all.

So you are saying that these systems are so rare that it should be
handled  as special cases if at all?

	Regards
		Oliver



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-04-28  6:56             ` Oliver Neukum
@ 2021-04-28 12:21               ` Rafael J. Wysocki
  2021-04-29  9:03                 ` Oliver Neukum
  0 siblings, 1 reply; 25+ messages in thread
From: Rafael J. Wysocki @ 2021-04-28 12:21 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: David Laight, Rafael J. Wysocki, Rajat Jain, Greg Kroah-Hartman,
	Bjorn Helgaas, Alan Stern, Linux Kernel Mailing List, Linux PCI,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Bjorn Helgaas, Rajat Jain, Jesse Barnes, Dmitry Torokhov

On Wed, Apr 28, 2021 at 8:57 AM Oliver Neukum <oneukum@suse.com> wrote:
>
> Am Dienstag, den 27.04.2021, 12:59 +0000 schrieb David Laight:
> > From: Oliver Neukum
> > > Sent: 27 April 2021 13:00
>
> > > that is true for those options, but not for the style
> > > of PCI hotplug which requires you to push a button and wait
> > > for the blinking light.
> >
> > True, I remember some of those PCI hotplug chassis from 25 years ago.
> > ISTR we did get the removal events working (SVR4/Unixware) but I
> > don't remember the relevant chassis ever being sold.
> > In spite of the marketing hype I suspect it was only ever possible
> > to remove a completely working board and replace it with an
> > exactly equivalent one.
> >
> > In any case those chassis are not 'surprise removal'.
> >
> > More modern drivers are less likely to crash (and burn?) when
> > a PCI read returns ~0u.
> > But I suspect an awful lot really don't handle surprise removal
> > very well at all.
>
> So you are saying that these systems are so rare that it should be
> handled  as special cases if at all?

In principle, in the wake of Thunderbolt every PCI driver handling
PCIe devices needs to be able to deal with a device that's gone away
without notice, because in principle any PCIe device can be included
into a Thunderbolt docking station which may go away as a whole
without notice.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-04-28 12:21               ` Rafael J. Wysocki
@ 2021-04-29  9:03                 ` Oliver Neukum
  2021-04-29  9:57                   ` Rafael J. Wysocki
  0 siblings, 1 reply; 25+ messages in thread
From: Oliver Neukum @ 2021-04-29  9:03 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: David Laight, Rajat Jain, Greg Kroah-Hartman, Bjorn Helgaas,
	Alan Stern, Linux Kernel Mailing List, Linux PCI,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Bjorn Helgaas, Rajat Jain, Jesse Barnes, Dmitry Torokhov

Am Mittwoch, den 28.04.2021, 14:21 +0200 schrieb Rafael J. Wysocki:

> In principle, in the wake of Thunderbolt every PCI driver handling
> PCIe devices needs to be able to deal with a device that's gone away
> without notice, because in principle any PCIe device can be included
> into a Thunderbolt docking station which may go away as a whole
> without notice.

Yes, but we are dealing with what we export to user space, don't we?

	Regards
		Oliver




^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-04-29  9:03                 ` Oliver Neukum
@ 2021-04-29  9:57                   ` Rafael J. Wysocki
  2021-04-29 16:59                     ` Rajat Jain
  0 siblings, 1 reply; 25+ messages in thread
From: Rafael J. Wysocki @ 2021-04-29  9:57 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Rafael J. Wysocki, David Laight, Rajat Jain, Greg Kroah-Hartman,
	Bjorn Helgaas, Alan Stern, Linux Kernel Mailing List, Linux PCI,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Bjorn Helgaas, Rajat Jain, Jesse Barnes, Dmitry Torokhov

On Thu, Apr 29, 2021 at 11:03 AM Oliver Neukum <oneukum@suse.com> wrote:
>
> Am Mittwoch, den 28.04.2021, 14:21 +0200 schrieb Rafael J. Wysocki:
>
> > In principle, in the wake of Thunderbolt every PCI driver handling
> > PCIe devices needs to be able to deal with a device that's gone away
> > without notice, because in principle any PCIe device can be included
> > into a Thunderbolt docking station which may go away as a whole
> > without notice.
>
> Yes, but we are dealing with what we export to user space, don't we?

Right, so it would be good to know why exporting this information to
user space is desired.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-04-29  9:57                   ` Rafael J. Wysocki
@ 2021-04-29 16:59                     ` Rajat Jain
  0 siblings, 0 replies; 25+ messages in thread
From: Rajat Jain @ 2021-04-29 16:59 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Oliver Neukum, David Laight, Greg Kroah-Hartman, Bjorn Helgaas,
	Alan Stern, Linux Kernel Mailing List, Linux PCI,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Bjorn Helgaas, Rajat Jain, Jesse Barnes, Dmitry Torokhov

On Thu, Apr 29, 2021 at 2:58 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Apr 29, 2021 at 11:03 AM Oliver Neukum <oneukum@suse.com> wrote:
> >
> > Am Mittwoch, den 28.04.2021, 14:21 +0200 schrieb Rafael J. Wysocki:
> >
> > > In principle, in the wake of Thunderbolt every PCI driver handling
> > > PCIe devices needs to be able to deal with a device that's gone away
> > > without notice, because in principle any PCIe device can be included
> > > into a Thunderbolt docking station which may go away as a whole
> > > without notice.
> >
> > Yes, but we are dealing with what we export to user space, don't we?
>
> Right, so it would be good to know why exporting this information to
> user space is desired.

For us, the driving motivation is to implement policies in userspace
for user removable devices. Eg:

* Tracking the statistics around usage of user removable devices (how
many users use such devices, how often etc).
* Removing user removable devices when a user logs out.
* Not allowing a new user removable device while the screen is locked.
* (perhaps additional such policies in future).

Thanks,

Rajat

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core
  2021-04-24  2:16 [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core Rajat Jain
  2021-04-24  2:16 ` [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices Rajat Jain
  2021-04-25 14:58 ` [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core Alan Stern
@ 2021-05-11 19:22 ` Bjorn Helgaas
  2021-05-11 21:36   ` Rajat Jain
  2021-05-12  1:00 ` Krzysztof Wilczyński
  3 siblings, 1 reply; 25+ messages in thread
From: Bjorn Helgaas @ 2021-05-11 19:22 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Bjorn Helgaas, Alan Stern,
	linux-kernel, linux-pci, linux-usb, rajatxjain, jsbarnes, dtor

On Fri, Apr 23, 2021 at 07:16:30PM -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 ABI for the attribute remains
> unchanged.

Thanks for confirming that the sysfs ABI is unchanged -- it wasn't
obvious to me from the doc descriptions below that the "removable"
file appears in the exact same place it did before.

Wrap above to fill 75 columns.

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

Looks reasonable to me.  Trivial comments below.

Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
> 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                        | 36 +++++++++++++++++++
>  include/linux/usb.h                           |  7 ----
>  8 files changed, 86 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..e13dddd547b5
> --- /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

s/platform specific/platform-specific/

> +		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 f29839382f81..b8ae4cc52805 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2327,6 +2327,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 *state;
> +
> +	switch (dev->removable) {
> +	case DEVICE_REMOVABLE:
> +		state = "removable";
> +		break;
> +	case DEVICE_FIXED:
> +		state = "fixed";
> +		break;
> +	default:
> +		state = "unknown";
> +	}
> +	return sprintf(buf, "%s\n", state);

Maybe sysfs_emit()?

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/filesystems/sysfs.rst?id=v5.12#n246

Ironically, the example below the admonition to use sysfs_emit() still
uses scnprintf() :)

> +}
> +static DEVICE_ATTR_RO(removable);
> +
>  int device_add_groups(struct device *dev, const struct attribute_group **groups)
>  {
>  	return sysfs_create_groups(&dev->kobj, groups);
> @@ -2504,8 +2523,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:
> @@ -2525,6 +2552,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 7f71218cc1e5..500e5648de04 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2442,11 +2442,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;
> @@ -2471,9 +2471,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 d85699bee671..e8ff3afdf7af 100644
> --- a/drivers/usb/core/sysfs.c
> +++ b/drivers/usb/core/sysfs.c
> @@ -298,29 +298,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)
>  {
> @@ -825,7 +802,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 a566bb494e24..5a0f73a28196 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -523,6 +523,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 ba660731bd25..d6442b811607 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -93,6 +93,12 @@ struct device_type {
>  	void (*release)(struct device *dev);
>  
>  	const struct dev_pm_ops *pm;
> +
> +	/*
> +	 * Determines whether the subsystem supports classifying the devices of
> +	 * this type into removable vs fixed.
> +	 */

Doesn't really match comment style in rest of the file.  Seems pretty
self-explanatory, maybe doesn't even need a comment, or perhaps a
short comment like this:

	bool supports_removable:1;  /* subsystem supplies info */

> +	bool supports_removable;

Most boolean structure members in this file are bitfields, e.g.,
"bool offline:1".

>  };
>  
>  /* interface for exporting device attributes */
> @@ -350,6 +356,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.

s/removable,/removable/

Or maybe drop the whole "The criteria ..." sentence?  Not sure it adds
much useful info.

> + * @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 +450,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().
> @@ -541,6 +563,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;
> @@ -778,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 d6a41841b93e..0bbb9e8b18c7 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,
> @@ -701,7 +695,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.498.g6c1eba8ee3d-goog
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-04-24  2:16 ` [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices Rajat Jain
  2021-04-26  9:17   ` Oliver Neukum
@ 2021-05-11 21:30   ` Bjorn Helgaas
  2021-05-11 22:15     ` Rajat Jain
  1 sibling, 1 reply; 25+ messages in thread
From: Bjorn Helgaas @ 2021-05-11 21:30 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Bjorn Helgaas, Alan Stern,
	linux-kernel, linux-pci, linux-usb, Rajat Jain, Jesse Barnes,
	Dmitry Torokhov, Oliver Neukum, David Laight

[+cc Oliver, David]

Please update the subject line, e.g.,

  PCI: Add sysfs "removable" attribute

On Fri, Apr 23, 2021 at 07:16:31PM -0700, Rajat Jain wrote:
> Export the already available info, to the userspace via the
> device core, so that userspace can implement whatever policies it
> wants to, for external removable devices.

I know it's not strictly part of *this* patch, but I think we should
connect the dots a little here, something like this:

  PCI: Add sysfs "removable" attribute

  A PCI device is "external_facing" if it's a Root Port with the ACPI
  "ExternalFacingPort" property or if it has the DT "external-facing"
  property.  We consider everything downstream from such a device to
  be removable.

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

Wrap to fill 75 columns.

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

This looks like a good start.  I think it would be useful to have a
more concrete example of how this information will be used.  I know
that use would be in userspace, so an example probably would not be a
kernel patch.  If you have user code published anywhere, that would
help.  Or even a patch to an existing daemon.  Or pointers to how
"removable" is used for USB devices.

> ---
> v2: Add documentation
> 
>  Documentation/ABI/testing/sysfs-devices-removable |  3 ++-
>  drivers/pci/pci-sysfs.c                           |  1 +
>  drivers/pci/probe.c                               | 12 ++++++++++++
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable
> index e13dddd547b5..daac4f007619 100644
> --- a/Documentation/ABI/testing/sysfs-devices-removable
> +++ b/Documentation/ABI/testing/sysfs-devices-removable
> @@ -14,4 +14,5 @@ Description:
>  
>  		Currently this is only supported by USB (which infers the
>  		information from a combination of hub descriptor bits and
> -		platform-specific data such as ACPI).
> +		platform-specific data such as ACPI) and PCI (which gets this
> +		from ACPI / device tree).
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index f8afd54ca3e1..9302f0076e73 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1582,4 +1582,5 @@ static const struct attribute_group *pci_dev_attr_groups[] = {
>  
>  const struct device_type pci_dev_type = {
>  	.groups = pci_dev_attr_groups,
> +	.supports_removable = true,
>  };
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 953f15abc850..d1cceee62e1b 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1575,6 +1575,16 @@ static void set_pcie_untrusted(struct pci_dev *dev)
>  		dev->untrusted = true;
>  }
>  
> +static void set_pci_dev_removable(struct pci_dev *dev)

Maybe just "pci_set_removable()"?  These "set_pci*" functions look a
little weird.

> +{
> +	struct pci_dev *parent = pci_upstream_bridge(dev);
> +	if (parent &&
> +	    (parent->external_facing || dev_is_removable(&parent->dev)))
> +		dev_set_removable(&dev->dev, DEVICE_REMOVABLE);
> +	else
> +		dev_set_removable(&dev->dev, DEVICE_FIXED);
> +}
> +
>  /**
>   * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config?
>   * @dev: PCI device
> @@ -1819,6 +1829,8 @@ int pci_setup_device(struct pci_dev *dev)
>  	/* "Unknown power state" */
>  	dev->current_state = PCI_UNKNOWN;
>  
> +	set_pci_dev_removable(dev);

So this *only* sets the "removable" attribute based on the
ExternalFacingPort or external-facing properties.  I think Oliver and
David were hinting that maybe we should also set it for devices in
hotpluggable slots.  What do you think?

I wonder if this (and similar hooks like set_pcie_port_type(),
set_pcie_untrusted(), set_pcie_thunderbolt(), etc) should go *after*
the early fixups so we could use fixups to work around issues?

>  	/* Early fixups, before probing the BARs */
>  	pci_fixup_device(pci_fixup_early, dev);
>  
> -- 
> 2.31.1.498.g6c1eba8ee3d-goog
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core
  2021-05-11 19:22 ` Bjorn Helgaas
@ 2021-05-11 21:36   ` Rajat Jain
  0 siblings, 0 replies; 25+ messages in thread
From: Rajat Jain @ 2021-05-11 21:36 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Bjorn Helgaas, Alan Stern,
	Linux Kernel Mailing List, linux-pci,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Rajat Jain, Jesse Barnes, Dmitry Torokhov

Hi Bjorn,

Thanks for the review.

On Tue, May 11, 2021 at 12:22 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Apr 23, 2021 at 07:16:30PM -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 ABI for the attribute remains
> > unchanged.
>
> Thanks for confirming that the sysfs ABI is unchanged -- it wasn't
> obvious to me from the doc descriptions below that the "removable"
> file appears in the exact same place it did before.


Yes, that is correct.

>
>
> Wrap above to fill 75 columns.


Will do.

>
>
> > Signed-off-by: Rajat Jain <rajatja@google.com>
>
> Looks reasonable to me.  Trivial comments below.
>
> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
>
> > ---
> > 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                        | 36 +++++++++++++++++++
> >  include/linux/usb.h                           |  7 ----
> >  8 files changed, 86 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..e13dddd547b5
> > --- /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
>
> s/platform specific/platform-specific/

Will do.

>
> > +             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 f29839382f81..b8ae4cc52805 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -2327,6 +2327,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 *state;
> > +
> > +     switch (dev->removable) {
> > +     case DEVICE_REMOVABLE:
> > +             state = "removable";
> > +             break;
> > +     case DEVICE_FIXED:
> > +             state = "fixed";
> > +             break;
> > +     default:
> > +             state = "unknown";
> > +     }
> > +     return sprintf(buf, "%s\n", state);
>
> Maybe sysfs_emit()?
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/filesystems/sysfs.rst?id=v5.12#n246

Will do.

>
> Ironically, the example below the admonition to use sysfs_emit() still
> uses scnprintf() :)
>
> > +}
> > +static DEVICE_ATTR_RO(removable);
> > +
> >  int device_add_groups(struct device *dev, const struct attribute_group **groups)
> >  {
> >       return sysfs_create_groups(&dev->kobj, groups);
> > @@ -2504,8 +2523,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:
> > @@ -2525,6 +2552,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 7f71218cc1e5..500e5648de04 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -2442,11 +2442,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;
> > @@ -2471,9 +2471,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 d85699bee671..e8ff3afdf7af 100644
> > --- a/drivers/usb/core/sysfs.c
> > +++ b/drivers/usb/core/sysfs.c
> > @@ -298,29 +298,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)
> >  {
> > @@ -825,7 +802,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 a566bb494e24..5a0f73a28196 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -523,6 +523,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 ba660731bd25..d6442b811607 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -93,6 +93,12 @@ struct device_type {
> >       void (*release)(struct device *dev);
> >
> >       const struct dev_pm_ops *pm;
> > +
> > +     /*
> > +      * Determines whether the subsystem supports classifying the devices of
> > +      * this type into removable vs fixed.
> > +      */
>
> Doesn't really match comment style in rest of the file.  Seems pretty
> self-explanatory, maybe doesn't even need a comment, or perhaps a
> short comment like this:
>
>         bool supports_removable:1;  /* subsystem supplies info */
>
> > +     bool supports_removable;

Will do.

>
> Most boolean structure members in this file are bitfields, e.g.,
> "bool offline:1".

Will do.

>
> >  };
> >
> >  /* interface for exporting device attributes */
> > @@ -350,6 +356,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.
>
> s/removable,/removable/

Will do.

>
> Or maybe drop the whole "The criteria ..." sentence?  Not sure it adds
> much useful info.

I'd prefer to keep the comment because I feel it makes it clear that
different buses may employ different criteria to decide what
classifies as "removable" vs "fixed".

Thanks,

Rajat



>
> > + * @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 +450,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().
> > @@ -541,6 +563,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;
> > @@ -778,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 d6a41841b93e..0bbb9e8b18c7 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,
> > @@ -701,7 +695,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.498.g6c1eba8ee3d-goog
> >

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-05-11 21:30   ` Bjorn Helgaas
@ 2021-05-11 22:15     ` Rajat Jain
  2021-05-11 23:02       ` Bjorn Helgaas
  0 siblings, 1 reply; 25+ messages in thread
From: Rajat Jain @ 2021-05-11 22:15 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Bjorn Helgaas, Alan Stern,
	Linux Kernel Mailing List, linux-pci,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Rajat Jain, Jesse Barnes, Dmitry Torokhov, Oliver Neukum,
	David Laight

Hi Bjorn,

Thanks for the review. Please see inline.



On Tue, May 11, 2021 at 2:30 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Oliver, David]
>
> Please update the subject line, e.g.,
>
>   PCI: Add sysfs "removable" attribute

Will do.

>
> On Fri, Apr 23, 2021 at 07:16:31PM -0700, Rajat Jain wrote:
> > Export the already available info, to the userspace via the
> > device core, so that userspace can implement whatever policies it
> > wants to, for external removable devices.
>
> I know it's not strictly part of *this* patch, but I think we should
> connect the dots a little here, something like this:
>
>   PCI: Add sysfs "removable" attribute
>
>   A PCI device is "external_facing" if it's a Root Port with the ACPI
>   "ExternalFacingPort" property or if it has the DT "external-facing"
>   property.  We consider everything downstream from such a device to
>   be removable.
>
>   Set pci_dev_type.supports_removable so the device core exposes the
>   "removable" file in sysfs, and tell the device core about removable
>   devices.
>
> Wrap to fill 75 columns.

Will do.

>
> > Signed-off-by: Rajat Jain <rajatja@google.com>
>
> This looks like a good start.  I think it would be useful to have a
> more concrete example of how this information will be used.  I know
> that use would be in userspace, so an example probably would not be a
> kernel patch.  If you have user code published anywhere, that would
> help.  Or even a patch to an existing daemon.  Or pointers to how
> "removable" is used for USB devices.

Sure, I'll point to some existing user space code (which will be using
a similar attribute we are carrying internally).

>
> > ---
> > v2: Add documentation
> >
> >  Documentation/ABI/testing/sysfs-devices-removable |  3 ++-
> >  drivers/pci/pci-sysfs.c                           |  1 +
> >  drivers/pci/probe.c                               | 12 ++++++++++++
> >  3 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable
> > index e13dddd547b5..daac4f007619 100644
> > --- a/Documentation/ABI/testing/sysfs-devices-removable
> > +++ b/Documentation/ABI/testing/sysfs-devices-removable
> > @@ -14,4 +14,5 @@ Description:
> >
> >               Currently this is only supported by USB (which infers the
> >               information from a combination of hub descriptor bits and
> > -             platform-specific data such as ACPI).
> > +             platform-specific data such as ACPI) and PCI (which gets this
> > +             from ACPI / device tree).
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index f8afd54ca3e1..9302f0076e73 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -1582,4 +1582,5 @@ static const struct attribute_group *pci_dev_attr_groups[] = {
> >
> >  const struct device_type pci_dev_type = {
> >       .groups = pci_dev_attr_groups,
> > +     .supports_removable = true,
> >  };
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 953f15abc850..d1cceee62e1b 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1575,6 +1575,16 @@ static void set_pcie_untrusted(struct pci_dev *dev)
> >               dev->untrusted = true;
> >  }
> >
> > +static void set_pci_dev_removable(struct pci_dev *dev)
>
> Maybe just "pci_set_removable()"?  These "set_pci*" functions look a
> little weird.

Will do.

>
> > +{
> > +     struct pci_dev *parent = pci_upstream_bridge(dev);
> > +     if (parent &&
> > +         (parent->external_facing || dev_is_removable(&parent->dev)))
> > +             dev_set_removable(&dev->dev, DEVICE_REMOVABLE);
> > +     else
> > +             dev_set_removable(&dev->dev, DEVICE_FIXED);
> > +}
> > +
> >  /**
> >   * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config?
> >   * @dev: PCI device
> > @@ -1819,6 +1829,8 @@ int pci_setup_device(struct pci_dev *dev)
> >       /* "Unknown power state" */
> >       dev->current_state = PCI_UNKNOWN;
> >
> > +     set_pci_dev_removable(dev);
>
> So this *only* sets the "removable" attribute based on the
> ExternalFacingPort or external-facing properties.  I think Oliver and
> David were hinting that maybe we should also set it for devices in
> hotpluggable slots.  What do you think?

I did think about it. So I have a mixed feeling about this. Primarily
because I have seen the use of hotpluggable slots in situations where
we wouldn't want to classify the device as removable:

- Using link-state based hotplug as a way to work around unstable PCIe
links. I have seen PCIe devices marked as hot-pluggable only to ensure
that if the PCIe device falls off PCI bus due to some reason (e.g. due
to SI issues or device firmware bugs), the kernel should be able to
detect it if it does come back up (remember quick "Link-Down" /
"Link-Up" events in succession?).

- Internal hot-pluggable PCI devices. In my past life, I was working
on a large system that would have hot-pluggable daughter cards, but
those wouldn't be user removable. Also, it is conceivable to have
hot-pluggable M.2 slots for PCIe devices such as NVMEs etc, but they
may still not be removable by user. I don't think these should be
treated as "removable". I was also looking at USB as an example where
this originally came from, USB does ensure that only devices that are
"user visible" devices are marked as "removable":

54d3f8c63d69 ("usb: Set device removable state based on ACPI USB data")
d35e70d50a06 ("usb: Use hub port data to determine whether a port is removable")

>
> I wonder if this (and similar hooks like set_pcie_port_type(),
> set_pcie_untrusted(), set_pcie_thunderbolt(), etc) should go *after*
> the early fixups so we could use fixups to work around issues?

I agree. We can do that if none of the early fixups actually use the
fields set by these functions. I think it should be ok to move
set_pcie_untrusted(), set_pcie_thunderbolt(), but I wonder if any
early fixups already use the pcie_cap or any other fields set by
set_pcie_port_type().

Thanks,

Rajat

>
> >       /* Early fixups, before probing the BARs */
> >       pci_fixup_device(pci_fixup_early, dev);
> >
> > --
> > 2.31.1.498.g6c1eba8ee3d-goog
> >

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-05-11 22:15     ` Rajat Jain
@ 2021-05-11 23:02       ` Bjorn Helgaas
  2021-05-12  0:02         ` Rajat Jain
  0 siblings, 1 reply; 25+ messages in thread
From: Bjorn Helgaas @ 2021-05-11 23:02 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Bjorn Helgaas, Alan Stern,
	Linux Kernel Mailing List, linux-pci,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Rajat Jain, Jesse Barnes, Dmitry Torokhov, Oliver Neukum,
	David Laight

On Tue, May 11, 2021 at 03:15:11PM -0700, Rajat Jain wrote:
> On Tue, May 11, 2021 at 2:30 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Apr 23, 2021 at 07:16:31PM -0700, Rajat Jain wrote:
> > ...
> > This looks like a good start.  I think it would be useful to have a
> > more concrete example of how this information will be used.  I know
> > that use would be in userspace, so an example probably would not be a
> > kernel patch.  If you have user code published anywhere, that would
> > help.  Or even a patch to an existing daemon.  Or pointers to how
> > "removable" is used for USB devices.
> 
> Sure, I'll point to some existing user space code (which will be using
> a similar attribute we are carrying internally).

Great, thanks!

> > > +     set_pci_dev_removable(dev);
> >
> > So this *only* sets the "removable" attribute based on the
> > ExternalFacingPort or external-facing properties.  I think Oliver and
> > David were hinting that maybe we should also set it for devices in
> > hotpluggable slots.  What do you think?
> 
> I did think about it. So I have a mixed feeling about this. Primarily
> because I have seen the use of hotpluggable slots in situations where
> we wouldn't want to classify the device as removable:
> 
> - Using link-state based hotplug as a way to work around unstable PCIe
> links. I have seen PCIe devices marked as hot-pluggable only to ensure
> that if the PCIe device falls off PCI bus due to some reason (e.g. due
> to SI issues or device firmware bugs), the kernel should be able to
> detect it if it does come back up (remember quick "Link-Down" /
> "Link-Up" events in succession?).
> 
> - Internal hot-pluggable PCI devices. In my past life, I was working
> on a large system that would have hot-pluggable daughter cards, but
> those wouldn't be user removable. Also, it is conceivable to have
> hot-pluggable M.2 slots for PCIe devices such as NVMEs etc, but they
> may still not be removable by user. I don't think these should be
> treated as "removable". I was also looking at USB as an example where
> this originally came from, USB does ensure that only devices that are
> "user visible" devices are marked as "removable":
> 
> 54d3f8c63d69 ("usb: Set device removable state based on ACPI USB data")
> d35e70d50a06 ("usb: Use hub port data to determine whether a port is removable")

IIUC your main concern is consumer platforms where PCI devices would
be hotplugged via a Thunderbolt or similar cable, and that port
would be marked as an "ExternalFacingPort" so we'd mark them as
"removable".

A device in a server hotplug slot would probably *not* be marked as
"removable".  The same device in an external chassis connected via an
iPass or similar cable *might* be "removable" depending on whether the
firmware calls the iPass port an "ExternalFacingPort".

Does the following capture some of what you're thinking?  Maybe some
wordsmithed version of it would be useful in a comment and/or commit
log?

  We're mainly concerned with consumer platforms with accessible
  Thunderbolt ports that are vulnerable to DMA attacks, and we expect
  those ports to be identified as "ExternalFacingPort".

  Devices in traditional hotplug slots are also "removable," but not
  as vulnerable because these slots are less accessible to users.

> > I wonder if this (and similar hooks like set_pcie_port_type(),
> > set_pcie_untrusted(), set_pcie_thunderbolt(), etc) should go *after*
> > the early fixups so we could use fixups to work around issues?
> 
> I agree. We can do that if none of the early fixups actually use the
> fields set by these functions. I think it should be ok to move
> set_pcie_untrusted(), set_pcie_thunderbolt(), but I wonder if any
> early fixups already use the pcie_cap or any other fields set by
> set_pcie_port_type().

I think you should move the one you're adding
(set_pci_dev_removable()) and leave the others where they are for now.

No need to expand the scope of your patch; I was just thinking they're
all basically similar and should ideally be done at similar times.

> > >       /* Early fixups, before probing the BARs */
> > >       pci_fixup_device(pci_fixup_early, dev);
> > >
> > > --
> > > 2.31.1.498.g6c1eba8ee3d-goog
> > >

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-05-11 23:02       ` Bjorn Helgaas
@ 2021-05-12  0:02         ` Rajat Jain
  2021-05-12 22:28           ` Rajat Jain
  0 siblings, 1 reply; 25+ messages in thread
From: Rajat Jain @ 2021-05-12  0:02 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Bjorn Helgaas, Alan Stern,
	Linux Kernel Mailing List, linux-pci,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Rajat Jain, Jesse Barnes, Dmitry Torokhov, Oliver Neukum,
	David Laight

On Tue, May 11, 2021 at 4:02 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, May 11, 2021 at 03:15:11PM -0700, Rajat Jain wrote:
> > On Tue, May 11, 2021 at 2:30 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Fri, Apr 23, 2021 at 07:16:31PM -0700, Rajat Jain wrote:
> > > ...
> > > This looks like a good start.  I think it would be useful to have a
> > > more concrete example of how this information will be used.  I know
> > > that use would be in userspace, so an example probably would not be a
> > > kernel patch.  If you have user code published anywhere, that would
> > > help.  Or even a patch to an existing daemon.  Or pointers to how
> > > "removable" is used for USB devices.
> >
> > Sure, I'll point to some existing user space code (which will be using
> > a similar attribute we are carrying internally).
>
> Great, thanks!
>
> > > > +     set_pci_dev_removable(dev);
> > >
> > > So this *only* sets the "removable" attribute based on the
> > > ExternalFacingPort or external-facing properties.  I think Oliver and
> > > David were hinting that maybe we should also set it for devices in
> > > hotpluggable slots.  What do you think?
> >
> > I did think about it. So I have a mixed feeling about this. Primarily
> > because I have seen the use of hotpluggable slots in situations where
> > we wouldn't want to classify the device as removable:
> >
> > - Using link-state based hotplug as a way to work around unstable PCIe
> > links. I have seen PCIe devices marked as hot-pluggable only to ensure
> > that if the PCIe device falls off PCI bus due to some reason (e.g. due
> > to SI issues or device firmware bugs), the kernel should be able to
> > detect it if it does come back up (remember quick "Link-Down" /
> > "Link-Up" events in succession?).
> >
> > - Internal hot-pluggable PCI devices. In my past life, I was working
> > on a large system that would have hot-pluggable daughter cards, but
> > those wouldn't be user removable. Also, it is conceivable to have
> > hot-pluggable M.2 slots for PCIe devices such as NVMEs etc, but they
> > may still not be removable by user. I don't think these should be
> > treated as "removable". I was also looking at USB as an example where
> > this originally came from, USB does ensure that only devices that are
> > "user visible" devices are marked as "removable":
> >
> > 54d3f8c63d69 ("usb: Set device removable state based on ACPI USB data")
> > d35e70d50a06 ("usb: Use hub port data to determine whether a port is removable")
>
> IIUC your main concern is consumer platforms where PCI devices would
> be hotplugged via a Thunderbolt or similar cable, and that port
> would be marked as an "ExternalFacingPort" so we'd mark them as
> "removable".

Yes.

>
> A device in a server hotplug slot would probably *not* be marked as
> "removable".  The same device in an external chassis connected via an
> iPass or similar cable *might* be "removable" depending on whether the
> firmware calls the iPass port an "ExternalFacingPort".

Yes.

>
> Does the following capture some of what you're thinking?  Maybe some
> wordsmithed version of it would be useful in a comment and/or commit
> log?

Yes, you captured my thoughts perfectly. I shall update the commit log
and / or provide comments to reflect this.

>
>   We're mainly concerned with consumer platforms with accessible
>   Thunderbolt ports that are vulnerable to DMA attacks, and we expect
>   those ports to be identified as "ExternalFacingPort".
>
>   Devices in traditional hotplug slots are also "removable," but not
>   as vulnerable because these slots are less accessible to users.
>
> > > I wonder if this (and similar hooks like set_pcie_port_type(),
> > > set_pcie_untrusted(), set_pcie_thunderbolt(), etc) should go *after*
> > > the early fixups so we could use fixups to work around issues?
> >
> > I agree. We can do that if none of the early fixups actually use the
> > fields set by these functions. I think it should be ok to move
> > set_pcie_untrusted(), set_pcie_thunderbolt(), but I wonder if any
> > early fixups already use the pcie_cap or any other fields set by
> > set_pcie_port_type().
>
> I think you should move the one you're adding
> (set_pci_dev_removable()) and leave the others where they are for now.

Ack, will do.

Thanks,

Rajat

>
> No need to expand the scope of your patch; I was just thinking they're
> all basically similar and should ideally be done at similar times.
>
> > > >       /* Early fixups, before probing the BARs */
> > > >       pci_fixup_device(pci_fixup_early, dev);
> > > >
> > > > --
> > > > 2.31.1.498.g6c1eba8ee3d-goog
> > > >

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core
  2021-04-24  2:16 [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core Rajat Jain
                   ` (2 preceding siblings ...)
  2021-05-11 19:22 ` Bjorn Helgaas
@ 2021-05-12  1:00 ` Krzysztof Wilczyński
  2021-05-12  1:20   ` Rajat Jain
  3 siblings, 1 reply; 25+ messages in thread
From: Krzysztof Wilczyński @ 2021-05-12  1:00 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Bjorn Helgaas, Alan Stern,
	linux-kernel, linux-pci, linux-usb, helgaas, rajatxjain,
	jsbarnes, dtor

Hi Rajat,

I have few questions below, but to add in advance, I might be confusing
the role that "type->supports_removable" and "dev->removable" plays
here, and if so then I apologise.

[...]
> @@ -2504,8 +2523,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;

Would a check for "dev->removable == DEVICE_REMOVABLE" here be more
appropriate?

Unless you wanted to add sysfs objects when the device hints that it has
a notion of being removable even though it might be set to "unknown" or
"fixed" (if that state is at all possible then), and in which case using
the dev_is_removable() helper would also not be an option since it does
a more complex check internally.

Technically, you could always add this sysfs object (similarly to what
USB core did) as it would then show the correct state depending on
"dev->removable".

Also, I suppose, it's not possible for a device to have
"supports_removable" set to true, but "removable" would be different
than "DEVICE_REMOVABLE", correct?

[...]
> +enum device_removable {
> +	DEVICE_REMOVABLE_UNKNOWN = 0,
> +	DEVICE_REMOVABLE,
> +	DEVICE_FIXED,
> +};

I know this was moved from the USB core, but I personally find it
a little bit awkward to read, would something like that be acceptable?

enum device_removable {
	DEVICE_STATE_UNKNOWN = 0,
	DEVICE_STATE_REMOVABLE,
	DEVICE_STATE_FIXED,
};

The addition of state to the name follows the removable_show() function
where the local variable is called "state", and I think it makes sense
to call this as such.  What do you think?

> +static inline bool dev_is_removable(struct device *dev)
> +{
> +	return dev && dev->type && dev->type->supports_removable
> +	    && dev->removable == DEVICE_REMOVABLE;
> +}

Similarly to my question about - would a simple check to see if
"dev->removable" is set to "DEVICE_REMOVABLE" here be enough?

Krzysztof

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core
  2021-05-12  1:00 ` Krzysztof Wilczyński
@ 2021-05-12  1:20   ` Rajat Jain
  2021-05-12 22:27     ` Rajat Jain
  0 siblings, 1 reply; 25+ messages in thread
From: Rajat Jain @ 2021-05-12  1:20 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Rajat Jain, Greg Kroah-Hartman, Rafael J. Wysocki, Bjorn Helgaas,
	Alan Stern, Linux Kernel Mailing List, linux-pci, linux-usb,
	Bjorn Helgaas, Jesse Barnes, Dmitry Torokhov

Hi Krzysztof,

Thanks a lot for your comments. Please see inline.

On Tue, May 11, 2021 at 6:00 PM Krzysztof Wilczyński <kw@linux.com> wrote:
>
> Hi Rajat,
>
> I have few questions below, but to add in advance, I might be confusing
> the role that "type->supports_removable" and "dev->removable" plays
> here, and if so then I apologise.
>
> [...]
> > @@ -2504,8 +2523,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;
>
> Would a check for "dev->removable == DEVICE_REMOVABLE" here be more
> appropriate?
>
> Unless you wanted to add sysfs objects when the device hints that it has
> a notion of being removable even though it might be set to "unknown" or
> "fixed" (if that state is at all possible then), and in which case using
> the dev_is_removable() helper would also not be an option since it does
> a more complex check internally.
>
> Technically, you could always add this sysfs object (similarly to what
> USB core did) as it would then show the correct state depending on
> "dev->removable".
>
> Also, I suppose, it's not possible for a device to have
> "supports_removable" set to true, but "removable" would be different
> than "DEVICE_REMOVABLE", correct?

No, that is not true.

device_type->supports_removable=1 indicates that the bus / subsystem
is capable of differentiating between removable and fixed devices.
It's essentially describing a capability of the bus / subsystem. This
flag needs to be true for a subsystem for any it's devices'
dev->removable field to be considered meaningful.

OTOH, the dev->removable => indicates the location of the device IF
device_type->supports location is true. Yes, it can be fixed /
removable / unknown (whatever the bus decides) if the
device_type->supports_location is true.

One of my primary considerations was also that the existing UAPI for
the USB's "removable" attribute shouldn't be changed. Currently, it
exists for all USB devices, so I think the current code / check is OK.

>
> [...]
> > +enum device_removable {
> > +     DEVICE_REMOVABLE_UNKNOWN = 0,
> > +     DEVICE_REMOVABLE,
> > +     DEVICE_FIXED,
> > +};
>
> I know this was moved from the USB core, but I personally find it
> a little bit awkward to read, would something like that be acceptable?
>
> enum device_removable {
>         DEVICE_STATE_UNKNOWN = 0,
>         DEVICE_STATE_REMOVABLE,
>         DEVICE_STATE_FIXED,
> };
>
> The addition of state to the name follows the removable_show() function
> where the local variable is called "state", and I think it makes sense
> to call this as such.  What do you think?

I think I made a mistake by using the "state" as the local variable
there. I will change it to "location". I'm happy to change the enums
above to DEVICE_LOCATION_REMOVABLE* etc if there is a wider consensus
on this. IMHO, the current shorter one also looks OK.

>
> > +static inline bool dev_is_removable(struct device *dev)
> > +{
> > +     return dev && dev->type && dev->type->supports_removable
> > +         && dev->removable == DEVICE_REMOVABLE;
> > +}
>
> Similarly to my question about - would a simple check to see if
> "dev->removable" is set to "DEVICE_REMOVABLE" here be enough?

No, as I mentioned above, the dev->removable field should be
considered meaningful only if device_type->supports_location is true.
So the check for supports_removable is needed here.

Please feel free to send me more thoughts.

Thanks & Best Regards,

Rajat


>
> Krzysztof

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core
  2021-05-12  1:20   ` Rajat Jain
@ 2021-05-12 22:27     ` Rajat Jain
  2021-05-12 23:32       ` Krzysztof Wilczyński
  0 siblings, 1 reply; 25+ messages in thread
From: Rajat Jain @ 2021-05-12 22:27 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Krzysztof Wilczyński, Greg Kroah-Hartman, Rafael J. Wysocki,
	Bjorn Helgaas, Alan Stern, Linux Kernel Mailing List, linux-pci,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Bjorn Helgaas, Jesse Barnes, Dmitry Torokhov

Posted a v3 of this patch here:
https://lore.kernel.org/patchwork/patch/1428133/

On Tue, May 11, 2021 at 6:21 PM Rajat Jain <rajatxjain@gmail.com> wrote:
>
> Hi Krzysztof,
>
> Thanks a lot for your comments. Please see inline.
>
> On Tue, May 11, 2021 at 6:00 PM Krzysztof Wilczyński <kw@linux.com> wrote:
> >
> > Hi Rajat,
> >
> > I have few questions below, but to add in advance, I might be confusing
> > the role that "type->supports_removable" and "dev->removable" plays
> > here, and if so then I apologise.
> >
> > [...]
> > > @@ -2504,8 +2523,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;
> >
> > Would a check for "dev->removable == DEVICE_REMOVABLE" here be more
> > appropriate?
> >
> > Unless you wanted to add sysfs objects when the device hints that it has
> > a notion of being removable even though it might be set to "unknown" or
> > "fixed" (if that state is at all possible then), and in which case using
> > the dev_is_removable() helper would also not be an option since it does
> > a more complex check internally.
> >
> > Technically, you could always add this sysfs object (similarly to what
> > USB core did) as it would then show the correct state depending on
> > "dev->removable".
> >
> > Also, I suppose, it's not possible for a device to have
> > "supports_removable" set to true, but "removable" would be different
> > than "DEVICE_REMOVABLE", correct?
>
> No, that is not true.
>
> device_type->supports_removable=1 indicates that the bus / subsystem
> is capable of differentiating between removable and fixed devices.
> It's essentially describing a capability of the bus / subsystem. This
> flag needs to be true for a subsystem for any it's devices'
> dev->removable field to be considered meaningful.
>
> OTOH, the dev->removable => indicates the location of the device IF
> device_type->supports location is true. Yes, it can be fixed /
> removable / unknown (whatever the bus decides) if the
> device_type->supports_location is true.
>
> One of my primary considerations was also that the existing UAPI for
> the USB's "removable" attribute shouldn't be changed. Currently, it
> exists for all USB devices, so I think the current code / check is OK.
>
> >
> > [...]
> > > +enum device_removable {
> > > +     DEVICE_REMOVABLE_UNKNOWN = 0,
> > > +     DEVICE_REMOVABLE,
> > > +     DEVICE_FIXED,
> > > +};
> >
> > I know this was moved from the USB core, but I personally find it
> > a little bit awkward to read, would something like that be acceptable?
> >
> > enum device_removable {
> >         DEVICE_STATE_UNKNOWN = 0,
> >         DEVICE_STATE_REMOVABLE,
> >         DEVICE_STATE_FIXED,
> > };
> >
> > The addition of state to the name follows the removable_show() function
> > where the local variable is called "state", and I think it makes sense
> > to call this as such.  What do you think?
>
> I think I made a mistake by using the "state" as the local variable
> there. I will change it to "location". I'm happy to change the enums
> above to DEVICE_LOCATION_REMOVABLE* etc if there is a wider consensus
> on this. IMHO, the current shorter one also looks OK.
>
> >
> > > +static inline bool dev_is_removable(struct device *dev)
> > > +{
> > > +     return dev && dev->type && dev->type->supports_removable
> > > +         && dev->removable == DEVICE_REMOVABLE;
> > > +}
> >
> > Similarly to my question about - would a simple check to see if
> > "dev->removable" is set to "DEVICE_REMOVABLE" here be enough?
>
> No, as I mentioned above, the dev->removable field should be
> considered meaningful only if device_type->supports_location is true.
> So the check for supports_removable is needed here.
>
> Please feel free to send me more thoughts.
>
> Thanks & Best Regards,
>
> Rajat
>
>
> >
> > Krzysztof

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices
  2021-05-12  0:02         ` Rajat Jain
@ 2021-05-12 22:28           ` Rajat Jain
  0 siblings, 0 replies; 25+ messages in thread
From: Rajat Jain @ 2021-05-12 22:28 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Bjorn Helgaas, Alan Stern,
	Linux Kernel Mailing List, linux-pci,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Rajat Jain, Jesse Barnes, Dmitry Torokhov, Oliver Neukum,
	David Laight

Posted v3 of this patch here:
https://lore.kernel.org/patchwork/patch/1428134/

On Tue, May 11, 2021 at 5:02 PM Rajat Jain <rajatja@google.com> wrote:
>
> On Tue, May 11, 2021 at 4:02 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Tue, May 11, 2021 at 03:15:11PM -0700, Rajat Jain wrote:
> > > On Tue, May 11, 2021 at 2:30 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Fri, Apr 23, 2021 at 07:16:31PM -0700, Rajat Jain wrote:
> > > > ...
> > > > This looks like a good start.  I think it would be useful to have a
> > > > more concrete example of how this information will be used.  I know
> > > > that use would be in userspace, so an example probably would not be a
> > > > kernel patch.  If you have user code published anywhere, that would
> > > > help.  Or even a patch to an existing daemon.  Or pointers to how
> > > > "removable" is used for USB devices.
> > >
> > > Sure, I'll point to some existing user space code (which will be using
> > > a similar attribute we are carrying internally).
> >
> > Great, thanks!
> >
> > > > > +     set_pci_dev_removable(dev);
> > > >
> > > > So this *only* sets the "removable" attribute based on the
> > > > ExternalFacingPort or external-facing properties.  I think Oliver and
> > > > David were hinting that maybe we should also set it for devices in
> > > > hotpluggable slots.  What do you think?
> > >
> > > I did think about it. So I have a mixed feeling about this. Primarily
> > > because I have seen the use of hotpluggable slots in situations where
> > > we wouldn't want to classify the device as removable:
> > >
> > > - Using link-state based hotplug as a way to work around unstable PCIe
> > > links. I have seen PCIe devices marked as hot-pluggable only to ensure
> > > that if the PCIe device falls off PCI bus due to some reason (e.g. due
> > > to SI issues or device firmware bugs), the kernel should be able to
> > > detect it if it does come back up (remember quick "Link-Down" /
> > > "Link-Up" events in succession?).
> > >
> > > - Internal hot-pluggable PCI devices. In my past life, I was working
> > > on a large system that would have hot-pluggable daughter cards, but
> > > those wouldn't be user removable. Also, it is conceivable to have
> > > hot-pluggable M.2 slots for PCIe devices such as NVMEs etc, but they
> > > may still not be removable by user. I don't think these should be
> > > treated as "removable". I was also looking at USB as an example where
> > > this originally came from, USB does ensure that only devices that are
> > > "user visible" devices are marked as "removable":
> > >
> > > 54d3f8c63d69 ("usb: Set device removable state based on ACPI USB data")
> > > d35e70d50a06 ("usb: Use hub port data to determine whether a port is removable")
> >
> > IIUC your main concern is consumer platforms where PCI devices would
> > be hotplugged via a Thunderbolt or similar cable, and that port
> > would be marked as an "ExternalFacingPort" so we'd mark them as
> > "removable".
>
> Yes.
>
> >
> > A device in a server hotplug slot would probably *not* be marked as
> > "removable".  The same device in an external chassis connected via an
> > iPass or similar cable *might* be "removable" depending on whether the
> > firmware calls the iPass port an "ExternalFacingPort".
>
> Yes.
>
> >
> > Does the following capture some of what you're thinking?  Maybe some
> > wordsmithed version of it would be useful in a comment and/or commit
> > log?
>
> Yes, you captured my thoughts perfectly. I shall update the commit log
> and / or provide comments to reflect this.
>
> >
> >   We're mainly concerned with consumer platforms with accessible
> >   Thunderbolt ports that are vulnerable to DMA attacks, and we expect
> >   those ports to be identified as "ExternalFacingPort".
> >
> >   Devices in traditional hotplug slots are also "removable," but not
> >   as vulnerable because these slots are less accessible to users.
> >
> > > > I wonder if this (and similar hooks like set_pcie_port_type(),
> > > > set_pcie_untrusted(), set_pcie_thunderbolt(), etc) should go *after*
> > > > the early fixups so we could use fixups to work around issues?
> > >
> > > I agree. We can do that if none of the early fixups actually use the
> > > fields set by these functions. I think it should be ok to move
> > > set_pcie_untrusted(), set_pcie_thunderbolt(), but I wonder if any
> > > early fixups already use the pcie_cap or any other fields set by
> > > set_pcie_port_type().
> >
> > I think you should move the one you're adding
> > (set_pci_dev_removable()) and leave the others where they are for now.
>
> Ack, will do.
>
> Thanks,
>
> Rajat
>
> >
> > No need to expand the scope of your patch; I was just thinking they're
> > all basically similar and should ideally be done at similar times.
> >
> > > > >       /* Early fixups, before probing the BARs */
> > > > >       pci_fixup_device(pci_fixup_early, dev);
> > > > >
> > > > > --
> > > > > 2.31.1.498.g6c1eba8ee3d-goog
> > > > >

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core
  2021-05-12 22:27     ` Rajat Jain
@ 2021-05-12 23:32       ` Krzysztof Wilczyński
  0 siblings, 0 replies; 25+ messages in thread
From: Krzysztof Wilczyński @ 2021-05-12 23:32 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Rajat Jain, Greg Kroah-Hartman, Rafael J. Wysocki, Bjorn Helgaas,
	Alan Stern, Linux Kernel Mailing List, linux-pci,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Bjorn Helgaas, Jesse Barnes, Dmitry Torokhov

Hi Rajat,

> Posted a v3 of this patch here:
> https://lore.kernel.org/patchwork/patch/1428133/

I saw!  Thank you!

I also found the original conversation around the main idea that's
driving the work done here, as per:

  https://lore.kernel.org/lkml/20200601232542.GA473883@bjorn-Precision-5520/

This helped to fill-in some missing pieces, so to speak, I've bad.

[...]
> > > > @@ -2504,8 +2523,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;
> > >
> > One of my primary considerations was also that the existing UAPI for
> > the USB's "removable" attribute shouldn't be changed. Currently, it
> > exists for all USB devices, so I think the current code / check is OK.
[...]

We wouldn't change addition of this attribute to the USB devices - after
the lift to the device core it still has to work as before, as you say.

Just to clarify.   What I was wondering is whether we should add this new
sysfs object so that every device now would get this attribute going
forward, regardless of whether it would have the "type->supports_removable"
set.

We would then have a lot of devices with this attribute set to
"unknown", and then were the is an actual support it would then be
either "fixed" or "removable".

Having said that, Bjorn pointed out to me that this might be
not necessarily desirable.

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

Krzysztof

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2021-05-12 23:44 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-24  2:16 [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core Rajat Jain
2021-04-24  2:16 ` [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices Rajat Jain
2021-04-26  9:17   ` Oliver Neukum
2021-04-26 11:49     ` Rafael J. Wysocki
2021-04-26 13:01       ` David Laight
2021-04-26 19:47         ` Rajat Jain
2021-04-27 11:59         ` Oliver Neukum
2021-04-27 12:59           ` David Laight
2021-04-28  6:56             ` Oliver Neukum
2021-04-28 12:21               ` Rafael J. Wysocki
2021-04-29  9:03                 ` Oliver Neukum
2021-04-29  9:57                   ` Rafael J. Wysocki
2021-04-29 16:59                     ` Rajat Jain
2021-05-11 21:30   ` Bjorn Helgaas
2021-05-11 22:15     ` Rajat Jain
2021-05-11 23:02       ` Bjorn Helgaas
2021-05-12  0:02         ` Rajat Jain
2021-05-12 22:28           ` Rajat Jain
2021-04-25 14:58 ` [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core Alan Stern
2021-05-11 19:22 ` Bjorn Helgaas
2021-05-11 21:36   ` Rajat Jain
2021-05-12  1:00 ` Krzysztof Wilczyński
2021-05-12  1:20   ` Rajat Jain
2021-05-12 22:27     ` Rajat Jain
2021-05-12 23:32       ` Krzysztof Wilczyński

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.