[PATCH] USB: hub: Move typec deattach before children disconnections in usb_disconnect()

[Marco Tormento <mtormento80@gmail.com>]

I have a Thinkpad T480s, it has 2 USB typec ports one of which is part of
the thunderbolt connector. On the thunderbolt typec port I plug a BenQ
EX3501R external monitor that has an integrated USB hub with a couple of
HID devices connected to it. On the other typec port I plug an Apple
Macbook USB-C power brick.

Steps to reproduce the issue this patch is supposed to fix:
- plug the power brick into typec port
- plug the monitor into the other typec port (thunderbolt)
- unplug the monitor
- unplug the power brick: get a warning stack trace saying "kernfs: can not
  remove 'typec', no directory" in typec_unregister_partner()
- plug the power brick again
- plug the monitor again
- unplug the monitor
- plug the monitor again: get a warning stack trace saying "sysfs: cannot
  create duplicate filename
  '/devices/platform/USBC000:00/typec/port0/port0-partner/3-1'" in
  typec_partner_link_device() called by typec_partner_attach()

From that point on if you do the following steps a couple of times you get
a kernel oops with a general protection fault in
typec_unregister_partner(), system becomes unstable and a restart is
required:
- unplug the monitor
- unplug the power brick
- plug the power brick
- plug the monitor
- unplug the monitor
- unplug the power brick

If I plug the power brick to thunderbolt and the monitor to the other typec
port none of the above happens.

I tracked down the issue to the following logic:
- power brick is plugged in
- monitor is plugged in
- when I unplug the monitor in usb_disconnect() hub_disconnect_children()
  calls usb_disconnect() recursively, and this results in
  connector_unbind() invoked on all connectors, which resets
  port_dev->connector to NULL on the ports
- typec_deattach() is called for each device that has a parent, which in
  turn should fire typec_partner_deattach()
- port_dev->connector is NULL though, so typec_partner_deattach() is not
  called and port->usb2_dev is not set to NULL even though the hub device
  is actually gone

Now if I plug the monitor again:
- since the type_partner_deattach() was not invoked before, some links are
  still there and typec_partner_link_device() complains

If I unplug the power brick instead:
- ucsi_handle_connector_change() is invoked and typec_unregister_partner()
  is executed, port->usb2_dev is not NULL so we try to unlink the device
  from the partner with typec_partner_unlink_device(), but 'typec'
  directory doesn't exist anymore so it fails in the sysfs_remove_link()
  call

The reason why at some point kernel oops when I unplug the power brick a
final time is this: in typec_unregister_partner() when
typec_partner_unlink_device() is called port->usb2_dev->kobj ref count is
zero (the device is gone already), so when accessing usb_dev->kobj fields
it's just a matter of time and memory will be overwritten with something
else, which will cause access to protected memory and an oops.

In order to fix the above I tried moving the code that handles typec
deattachment from the parent before all the disconnections, this way
typec_partner_deattach() is invoked for the partner, port->usb2_dev is
cleared and typec_unregister_partner() is happy.

Note
It's still not clear to me why the hub device of the monitor is linked with
the typec port partner of the power brick in /sys/class/typec. The
thunderbolt controller surely has something to do with it, since it doesn't
happen if the monitor is plugged into the standard typec port instead, but
I haven't fully understood why it happens yet: maybe this is the actual
bug?
e.g.
Plug the power brick:
- /sys/class/typec/port0-partner appears
Plug the monitor:
- /sys/class/typec/port1-partner appears
- /sys/class/typec/port0-partner/3-1 appears, which is the monitor hub
If I plug the monitor first and then the power brick
/sys/class/typec/port0-partner/3-1 doesn't show up.

Disclaimer
I just started looking at kernel code for the first time a week ago, so all
of the above might be spectacularly wrong or a better way to fix the issue
might exist. Initially I did a quick fix that was just checking
port->usb2_dev->kobj ref count in typec_partner_unlink_device() and bailing
out if it was zero, but then I decided to dig deeper just for fun.
This patch has solved my problem, I'm running it on laptop since last week
without any issue, but it might have side-effects I'm not aware of.
I really hope I'm not wasting your time, thanks for all the great work you
do on the Linux kernel!

Signed-off-by: Marco Tormento <mtormento80@gmail.com>
---
 drivers/usb/core/hub.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 256fe8c86828..dfe4ba192faf 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2334,16 +2334,6 @@ void usb_disconnect(struct usb_device **pdev)
 
 	usb_lock_device(udev);
 
-	hub_disconnect_children(udev);
-
-	/* deallocate hcd/hardware state ... nuking all pending urbs and
-	 * cleaning up all state associated with the current configuration
-	 * so that the hardware is now fully quiesced.
-	 */
-	dev_dbg(&udev->dev, "unregistering device\n");
-	usb_disable_device(udev, 0);
-	usb_hcd_synchronize_unlinks(udev);
-
 	if (udev->parent) {
 		port1 = udev->portnum;
 		hub = usb_hub_to_struct_hub(udev->parent);
@@ -2362,6 +2352,16 @@ void usb_disconnect(struct usb_device **pdev)
 		typec_deattach(port_dev->connector, &udev->dev);
 	}
 
+	hub_disconnect_children(udev);
+
+	/* deallocate hcd/hardware state ... nuking all pending urbs and
+	 * cleaning up all state associated with the current configuration
+	 * so that the hardware is now fully quiesced.
+	 */
+	dev_dbg(&udev->dev, "unregistering device\n");
+	usb_disable_device(udev, 0);
+	usb_hcd_synchronize_unlinks(udev);
+
 	usb_remove_ep_devs(&udev->ep0);
 	usb_unlock_device(udev);
 
-- 
2.50.1