| Message ID | 20180928121847.16335-1-bjorn@mork.no (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] usb: export firmware port location in sysfs | expand |
On Fri, 28 Sep 2018, Bjørn Mork wrote: > The platform firmware "location" data is used to find port peer > relationships. But firmware is an unreliable source, and there are > real world examples of errors leading to missing or wrong peer > relationships. Debugging this is currently hard. > > Exporting the location attribute makes it easier to spot mismatches > between the firmware data and the real world. > > Signed-off-by: Bjørn Mork <bjorn@mork.no> > --- > v3: resurrect missing hunk, as pointed out by the kbuild test robot > v2: Sorry, forgot to rebase the old branch before sending v1 > > This patch got stuck in one of my debugging branches. It proved very > useful to me while trying to figure out why the "peer" link was useless > on a specific host. And if it was useful to me once, then maybe it will > be to someone else as well... > > tl;dr; The full use case for anyone interested: > > Some current LTE modems with USB3 SS support come with a bootloader > supporting USB2 only. The application and bootloader modes are provided > by different softwares running on the modem (which of course is just > another Linux system with a UDC). None of the descriptors are therefore > guaranteed to be identical, or even similar. > > Doing a firmware upgrade of such a device involves > - some preparation in application mode (USB3), > - rebooting into bootloader mode (USB2), and > - finally booting into the upgraded application firmware (USB3) to verify > the upgrade. > > The firmware upgrade tool should make sure it talks to the same physical > device in all three phases. The only semi-reliable way to do that is to > look for "new" devices in the expected mode, connected to the same physical > USB port. But the locical port will change due to the USB2/3 switch, and > all we are left with is the "peer" link. Which can, and do, fail due to > buggy ACPI tables. > > This patch won't solve that problem, but it makes it a lot easier to > detect. > > Lesson for the next time: Either submit rightaway or just delete it. > Three attempts to get this even building without a warning is more > than embarrassing. Sorry about that. It *is* build tested now. And > even run tested. Honestly ;-) > > > Documentation/ABI/testing/sysfs-bus-usb | 18 ++++++++++++++---- > drivers/usb/core/port.c | 10 ++++++++++ > 2 files changed, 24 insertions(+), 4 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb > index 08d456e07b53..8f394c976fee 100644 > --- a/Documentation/ABI/testing/sysfs-bus-usb > +++ b/Documentation/ABI/testing/sysfs-bus-usb > @@ -173,14 +173,14 @@ Description: > The file will be present for all speeds of USB devices, and will > always read "no" for USB 1.1 and USB 2.0 devices. > > -What: /sys/bus/usb/devices/.../(hub interface)/portX > +What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX This name change doesn't have anything to do with the purpose of the patch. It also doesn't make any sense, and in fact it isn't actually implemented in the patch. > Date: August 2012 > Contact: Lan Tianyu <tianyu.lan@intel.com> > Description: > The /sys/bus/usb/devices/.../(hub interface)/portX Nor is it reflected here. > is usb port device's sysfs directory. > > -What: /sys/bus/usb/devices/.../(hub interface)/portX/connect_type > +What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX/connect_type Ditto. > Date: January 2013 > Contact: Lan Tianyu <tianyu.lan@intel.com> > Description: > @@ -189,7 +189,17 @@ Description: > The file will read "hotplug", "wired" and "not used" if the > information is available, and "unknown" otherwise. > > -What: /sys/bus/usb/devices/.../(hub interface)/portX/quirks > +What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX/location Ditto. > +Date: October 2018 > +Contact: Bjørn Mork <bjorn@mork.no> > +Description: > + Some platforms provide usb port physical location through > + firmware. This is used by the kernel to pair up logical ports > + mapping to the same physical connector. The attribute exposes the > + raw location value as a hex integer. > + > + > +What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX/quirks Ditto > Date: May 2018 > Contact: Nicolas Boichat <drinkcat@chromium.org> > Description: > @@ -211,7 +221,7 @@ Description: > used to help make enumeration work better on some high speed > devices. > > -What: /sys/bus/usb/devices/.../(hub interface)/portX/over_current_count > +What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX/over_current_count Ditto. Alan Stern > Date: February 2018 > Contact: Richard Leitner <richard.leitner@skidata.com> > Description: > diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c > index 4a2143195395..1a06a4b5fbb1 100644 > --- a/drivers/usb/core/port.c > +++ b/drivers/usb/core/port.c > @@ -16,6 +16,15 @@ static int usb_port_block_power_off; > > static const struct attribute_group *port_dev_group[]; > > +static ssize_t location_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct usb_port *port_dev = to_usb_port(dev); > + > + return sprintf(buf, "0x%08x\n", port_dev->location); > +} > +static DEVICE_ATTR_RO(location); > + > static ssize_t connect_type_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > @@ -140,6 +149,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit); > > static struct attribute *port_dev_attrs[] = { > &dev_attr_connect_type.attr, > + &dev_attr_location.attr, > &dev_attr_quirks.attr, > &dev_attr_over_current_count.attr, > NULL, >
Alan Stern <stern@rowland.harvard.edu> writes: >> diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb >> index 08d456e07b53..8f394c976fee 100644 >> --- a/Documentation/ABI/testing/sysfs-bus-usb >> +++ b/Documentation/ABI/testing/sysfs-bus-usb >> @@ -173,14 +173,14 @@ Description: >> The file will be present for all speeds of USB devices, and will >> always read "no" for USB 1.1 and USB 2.0 devices. >> >> -What: /sys/bus/usb/devices/.../(hub interface)/portX >> +What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX > > This name change doesn't have anything to do with the purpose of the > patch. It also doesn't make any sense, and in fact it isn't actually > implemented in the patch. Yuck. You're right of course. Will fix. Bjørn
diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb index 08d456e07b53..8f394c976fee 100644 --- a/Documentation/ABI/testing/sysfs-bus-usb +++ b/Documentation/ABI/testing/sysfs-bus-usb @@ -173,14 +173,14 @@ Description: The file will be present for all speeds of USB devices, and will always read "no" for USB 1.1 and USB 2.0 devices. -What: /sys/bus/usb/devices/.../(hub interface)/portX +What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX Date: August 2012 Contact: Lan Tianyu <tianyu.lan@intel.com> Description: The /sys/bus/usb/devices/.../(hub interface)/portX is usb port device's sysfs directory. -What: /sys/bus/usb/devices/.../(hub interface)/portX/connect_type +What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX/connect_type Date: January 2013 Contact: Lan Tianyu <tianyu.lan@intel.com> Description: @@ -189,7 +189,17 @@ Description: The file will read "hotplug", "wired" and "not used" if the information is available, and "unknown" otherwise. -What: /sys/bus/usb/devices/.../(hub interface)/portX/quirks +What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX/location +Date: October 2018 +Contact: Bjørn Mork <bjorn@mork.no> +Description: + Some platforms provide usb port physical location through + firmware. This is used by the kernel to pair up logical ports + mapping to the same physical connector. The attribute exposes the + raw location value as a hex integer. + + +What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX/quirks Date: May 2018 Contact: Nicolas Boichat <drinkcat@chromium.org> Description: @@ -211,7 +221,7 @@ Description: used to help make enumeration work better on some high speed devices. -What: /sys/bus/usb/devices/.../(hub interface)/portX/over_current_count +What: /sys/bus/usb/devices/.../(hub interface)/usbY-portX/over_current_count Date: February 2018 Contact: Richard Leitner <richard.leitner@skidata.com> Description: diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c index 4a2143195395..1a06a4b5fbb1 100644 --- a/drivers/usb/core/port.c +++ b/drivers/usb/core/port.c @@ -16,6 +16,15 @@ static int usb_port_block_power_off; static const struct attribute_group *port_dev_group[]; +static ssize_t location_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct usb_port *port_dev = to_usb_port(dev); + + return sprintf(buf, "0x%08x\n", port_dev->location); +} +static DEVICE_ATTR_RO(location); + static ssize_t connect_type_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -140,6 +149,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit); static struct attribute *port_dev_attrs[] = { &dev_attr_connect_type.attr, + &dev_attr_location.attr, &dev_attr_quirks.attr, &dev_attr_over_current_count.attr, NULL,
The platform firmware "location" data is used to find port peer relationships. But firmware is an unreliable source, and there are real world examples of errors leading to missing or wrong peer relationships. Debugging this is currently hard. Exporting the location attribute makes it easier to spot mismatches between the firmware data and the real world. Signed-off-by: Bjørn Mork <bjorn@mork.no> --- v3: resurrect missing hunk, as pointed out by the kbuild test robot v2: Sorry, forgot to rebase the old branch before sending v1 This patch got stuck in one of my debugging branches. It proved very useful to me while trying to figure out why the "peer" link was useless on a specific host. And if it was useful to me once, then maybe it will be to someone else as well... tl;dr; The full use case for anyone interested: Some current LTE modems with USB3 SS support come with a bootloader supporting USB2 only. The application and bootloader modes are provided by different softwares running on the modem (which of course is just another Linux system with a UDC). None of the descriptors are therefore guaranteed to be identical, or even similar. Doing a firmware upgrade of such a device involves - some preparation in application mode (USB3), - rebooting into bootloader mode (USB2), and - finally booting into the upgraded application firmware (USB3) to verify the upgrade. The firmware upgrade tool should make sure it talks to the same physical device in all three phases. The only semi-reliable way to do that is to look for "new" devices in the expected mode, connected to the same physical USB port. But the locical port will change due to the USB2/3 switch, and all we are left with is the "peer" link. Which can, and do, fail due to buggy ACPI tables. This patch won't solve that problem, but it makes it a lot easier to detect. Lesson for the next time: Either submit rightaway or just delete it. Three attempts to get this even building without a warning is more than embarrassing. Sorry about that. It *is* build tested now. And even run tested. Honestly ;-) Documentation/ABI/testing/sysfs-bus-usb | 18 ++++++++++++++---- drivers/usb/core/port.c | 10 ++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-)