Skip to contentSkip to conversation

ANDROID: usb: core: Send uevent when USB TOPO layer over 6

PATCH In review Ricky Niu 2 participants 7 messages Feb 26, 2021 – Mar 5, 2021 USB

lore
Share & tools
lore

Ricky Niu

author
#0 lore raw

PATCH 1/1 ANDROID: usb: core: Send uevent when USB TOPO layer over 6

When the topology of the nested hubs are over 6 layers Send uevent to user space when USB TOPO layer over 6. Let end user more understand what happened.

---
 drivers/usb/core/hub.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
drivers/usb/core/hub.c +22 -0
index 7f71218cc1e5..e5e924526822 100644
@@ -55,6 +55,10 @@ static DEFINE_SPINLOCK(device_state_lock);
 static struct workqueue_struct *hub_wq;
 static void hub_event(struct work_struct *work);
 
+/* struct to notify userspace of hub events */
+static struct class *hub_class;
+static struct device *hub_device;
+
 /* synchronize hub-port add/remove and peering operations */
 DEFINE_MUTEX(usb_port_peer_mutex);
 
@@ -1764,6 +1768,13 @@ static bool hub_descriptor_is_sane(struct usb_host_interface *desc)
         return true;
 }
 
+static void hub_over_tier(void)
+{
+	char *envp[2] = { "HUB=OVERTIER", NULL };
+
+	kobject_uevent_env(&hub_device->kobj, KOBJ_CHANGE, envp);
+}
+
 static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
 {
 	struct usb_host_interface *desc;
@@ -1831,6 +1842,7 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	if (hdev->level == MAX_TOPO_LEVEL) {
 		dev_err(&intf->dev,
 			"Unsupported bus topology: hub nested too deep\n");
+		hub_over_tier();
 		return -E2BIG;
 	}
 
@@ -5680,6 +5692,13 @@ int usb_hub_init(void)
 		return -1;
 	}
 
+	hub_class = class_create(THIS_MODULE, "usb_hub");
+	if (IS_ERR(hub_class))
+		return PTR_ERR(hub_class);
+
+	hub_device =
+		device_create(hub_class, NULL, MKDEV(0, 0), NULL, "usb_hub");
+
 	/*
 	 * The workqueue needs to be freezable to avoid interfering with
 	 * USB-PERSIST port handover. Otherwise it might see that a full-speed
@@ -5699,6 +5718,9 @@ int usb_hub_init(void)
 
 void usb_hub_cleanup(void)
 {
+	if (!IS_ERR(hub_class))
+		class_destroy(hub_class);
+
 	destroy_workqueue(hub_wq);
 
 	/*
Signature · 1 line
2.30.1.766.gb4fecdf3b7-goog
maintainer
#1 lore raw
Quoted 25 lines · Ricky Niu

When the topology of the nested hubs are over 6 layers Send uevent to user space when USB TOPO layer over 6. Let end user more understand what happened.

---
 drivers/usb/core/hub.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
drivers/usb/core/hub.c +4 -0
index 7f71218cc1e5..e5e924526822 100644
@@ -55,6 +55,10 @@ static DEFINE_SPINLOCK(device_state_lock);
 static struct workqueue_struct *hub_wq;
 static void hub_event(struct work_struct *work);
 
+/* struct to notify userspace of hub events */
+static struct class *hub_class;
+static struct device *hub_device;
+
 /* synchronize hub-port add/remove and peering operations */
 DEFINE_MUTEX(usb_port_peer_mutex);
 

drivers/usb/core/hub.c static bool hub_descriptor_is_sane(struct usb_host_interface *desc) · line 1775

Quoted 8 lines
drivers/usb/core/hub.c +5 -0
@@ -1764,6 +1768,13 @@ static bool hub_descriptor_is_sane(struct usb_host_interface *desc)
         return true;
 }
 
+static void hub_over_tier(void)
+{
+	char *envp[2] = { "HUB=OVERTIER", NULL };
+
+	kobject_uevent_env(&hub_device->kobj, KOBJ_CHANGE, envp);

Where have you now documented this odd uevent that is never sent by anything else?

What tool will "catch" this? Where is that code located at?

uevents are not for stuff like this, you are trying to send "error conditions" to userspace, please use the "proper" interfaces like this and not abuse existing ones.

Quoted 14 lines
drivers/usb/core/hub.c +3 -0
+}
+
 static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
 {
 	struct usb_host_interface *desc;
@@ -1831,6 +1842,7 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	if (hdev->level == MAX_TOPO_LEVEL) {
 		dev_err(&intf->dev,
 			"Unsupported bus topology: hub nested too deep\n");
+		hub_over_tier();
 		return -E2BIG;
 	}
 

drivers/usb/core/hub.c int usb_hub_init(void) · line 5700

Quoted 9 lines
drivers/usb/core/hub.c +6 -0
@@ -5680,6 +5692,13 @@ int usb_hub_init(void)
 		return -1;
 	}
 
