| Message ID | 20190605090556.17792-1-erosca@de.adit-jv.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: hub: report failure to enumerate uevent to userspace | expand |
On Wed, Jun 05, 2019 at 11:05:56AM +0200, Eugeniu Rosca wrote: > From: Spyridon Papageorgiou <spapageorgiou@de.adit-jv.com> > > When a USB device fails to enumerate, only a kernel message is printed. > With this patch, a uevent is also generated to notify userspace. > Services can monitor for the event through udev and handle failures > accordingly. > > The "port_enumerate_fail_notify()" function name follows the syntax of > "port_over_current_notify()" used in v4.20-rc1 > commit 201af55da8a398 ("usb: core: added uevent for over-current"). > > Signed-off-by: Spyridon Papageorgiou <spapageorgiou@de.adit-jv.com> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> All we need is one special notifier! ... {grumble} This can end up causing loads of new kobject change events to be added, overloading what uevents were supposed to be in the first place (add/remove of sysfs objects). I just talked with David Howells, and this type of thing really should be tied into the new "notifier" interface/api. That way you can register for any specific type of event and just get notified of them when they happen. No need to mess with uevents. See his posts on linux-api starting with: Subject: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2] for the proposal. If we added USB (or really any hardware events) to that interface, would it solve the issue you are trying to solve here? thanks, greg k-h
Hi Greg, We really appreciate your feedback. On Wed, Jun 05, 2019 at 12:03:37PM +0200, Greg Kroah-Hartman wrote: > On Wed, Jun 05, 2019 at 11:05:56AM +0200, Eugeniu Rosca wrote: > > From: Spyridon Papageorgiou <spapageorgiou@de.adit-jv.com> > > > > When a USB device fails to enumerate, only a kernel message is printed. > > With this patch, a uevent is also generated to notify userspace. > > Services can monitor for the event through udev and handle failures > > accordingly. > > > > The "port_enumerate_fail_notify()" function name follows the syntax of > > "port_over_current_notify()" used in v4.20-rc1 > > commit 201af55da8a398 ("usb: core: added uevent for over-current"). > > > > Signed-off-by: Spyridon Papageorgiou <spapageorgiou@de.adit-jv.com> > > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > > All we need is one special notifier! ... > > {grumble} > > This can end up causing loads of new kobject change events to be added, > overloading what uevents were supposed to be in the first place > (add/remove of sysfs objects). I guess that's the case for every other kobject_uevent*(*, KOBJ_CHANGE) call in the USB subsystem (in case of either HW or code misbehavior). JFTR, there are around 120 such calls in the entire v5.2-rc3 kernel. > > I just talked with David Howells, and this type of thing really should > be tied into the new "notifier" interface/api. That way you can > register for any specific type of event and just get notified of them > when they happen. No need to mess with uevents. > > See his posts on linux-api starting with: > Subject: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2] > for the proposal. > > If we added USB (or really any hardware events) to that interface, would > it solve the issue you are trying to solve here? I checked this patch series in linux-fs.git [3], as well as shared my thoughts with our security and RFS experts, and we came up with the following questions/remarks: - Looking at commit [4], it seems that the new "notifier" interface/api forces userspace applications to link against -lkeyutils [5]. Assuming the latter is designed for ("Kernel key management") [6], it may look like the keyutils library is being abused to handle the "USB (or really any hardware events)". Do you really plan to extend the scope of the library to handle these new tasks? - Currently, to be able to get kobject uevent notifications, our applications must include "libudev.h" and must link against -ludev. By using the feature implemented in [3], we would significantly increase the complexity of those applications, particularly because they would need to arbitrate between two different categories of events received via two different APIs. - It is also my assumption that the existing KOBJ_CHANGE events cannot be easily converted to the new API, since this would hurt a dozen of userland applications relying on them. Overall, I am quite clueless how to proceed with this patch, except to keep it in our internal tree, most likely forever. Any comments/recommendations would be appreciated. > > thanks, > > greg k-h [1] linux (v5.2-rc3) git grep KOBJ_CHANGE -- drivers/usb/ drivers/usb/core/hub.c: kobject_uevent_env(&hub_dev->kobj, KOBJ_CHANGE, envp); drivers/usb/gadget/udc/core.c: kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); drivers/usb/gadget/udc/core.c: kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); drivers/usb/phy/phy.c: kobject_uevent_env(&usb_phy->dev->kobj, KOBJ_CHANGE, envp); drivers/usb/typec/class.c: kobject_uevent(&adev->dev.kobj, KOBJ_CHANGE); drivers/usb/typec/class.c: kobject_uevent(&port->dev.kobj, KOBJ_CHANGE); drivers/usb/typec/class.c: kobject_uevent(&port->dev.kobj, KOBJ_CHANGE); drivers/usb/typec/class.c: kobject_uevent(&port->dev.kobj, KOBJ_CHANGE); drivers/usb/typec/class.c: kobject_uevent(&port->dev.kobj, KOBJ_CHANGE); [2] git grep -w KOBJ_CHANGE -- ":\!Documentation" ":\!include" | wc -l 122 [3] https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=notifications [4] https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=16a8aad951990 [5] https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/keyutils.git [6] https://lwn.net/Articles/210502/ ("Kernel key management")
On Wed, Jun 05, 2019 at 06:55:30PM +0200, Eugeniu Rosca wrote: > Hi Greg, > > We really appreciate your feedback. > > On Wed, Jun 05, 2019 at 12:03:37PM +0200, Greg Kroah-Hartman wrote: > > On Wed, Jun 05, 2019 at 11:05:56AM +0200, Eugeniu Rosca wrote: > > > From: Spyridon Papageorgiou <spapageorgiou@de.adit-jv.com> > > > > > > When a USB device fails to enumerate, only a kernel message is printed. > > > With this patch, a uevent is also generated to notify userspace. > > > Services can monitor for the event through udev and handle failures > > > accordingly. > > > > > > The "port_enumerate_fail_notify()" function name follows the syntax of > > > "port_over_current_notify()" used in v4.20-rc1 > > > commit 201af55da8a398 ("usb: core: added uevent for over-current"). > > > > > > Signed-off-by: Spyridon Papageorgiou <spapageorgiou@de.adit-jv.com> > > > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > > > > All we need is one special notifier! ... > > > > {grumble} > > > > This can end up causing loads of new kobject change events to be added, > > overloading what uevents were supposed to be in the first place > > (add/remove of sysfs objects). > > I guess that's the case for every other kobject_uevent*(*, KOBJ_CHANGE) > call in the USB subsystem (in case of either HW or code misbehavior). We only currently have 8 of those in USB: - over current notification (the new one added) - gadget driver removed - gadget driver unregistered (these 2 are odd...) - phy charger state change (like other power sources provide) - typec alt mode change - typec data role change - typec power role change - typec vconn role change - typec power operation change Only the over current notification is something that is "not normal", and was just recently added because no one could think of a better way to do it. But now we have a better way to do it :) > JFTR, there are around 120 such calls in the entire v5.2-rc3 kernel. Most of those are state changes, which is fine. They are not error conditions, correct? > > I just talked with David Howells, and this type of thing really should > > be tied into the new "notifier" interface/api. That way you can > > register for any specific type of event and just get notified of them > > when they happen. No need to mess with uevents. > > > > See his posts on linux-api starting with: > > Subject: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2] > > for the proposal. > > > > If we added USB (or really any hardware events) to that interface, would > > it solve the issue you are trying to solve here? > > I checked this patch series in linux-fs.git [3], as well as shared my > thoughts with our security and RFS experts, and we came up with the > following questions/remarks: > > - Looking at commit [4], it seems that the new "notifier" interface/api > forces userspace applications to link against -lkeyutils [5]. > Assuming the latter is designed for ("Kernel key management") [6], > it may look like the keyutils library is being abused to handle > the "USB (or really any hardware events)". Do you really plan to > extend the scope of the library to handle these new tasks? You can write notifier libraries for any subsystem, no need to link against any other type of subsystem (i.e. if you only care about USB ones, you will not need keyutils.) > - Currently, to be able to get kobject uevent notifications, our > applications must include "libudev.h" and must link against -ludev. > By using the feature implemented in [3], we would significantly > increase the complexity of those applications, particularly because > they would need to arbitrate between two different categories of > events received via two different APIs. What other event do you get today that requires you to use libudev that a notifier for USB events would not provide you? Also, given that we haven't written such code, we can work together to ensure that all of the events you care about are present. > - It is also my assumption that the existing KOBJ_CHANGE events cannot > be easily converted to the new API, since this would hurt a dozen of > userland applications relying on them. For USB, there is only one such odd event (as listed above). For other kobjects, we can work to implement state change notification as well. > Overall, I am quite clueless how to proceed with this patch, except to > keep it in our internal tree, most likely forever. Any > comments/recommendations would be appreciated. Please respond to David's patch series if you have any questions/issues about it. I do not want to add random new USB event notifications through KOBJ_CHANGE until we come to a decision as to what this new event notification framework will look like. If it is not possible for USB to fit into that, then I will be glad to revisit this patch. thanks, greg k-h
Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > - Looking at commit [4], it seems that the new "notifier" interface/api > forces userspace applications to link against -lkeyutils [5]. No. The keyctl(2) syscall is implemented in -lkeyutils library, and not in -lc. That's all. If you want to call KEYCTL_NOTIFY to watch a key or keyring, you need it; not otherwise. > Assuming the latter is designed for ("Kernel key management") [6], > it may look like the keyutils library is being abused to handle > the "USB (or really any hardware events)". Do you really plan to > extend the scope of the library to handle these new tasks? No. That said, it's probably worth providing some userspace library to wrap the ring management. David
David Howells <dhowells@redhat.com> wrote: > > - Looking at commit [4], it seems that the new "notifier" interface/api > > forces userspace applications to link against -lkeyutils [5]. > > No. The keyctl(2) syscall is implemented in -lkeyutils library, and not in > -lc. That's all. If you want to call KEYCTL_NOTIFY to watch a key or > keyring, you need it; not otherwise. Sorry, KEYCTL_WATCH_KEY, not KEYCTL_NOTIFY. I've changed my mind about the naming of things a couple of times and should probably harmonise them. David
diff --git a/Documentation/ABI/testing/usb-uevent b/Documentation/ABI/testing/usb-uevent index d35c3cad892c..23e618227d31 100644 --- a/Documentation/ABI/testing/usb-uevent +++ b/Documentation/ABI/testing/usb-uevent @@ -25,3 +25,39 @@ Description: When the USB Host Controller has entered a state where it is no TYPE=9/0/1 Users: chromium-os-dev@chromium.org + +What: Raise a uevent when USB device enumeration has failed +Date: 2019-06-05 +KernelVersion: 5.2 +Contact: linux-usb@vger.kernel.org +Description: When a USB device has failed to enumerate, a uevent will be generated. + The uevent will contain ACTION=change, ENUMERATION_FAILURE=1 and + ENUMERATION_FAIL_PORT=<port_id>. Services can monitor for the event + through udev and handle failures accordingly. + + Here is an example taken using udevadm monitor -p (R-Car H3ULCB): + + UDEV [47.298493] change /devices/platform/soc/ee0a0000.usb/usb4/4-0:1.0 (usb) + ACTION=change + DEVPATH=/devices/platform/soc/ee0a0000.usb/usb4/4-0:1.0 + DEVTYPE=usb_interface + DRIVER=hub + ENUMERATION_FAILURE=1 + ENUMERATION_FAIL_PORT=1 + ID_MODEL_FROM_DATABASE=1.1 root hub + ID_USB_CLASS_FROM_DATABASE=Hub + ID_USB_PROTOCOL_FROM_DATABASE=Full speed (or root) hub + ID_VENDOR_FROM_DATABASE=Linux Foundation + INTERFACE=9/0/0 + MODALIAS=usb:v1D6Bp0001d0502dc09dsc00dp00ic09isc00ip00in00 + OF_COMPATIBLE_0=generic-ohci + OF_COMPATIBLE_N=1 + OF_FULLNAME=/soc/usb@ee0a0000 + OF_NAME=usb + PRODUCT=1d6b/1/502 + SEQNUM=1762 + SUBSYSTEM=usb + TYPE=9/0/0 + USEC_INITIALIZED=24344435 + +Users: ADIT, DENSO TEN diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 2f94568ba385..da1a3d47a071 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -4921,6 +4921,17 @@ hub_power_remaining(struct usb_hub *hub) return remaining; } +static void port_enumerate_fail_notify(struct usb_port *port) +{ + char env_port[32]; + char *envp[3] = { "ENUMERATION_FAILURE=1", env_port, NULL }; + + snprintf(env_port, sizeof(env_port), "ENUMERATION_FAIL_PORT=%d", + port->portnum); + + kobject_uevent_env(&port->dev.parent->kobj, KOBJ_CHANGE, envp); +} + static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus, u16 portchange) { @@ -5131,9 +5142,11 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus, if (hub->hdev->parent || !hcd->driver->port_handed_over || !(hcd->driver->port_handed_over)(hcd, port1)) { - if (status != -ENOTCONN && status != -ENODEV) + if (status != -ENOTCONN && status != -ENODEV) { + port_enumerate_fail_notify(port_dev); dev_err(&port_dev->dev, "unable to enumerate USB device\n"); + } } done: