Messages in this thread | | | From | Won Chung <> | Date | Tue, 22 Mar 2022 09:36:02 -0700 | Subject | Re: [PATCH v6] driver core: Add sysfs support for physical location of a device |
| |
On Mon, Mar 14, 2022 at 12:55 PM Won Chung <wonchung@google.com> wrote: > > When ACPI table includes _PLD fields for a device, create a new > directory (physical_location) in sysfs to share _PLD fields. > > Currently without PLD information, when there are multiple of same > devices, it is hard to distinguish which device corresponds to which > physical device at which location. For example, when there are two Type > C connectors, it is hard to find out which connector corresponds to the > Type C port on the left panel versus the Type C port on the right panel. > With PLD information provided, we can determine which specific device at > which location is doing what. > > _PLD output includes much more fields, but only generic fields are added > and exposed to sysfs, so that non-ACPI devices can also support it in > the future. The minimal generic fields needed for locating a device are > the following. > - panel > - vertical_position > - horizontal_position > - dock > - lid > > Signed-off-by: Won Chung <wonchung@google.com> > --- > > Changes from v5 > - Remove physical_location directory only if physical_location is present. > - Free memory for physical_location in device when device is removed. > > Changes from v4 > - Remove physical_location directory when device is deleted. > - Correctly handle error from adding physical_location in > device_add_attrs(). > > Changes from v3 > - Move dev_add_physical_location() and dev_attr_physical_location_group > to driver/base/physical_location.h. > - Use pointer and reorder physical_location in struct device to pack its > bytes. (checked using pahole) > - Unify naming to physical_location since the name location is used in > some places like USB port for different information. > > Changes from v2 > - Use sysfs_emit to create files. > - Correct mix of spaces and tabs. > > Changes from v1 > - Correct directory names in Documentation. > - Clarify namings in core.c > > .../testing/sysfs-devices-physical_location | 42 ++++++ > drivers/base/Makefile | 1 + > drivers/base/core.c | 15 ++ > drivers/base/physical_location.c | 137 ++++++++++++++++++ > drivers/base/physical_location.h | 16 ++ > include/linux/device.h | 73 ++++++++++ > 6 files changed, 284 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-devices-physical_location > create mode 100644 drivers/base/physical_location.c > create mode 100644 drivers/base/physical_location.h > > diff --git a/Documentation/ABI/testing/sysfs-devices-physical_location b/Documentation/ABI/testing/sysfs-devices-physical_location > new file mode 100644 > index 000000000000..202324b87083 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-devices-physical_location > @@ -0,0 +1,42 @@ > +What: /sys/devices/.../physical_location > +Date: March 2022 > +Contact: Won Chung <wonchung@google.com> > +Description: > + This directory contains information on physical location of > + the device connection point with respect to the system's > + housing. > + > +What: /sys/devices/.../physical_location/panel > +Date: March 2022 > +Contact: Won Chung <wonchung@google.com> > +Description: > + Describes which panel surface of the system’s housing the > + device connection point resides on. > + > +What: /sys/devices/.../physical_location/vertical_position > +Date: March 2022 > +Contact: Won Chung <wonchung@google.com> > +Description: > + Describes vertical position of the device connection point on > + the panel surface. > + > +What: /sys/devices/.../physical_location/horizontal_position > +Date: March 2022 > +Contact: Won Chung <wonchung@google.com> > +Description: > + Describes horizontal position of the device connection point on > + the panel surface. > + > +What: /sys/devices/.../physical_location/dock > +Date: March 2022 > +Contact: Won Chung <wonchung@google.com> > +Description: > + "Yes" if the device connection point resides in a docking > + station or a port replicator. "No" otherwise. > + > +What: /sys/devices/.../physical_location/lid > +Date: March 2022 > +Contact: Won Chung <wonchung@google.com> > +Description: > + "Yes" if the device connection point resides on the lid of > + laptop system. "No" otherwise. > diff --git a/drivers/base/Makefile b/drivers/base/Makefile > index 02f7f1358e86..83217d243c25 100644 > --- a/drivers/base/Makefile > +++ b/drivers/base/Makefile > @@ -25,6 +25,7 @@ obj-$(CONFIG_DEV_COREDUMP) += devcoredump.o > obj-$(CONFIG_GENERIC_MSI_IRQ_DOMAIN) += platform-msi.o > obj-$(CONFIG_GENERIC_ARCH_TOPOLOGY) += arch_topology.o > obj-$(CONFIG_GENERIC_ARCH_NUMA) += arch_numa.o > +obj-$(CONFIG_ACPI) += physical_location.o > > obj-y += test/ > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 7bb957b11861..64d4d8646c12 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -32,6 +32,7 @@ > #include <linux/dma-map-ops.h> /* for dma_default_coherent */ > > #include "base.h" > +#include "physical_location.h" > #include "power/power.h" > > #ifdef CONFIG_SYSFS_DEPRECATED > @@ -2649,8 +2650,17 @@ static int device_add_attrs(struct device *dev) > goto err_remove_dev_waiting_for_supplier; > } > > + if (dev_add_physical_location(dev)) { > + error = device_add_group(dev, > + &dev_attr_physical_location_group); > + if (error) > + goto err_remove_dev_removable; > + } > + > return 0; > > + err_remove_dev_removable: > + device_remove_file(dev, &dev_attr_removable); > err_remove_dev_waiting_for_supplier: > device_remove_file(dev, &dev_attr_waiting_for_supplier); > err_remove_dev_online: > @@ -2672,6 +2682,11 @@ static void device_remove_attrs(struct device *dev) > struct class *class = dev->class; > const struct device_type *type = dev->type; > > + if (dev->physical_location) { > + device_remove_group(dev, &dev_attr_physical_location_group); > + kfree(dev->physical_location); > + } > + > device_remove_file(dev, &dev_attr_removable); > device_remove_file(dev, &dev_attr_waiting_for_supplier); > device_remove_file(dev, &dev_attr_online); > diff --git a/drivers/base/physical_location.c b/drivers/base/physical_location.c > new file mode 100644 > index 000000000000..4c1a52ecd7f6 > --- /dev/null > +++ b/drivers/base/physical_location.c > @@ -0,0 +1,137 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Device physical location support > + * > + * Author: Won Chung <wonchung@google.com> > + */ > + > +#include <linux/acpi.h> > +#include <linux/sysfs.h> > + > +#include "physical_location.h" > + > +bool dev_add_physical_location(struct device *dev) > +{ > + struct acpi_pld_info *pld; > + acpi_status status; > + > + if (!has_acpi_companion(dev)) > + return false; > + > + status = acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld); > + if (ACPI_FAILURE(status)) > + return false; > + > + dev->physical_location = > + kzalloc(sizeof(*dev->physical_location), GFP_KERNEL); > + dev->physical_location->panel = pld->panel; > + dev->physical_location->vertical_position = pld->vertical_position; > + dev->physical_location->horizontal_position = pld->horizontal_position; > + dev->physical_location->dock = pld->dock; > + dev->physical_location->lid = pld->lid; > + > + return true; > +} > + > +static ssize_t panel_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + const char *panel; > + > + switch (dev->physical_location->panel) { > + case DEVICE_PANEL_TOP: > + panel = "top"; > + break; > + case DEVICE_PANEL_BOTTOM: > + panel = "bottom"; > + break; > + case DEVICE_PANEL_LEFT: > + panel = "left"; > + break; > + case DEVICE_PANEL_RIGHT: > + panel = "right"; > + break; > + case DEVICE_PANEL_FRONT: > + panel = "front"; > + break; > + default: > + panel = "unknown"; > + } > + return sysfs_emit(buf, "%s\n", panel); > +} > +static DEVICE_ATTR_RO(panel); > + > +static ssize_t vertical_position_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + const char *vertical_position; > + > + switch (dev->physical_location->vertical_position) { > + case DEVICE_VERT_POS_UPPER: > + vertical_position = "upper"; > + break; > + case DEVICE_VERT_POS_CENTER: > + vertical_position = "center"; > + break; > + case DEVICE_VERT_POS_LOWER: > + vertical_position = "lower"; > + break; > + default: > + vertical_position = "unknown"; > + } > + return sysfs_emit(buf, "%s\n", vertical_position); > +} > +static DEVICE_ATTR_RO(vertical_position); > + > +static ssize_t horizontal_position_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + const char *horizontal_position; > + > + switch (dev->physical_location->horizontal_position) { > + case DEVICE_HORI_POS_LEFT: > + horizontal_position = "left"; > + break; > + case DEVICE_HORI_POS_CENTER: > + horizontal_position = "center"; > + break; > + case DEVICE_HORI_POS_RIGHT: > + horizontal_position = "right"; > + break; > + default: > + horizontal_position = "unknown"; > + } > + return sysfs_emit(buf, "%s\n", horizontal_position); > +} > +static DEVICE_ATTR_RO(horizontal_position); > + > +static ssize_t dock_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + return sysfs_emit(buf, "%s\n", > + dev->physical_location->dock ? "yes" : "no"); > +} > +static DEVICE_ATTR_RO(dock); > + > +static ssize_t lid_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + return sysfs_emit(buf, "%s\n", > + dev->physical_location->lid ? "yes" : "no"); > +} > +static DEVICE_ATTR_RO(lid); > + > +static struct attribute *dev_attr_physical_location[] = { > + &dev_attr_panel.attr, > + &dev_attr_vertical_position.attr, > + &dev_attr_horizontal_position.attr, > + &dev_attr_dock.attr, > + &dev_attr_lid.attr, > + NULL, > +}; > + > +const struct attribute_group dev_attr_physical_location_group = { > + .name = "physical_location", > + .attrs = dev_attr_physical_location, > +}; > + > diff --git a/drivers/base/physical_location.h b/drivers/base/physical_location.h > new file mode 100644 > index 000000000000..82cde9f1b161 > --- /dev/null > +++ b/drivers/base/physical_location.h > @@ -0,0 +1,16 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Device physical location support > + * > + * Author: Won Chung <wonchung@google.com> > + */ > + > +#include <linux/device.h> > + > +#ifdef CONFIG_ACPI > +extern bool dev_add_physical_location(struct device *dev); > +extern const struct attribute_group dev_attr_physical_location_group; > +#else > +static inline bool dev_add_physical_location(struct device *dev) { return false; }; > +static const struct attribute_group dev_attr_physical_location_group = {}; > +#endif > diff --git a/include/linux/device.h b/include/linux/device.h > index 93459724dcde..766fbea6ca83 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -386,6 +386,75 @@ struct dev_msi_info { > #endif > }; > > +/** > + * enum device_physical_location_panel - Describes which panel surface of the > + * system's housing the device connection point resides on. > + * @DEVICE_PANEL_TOP: Device connection point is on the top panel. > + * @DEVICE_PANEL_BOTTOM: Device connection point is on the bottom panel. > + * @DEVICE_PANEL_LEFT: Device connection point is on the left panel. > + * @DEVICE_PANEL_RIGHT: Device connection point is on the right panel. > + * @DEVICE_PANEL_FRONT: Device connection point is on the front panel. > + * @DEVICE_PANEL_BACK: Device connection point is on the back panel. > + * @DEVICE_PANEL_UNKNOWN: The panel with device connection point is unknown. > + */ > +enum device_physical_location_panel { > + DEVICE_PANEL_TOP, > + DEVICE_PANEL_BOTTOM, > + DEVICE_PANEL_LEFT, > + DEVICE_PANEL_RIGHT, > + DEVICE_PANEL_FRONT, > + DEVICE_PANEL_BACK, > + DEVICE_PANEL_UNKNOWN, > +}; > + > +/** > + * enum device_physical_location_vertical_position - Describes vertical > + * position of the device connection point on the panel surface. > + * @DEVICE_VERT_POS_UPPER: Device connection point is at upper part of panel. > + * @DEVICE_VERT_POS_CENTER: Device connection point is at center part of panel. > + * @DEVICE_VERT_POS_LOWER: Device connection point is at lower part of panel. > + */ > +enum device_physical_location_vertical_position { > + DEVICE_VERT_POS_UPPER, > + DEVICE_VERT_POS_CENTER, > + DEVICE_VERT_POS_LOWER, > +}; > + > +/** > + * enum device_physical_location_horizontal_position - Describes horizontal > + * position of the device connection point on the panel surface. > + * @DEVICE_HORI_POS_LEFT: Device connection point is at left part of panel. > + * @DEVICE_HORI_POS_CENTER: Device connection point is at center part of panel. > + * @DEVICE_HORI_POS_RIGHT: Device connection point is at right part of panel. > + */ > +enum device_physical_location_horizontal_position { > + DEVICE_HORI_POS_LEFT, > + DEVICE_HORI_POS_CENTER, > + DEVICE_HORI_POS_RIGHT, > +}; > + > +/** > + * struct device_physical_location - Device data related to physical location > + * of the device connection point. > + * @panel: Panel surface of the system's housing that the device connection > + * point resides on. > + * @vertical_position: Vertical position of the device connection point within > + * the panel. > + * @horizontal_position: Horizontal position of the device connection point > + * within the panel. > + * @dock: Set if the device connection point resides in a docking station or > + * port replicator. > + * @lid: Set if this device connection point resides on the lid of laptop > + * system. > + */ > +struct device_physical_location { > + enum device_physical_location_panel panel; > + enum device_physical_location_vertical_position vertical_position; > + enum device_physical_location_horizontal_position horizontal_position; > + bool dock; > + bool lid; > +}; > + > /** > * struct device - The basic device structure > * @parent: The device's "parent" device, the device to which it is attached. > @@ -453,6 +522,8 @@ struct dev_msi_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 > + * @physical_location: Describes physical location of the device connection > + * point in the system housing. > * @removable: Whether the device can be removed from the system. This > * should be set by the subsystem / bus driver that discovered > * the device. > @@ -567,6 +638,8 @@ struct device { > struct iommu_group *iommu_group; > struct dev_iommu *iommu; > > + struct device_physical_location *physical_location; > + > enum device_removable removable; > > bool offline_disabled:1; > -- > 2.35.1.723.g4982287a31-goog >
Hi Greg,
I am sorry to keep bothering you with this, but can you take a look at this patch for a review when you have time? Thank you very much!
Won
| |