+	hub_class = class_create(THIS_MODULE, "usb_hub");
+	if (IS_ERR(hub_class))
+		return PTR_ERR(hub_class);
+
+	hub_device =
+		device_create(hub_class, NULL, MKDEV(0, 0), NULL, "usb_hub");

You just created a whole new sysfs class with no Documentation/ABI/ update?

{sigh}

greg k-h

maintainer
#2 lore raw
In reply to Ricky Niu
Quoted 14 lines · Ricky Niu

When the topology of the nested hubs are over 6 layers Send uevent to user space when USB TOPO layer over 6. Let end user more understand what happened.

---
 drivers/usb/core/hub.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
drivers/usb/core/hub.c +0 -0
index 7f71218cc1e5..e5e924526822 100644

drivers/usb/core/hub.c static DEFINE_SPINLOCK(device_state_lock); · line 60

Quoted 6 lines
drivers/usb/core/hub.c +3 -0
@@ -55,6 +55,10 @@ static DEFINE_SPINLOCK(device_state_lock);
 static struct workqueue_struct *hub_wq;
 static void hub_event(struct work_struct *work);
 
+/* struct to notify userspace of hub events */
+static struct class *hub_class;
+static struct device *hub_device;

Wait, how did you even test this code? This will not work if you have more than one hub in the system at a single time, right?

That's going to be really rough, given here's the output of just my desktop system, count the number of hubs in it:rdevmgmt.msc

$ lsusb -t
/:  Bus 10.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 10000M
/:  Bus 09.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/6p, 480M
    |__ Port 5: Dev 2, If 0, Class=Wireless, Driver=btusb, 12M
    |__ Port 5: Dev 2, If 1, Class=Wireless, Driver=btusb, 12M
    |__ Port 6: Dev 3, If 0, Class=Hub, Driver=hub/4p, 480M
/:  Bus 08.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 10000M
    |__ Port 2: Dev 2, If 0, Class=Hub, Driver=hub/4p, 5000M
    |__ Port 3: Dev 3, If 0, Class=Mass Storage, Driver=uas, 10000M
/:  Bus 07.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/6p, 480M
    |__ Port 2: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
        |__ Port 2: Dev 4, If 0, Class=Hub, Driver=hub/2p, 12M
            |__ Port 2: Dev 5, If 0, Class=Human Interface Device, Driver=usbhid, 12M
            |__ Port 2: Dev 5, If 1, Class=Human Interface Device, Driver=usbhid, 12M
            |__ Port 2: Dev 5, If 2, Class=Human Interface Device, Driver=usbhid, 12M
    |__ Port 5: Dev 3, If 3, Class=Audio, Driver=snd-usb-audio, 480M
    |__ Port 5: Dev 3, If 1, Class=Audio, Driver=snd-usb-audio, 480M
    |__ Port 5: Dev 3, If 6, Class=Audio, Driver=snd-usb-audio, 480M
    |__ Port 5: Dev 3, If 4, Class=Audio, Driver=snd-usb-audio, 480M
    |__ Port 5: Dev 3, If 2, Class=Audio, Driver=snd-usb-audio, 480M
    |__ Port 5: Dev 3, If 0, Class=Audio, Driver=snd-usb-audio, 480M
    |__ Port 5: Dev 3, If 7, Class=Human Interface Device, Driver=usbhid, 480M
    |__ Port 5: Dev 3, If 5, Class=Audio, Driver=snd-usb-audio, 480M
    |__ Port 6: Dev 6, If 0, Class=Human Interface Device, Driver=usbhid, 12M
/:  Bus 06.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/1p, 10000M/x2
/:  Bus 05.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/1p, 480M
/:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 10000M
    |__ Port 1: Dev 11, If 0, Class=Hub, Driver=hub/3p, 5000M
        |__ Port 3: Dev 12, If 0, Class=Hub, Driver=hub/3p, 5000M
            |__ Port 1: Dev 13, If 0, Class=Mass Storage, Driver=usb-storage, 5000M
