[PATCH] usb: hub: report failure to enumerate uevent to userspace

[PATCH] usb: hub: report failure to enumerate uevent to userspace

[Eugeniu Rosca]

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>
---
 Documentation/ABI/testing/usb-uevent | 36 ++++++++++++++++++++++++++++
 drivers/usb/core/hub.c               | 15 +++++++++++-
 2 files changed, 50 insertions(+), 1 deletion(-)

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:
-- 
2.21.0

Re: [PATCH] usb: hub: report failure to enumerate uevent to userspace

[Greg Kroah-Hartman]

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

Re: [PATCH] usb: hub: report failure to enumerate uevent to userspace

[Eugeniu Rosca]

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

-- 
Best Regards,
Eugeniu.

Re: [PATCH] usb: hub: report failure to enumerate uevent to userspace

[Greg Kroah-Hartman]

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

Re: [PATCH] usb: hub: report failure to enumerate uevent to userspace

[David Howells]

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

Re: [PATCH] usb: hub: report failure to enumerate uevent to userspace

[David Howells]

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