/:  Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 480M
    |__ Port 1: Dev 14, If 0, Class=Hub, Driver=hub/3p, 480M
        |__ Port 3: Dev 15, If 0, Class=Hub, Driver=hub/3p, 480M
            |__ Port 2: Dev 17, If 0, Class=Human Interface Device, Driver=usbhid, 12M
            |__ Port 3: Dev 18, If 3, Class=Human Interface Device, Driver=usbhid, 12M
            |__ Port 3: Dev 18, If 1, Class=Audio, Driver=snd-usb-audio, 12M
            |__ Port 3: Dev 18, If 2, Class=Audio, Driver=snd-usb-audio, 12M
            |__ Port 3: Dev 18, If 0, Class=Audio, Driver=snd-usb-audio, 12M
/:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 10000M
/:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 480M

So, proof that this works? How did you test this?

Also, you have a memory leak in this submission :(

greg k-h

#3 lore raw

Hi , Greg

What tool will "catch" this? Where is that code located at? => I prepare merge the code to Android phone , so I used Android HLOS to catch this uevent.

uevents are not for stuff like this, you are trying to send "error conditions" to userspace, please use the "proper" interfaces like this and not abuse existing ones.
=> Sorry , I am not sure what is the "proper" interfaces your mean.
Could you please give me more description?

You just created a whole new sysfs class with no Documentation/ABI/ update?
=> Yes, I will add it.

Wait, how did you even test this code? This will not work if you have more than one hub in the system at a single time, right?
=> I build the test build which flash in Android phone and connect several hubs to try it.
When the new hub which met MAX_TOPO_LEVEL connected , it sent
notify to user space.

Phone ------↓

                 hub ------↓
                             hub ------↓
                                           ...------↓
                                                    hub

     if (hdev->level == MAX_TOPO_LEVEL) {
                dev_err(&intf->dev,
                        "Unsupported bus topology: hub nested too deep\n");
                hub_over_tier();
                return -E2BIG;
     }

So, proof that this works? How did you test this?
=> I use the Pixel phone to verify the code , the framework received the uevent when the last connected hub over "MAX_TOPO_LEVEL".

Also, you have a memory leak in this submission :( => Do you mean I should add device_destroy here ?

hub_device =
device_create(hub_class, NULL, MKDEV(0, 0), NULL, "usb_hub"); +if (IS_ERR(hub_device))
+ return PTR_ERR(hub_device);

void usb_hub_cleanup(void)
{
+if (!IS_ERR(hub_device))
+device_destroy(hub_class, hub_device->devt);

if (!IS_ERR(hub_class))
class_destroy(hub_class);

Best Regards,

Chien Kun Niu
[email protected]

Chien Kun Niu
SSD USB
[email protected]
02 8729 0651

Greg KH <[email protected]> 於 2021年2月26日 週五 下午5:31寫道:

Quoted 76 lines · Ricky Niu
Quoted 14 lines

When the topology of the nested hubs are over 6 layers Send uevent to user space when USB TOPO layer over 6. Let end user more understand what happened.

---
 drivers/usb/core/hub.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
drivers/usb/core/hub.c +0 -0
index 7f71218cc1e5..e5e924526822 100644

drivers/usb/core/hub.c static DEFINE_SPINLOCK(device_state_lock); · line 60

Quoted 6 lines
drivers/usb/core/hub.c +3 -0
@@ -55,6 +55,10 @@ static DEFINE_SPINLOCK(device_state_lock);
 static struct workqueue_struct *hub_wq;
 static void hub_event(struct work_struct *work);
 
+/* struct to notify userspace of hub events */
+static struct class *hub_class;
+static struct device *hub_device;

Wait, how did you even test this code? This will not work if you have more than one hub in the system at a single time, right?

That's going to be really rough, given here's the output of just my desktop system, count the number of hubs in it:rdevmgmt.msc

$ lsusb -t
/:  Bus 10.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 10000M
/:  Bus 09.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/6p, 480M
    |__ Port 5: Dev 2, If 0, Class=Wireless, Driver=btusb, 12M
    |__ Port 5: Dev 2, If 1, Class=Wireless, Driver=btusb, 12M
    |__ Port 6: Dev 3, If 0, Class=Hub, Driver=hub/4p, 480M
/:  Bus 08.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/4p, 10000M
    |__ Port 2: Dev 2, If 0, Class=Hub, Driver=hub/4p, 5000M
    |__ Port 3: Dev 3, If 0, Class=Mass Storage, Driver=uas, 10000M
/:  Bus 07.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/6p, 480M
    |__ Port 2: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
        |__ Port 2: Dev 4, If 0, Class=Hub, Driver=hub/2p, 12M
            |__ Port 2: Dev 5, If 0, Class=Human Interface Device, Driver=usbhid, 12M
            |__ Port 2: Dev 5, If 1, Class=Human Interface Device, Driver=usbhid, 12M
            |__ Port 2: Dev 5, If 2, Class=Human Interface Device, Driver=usbhid, 12M
    |__ Port 5: Dev 3, If 3, Class=Audio, Driver=snd-usb-audio, 480M
    |__ Port 5: Dev 3, If 1, Class=Audio, Driver=snd-usb-audio, 480M
    |__ Port 5: Dev 3, If 6, Class=Audio, Driver=snd-usb-audio, 480M
    |__ Port 5: Dev 3, If 4, Class=Audio, Driver=snd-usb-audio, 480M
    |__ Port 5: Dev 3, If 2, Class=Audio, Driver=snd-usb-audio, 480M
    |__ Port 5: Dev 3, If 0, Class=Audio, Driver=snd-usb-audio, 480M
    |__ Port 5: Dev 3, If 7, Class=Human Interface Device, Driver=usbhid, 480M
    |__ Port 5: Dev 3, If 5, Class=Audio, Driver=snd-usb-audio, 480M
    |__ Port 6: Dev 6, If 0, Class=Human Interface Device, Driver=usbhid, 12M
/:  Bus 06.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/1p, 10000M/x2
/:  Bus 05.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/1p, 480M
/:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 10000M
    |__ Port 1: Dev 11, If 0, Class=Hub, Driver=hub/3p, 5000M
        |__ Port 3: Dev 12, If 0, Class=Hub, Driver=hub/3p, 5000M
            |__ Port 1: Dev 13, If 0, Class=Mass Storage, Driver=usb-storage, 5000M
/:  Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 480M
    |__ Port 1: Dev 14, If 0, Class=Hub, Driver=hub/3p, 480M
        |__ Port 3: Dev 15, If 0, Class=Hub, Driver=hub/3p, 480M
            |__ Port 2: Dev 17, If 0, Class=Human Interface Device, Driver=usbhid, 12M
            |__ Port 3: Dev 18, If 3, Class=Human Interface Device, Driver=usbhid, 12M
            |__ Port 3: Dev 18, If 1, Class=Audio, Driver=snd-usb-audio, 12M
            |__ Port 3: Dev 18, If 2, Class=Audio, Driver=snd-usb-audio, 12M
            |__ Port 3: Dev 18, If 0, Class=Audio, Driver=snd-usb-audio, 12M
/:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 10000M
/:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 480M

So, proof that this works? How did you test this?

Also, you have a memory leak in this submission :(

greg k-h

maintainer
#4 lore raw
Quoted 5 lines · Chien Kun Niu

Hi , Greg

What tool will "catch" this? Where is that code located at? => I prepare merge the code to Android phone , so I used Android HLOS to catch this uevent.

Very odd quoting style, perhaps you might want to read up on how to do this properly at:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

Quoted 5 lines

uevents are not for stuff like this, you are trying to send "error conditions" to userspace, please use the "proper" interfaces like this and not abuse existing ones.
=> Sorry , I am not sure what is the "proper" interfaces your mean.
Could you please give me more description?

How does the kernel normally send error conditions that it detects in hardware to userspace?

Quoted 12 lines

You just created a whole new sysfs class with no Documentation/ABI/ update?
=> Yes, I will add it.

Wait, how did you even test this code? This will not work if you have more than one hub in the system at a single time, right?
=> I build the test build which flash in Android phone and connect several hubs to try it.
When the new hub which met MAX_TOPO_LEVEL connected , it sent
notify to user space.

Phone ------↓

Quoted 12 lines
                 hub ------↓
                             hub ------↓
                                           ...------↓
                                                    hub

     if (hdev->level == MAX_TOPO_LEVEL) {
                dev_err(&intf->dev,
                        "Unsupported bus topology: hub nested too deep\n");
                hub_over_tier();
                return -E2BIG;
     }

But you only have a single hub variable, and a huge memory leak, did you not detect that in your testing?

Quoted 3 lines

So, proof that this works? How did you test this?
=> I use the Pixel phone to verify the code , the framework received the uevent when the last connected hub over "MAX_TOPO_LEVEL".

Try it on a desktop as well, with many hubs and see what happens :(

Quoted 2 lines

Also, you have a memory leak in this submission :( => Do you mean I should add device_destroy here ?

What do you think should be done?

Quoted 11 lines

hub_device =
device_create(hub_class, NULL, MKDEV(0, 0), NULL, "usb_hub");

(quoted diff) +2 -0
+if (IS_ERR(hub_device))
+               return PTR_ERR(hub_device);

void usb_hub_cleanup(void)
{

(quoted diff) +2 -0
+if (!IS_ERR(hub_device))
+device_destroy(hub_class, hub_device->devt);
Quoted 2 lines

if (!IS_ERR(hub_class))
class_destroy(hub_class);

I don't think you are understanding that you can have multiple hubs in the system at the same time :(

thanks,

greg k-h

#5 lore raw

Greg KH <[email protected]> 於 2021年3月3日 週三 下午5:10寫道:

Quoted 12 lines · Chien Kun Niu
Quoted 5 lines

Hi , Greg

What tool will "catch" this? Where is that code located at? => I prepare merge the code to Android phone , so I used Android HLOS to catch this uevent.

Very odd quoting style, perhaps you might want to read up on how to do this properly at:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

Quoted 9 lines
Quoted 5 lines

uevents are not for stuff like this, you are trying to send "error conditions" to userspace, please use the "proper" interfaces like this and not abuse existing ones.
=> Sorry , I am not sure what is the "proper" interfaces your mean.
Could you please give me more description?

How does the kernel normally send error conditions that it detects in hardware to userspace?

I will create a sysfs attribute to record the hub status. If there is a new hub with over 6 USB TOPO layer connected, I will use the sysfs_notify to send the "error conditions" to userspace. Is it a proper interfaces to delivery "error conditions"?

Quoted 56 lines
Quoted 12 lines

You just created a whole new sysfs class with no Documentation/ABI/ update?
=> Yes, I will add it.

Wait, how did you even test this code? This will not work if you have more than one hub in the system at a single time, right?
=> I build the test build which flash in Android phone and connect several hubs to try it.
When the new hub which met MAX_TOPO_LEVEL connected , it sent
notify to user space.

Phone ------↓

Quoted 12 lines
                 hub ------↓
                             hub ------↓
                                           ...------↓
                                                    hub

     if (hdev->level == MAX_TOPO_LEVEL) {
                dev_err(&intf->dev,
                        "Unsupported bus topology: hub nested too deep\n");
                hub_over_tier();
                return -E2BIG;
     }

But you only have a single hub variable, and a huge memory leak, did you not detect that in your testing?

Quoted 3 lines

So, proof that this works? How did you test this?
=> I use the Pixel phone to verify the code , the framework received the uevent when the last connected hub over "MAX_TOPO_LEVEL".

Try it on a desktop as well, with many hubs and see what happens :(

Quoted 2 lines

Also, you have a memory leak in this submission :( => Do you mean I should add device_destroy here ?

What do you think should be done?

Quoted 11 lines

hub_device =
device_create(hub_class, NULL, MKDEV(0, 0), NULL, "usb_hub");

(quoted diff) +2 -0
+if (IS_ERR(hub_device))
+               return PTR_ERR(hub_device);

void usb_hub_cleanup(void)
{

(quoted diff) +2 -0
+if (!IS_ERR(hub_device))
+device_destroy(hub_class, hub_device->devt);
Quoted 2 lines

if (!IS_ERR(hub_class))
class_destroy(hub_class);

I don't think you are understanding that you can have multiple hubs in the system at the same time :(

Quoted 3 lines

thanks,

greg k-h

Thanks,

Chien Kun Niu

maintainer
#6 lore raw
Quoted 19 lines · Chien Kun Niu

Greg KH <[email protected]> 於 2021年3月3日 週三 下午5:10寫道:

Quoted 18 lines · Chien Kun Niu
Quoted 5 lines

Hi , Greg

What tool will "catch" this? Where is that code located at? => I prepare merge the code to Android phone , so I used Android HLOS to catch this uevent.

Very odd quoting style, perhaps you might want to read up on how to do this properly at:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

Quoted 5 lines

uevents are not for stuff like this, you are trying to send "error conditions" to userspace, please use the "proper" interfaces like this and not abuse existing ones.
=> Sorry , I am not sure what is the "proper" interfaces your mean.
Could you please give me more description?

Quoted 8 lines
Quoted 3 lines

How does the kernel normally send error conditions that it detects in hardware to userspace?

I will create a sysfs attribute to record the hub status. If there is a new hub with over 6 USB TOPO layer connected, I will use the sysfs_notify to send the "error conditions" to userspace. Is it a proper interfaces to delivery "error conditions"?

Maybe, it all depends on what you are wanting to show here. Try it out and see, it's easier to review patches that you have shown work properly for your use case than it is to try to discuss general issues.

thanks,

greg k-h