linux-kernel - Re: USB-C device hotplug issue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.44L0.1810261000040.1467-100000@iolanthe.rowland.org>
Date:   Fri, 26 Oct 2018 10:07:52 -0400 (EDT)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Dennis Wassenberg <dennis.wassenberg@...unet.com>
cc:     Mathias Nyman <mathias.nyman@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Ravi Chandra Sadineni <ravisadineni@...omium.org>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Bin Liu <b-liu@...com>,
        Maxim Moseychuk <franchesko.salias.hudro.pedros@...il.com>,
        Mike Looijmans <mike.looijmans@...ic.nl>,
        Dominik Bozek <dominikx.bozek@...el.com>,
        USB list <linux-usb@...r.kernel.org>,
        Kernel development list <linux-kernel@...r.kernel.org>
Subject: Re: USB-C device hotplug issue

On Fri, 26 Oct 2018, Dennis Wassenberg wrote:

> >> --- a/drivers/usb/core/hub.c
> >> +++ b/drivers/usb/core/hub.c
> >> @@ -2815,7 +2815,9 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
> >>  					USB_PORT_FEAT_C_BH_PORT_RESET);
> >>  			usb_clear_port_feature(hub->hdev, port1,
> >>  					USB_PORT_FEAT_C_PORT_LINK_STATE);
> >> -			usb_clear_port_feature(hub->hdev, port1,
> >> +
> >> +			if (!warm)
> >> +				usb_clear_port_feature(hub->hdev, port1,
> >>  					USB_PORT_FEAT_C_CONNECTION);
> >>  
> >>  			/*
> > 
> > The key fact is that connection events can get lost if they happen to 
> > occur during a port reset.
> Yes.
> > 
> > I'm not entirely certain of the logic here, but it looks like the
> > correct test to add should be "if (udev != NULL)", not "if (!warm)".  
> > Perhaps Mathias can confirm this
> I don't know if clearing the USB_PORT_FEAT_C_CONNECTION bit is
> correct in case of a non warm reset. In my case I always observed a
> warm reset because of the link state change.
> Thats why I checked the warm variable to not change the behavoir for
> cases a didn't checked for the first shot.

(The syntax of that last sentence is pretty mangled; I can't understand 
it.)

Think of it this way:

	If a device was not attached before the reset, we don't want
	to clear the connect-change status because then we wouldn't
	recognize a device that was plugged in during the reset.

	If a device was attached before the reset, we don't want any
	connect-change status which might be provoked by the reset to
	last, because we don't want the core to think that the device
	was unplugged and replugged when all that happened was a reset.

So the important criterion is whether or not a device was attached to 
the port when the reset started.  It's something of a coincidence that 
you only observe warm resets when there's nothing attached.

> During the first run of usb_hub_reset the udev is NULL because in
> this situation the device is not attached logically.
> 
> [  112.889810] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: 0x40!
> [  113.201192] usb 4-1-port1: XXX: hub_port_reset: udev:            (nil)!
> [  113.201198] usb 4-1-port1: XXX: hub_port_reset (not clearing USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1!
> [  113.253612] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x1!
> [  113.377208] usb 4-1-port1: XXX: hub_port_reset: udev: ffff88046b302800!
> [  113.377214] usb 4-1-port1: XXX: hub_port_reset (not clearing USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0!
> [  113.429478] usb 4-1.1: new SuperSpeed USB device number 7 using xhci_hcd
> [  113.442370] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5596
> [  113.442376] usb 4-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> [  113.442381] usb 4-1.1: Product: Ultra T C 
> [  113.442385] usb 4-1.1: Manufacturer: SanDisk
> [  113.442388] usb 4-1.1: SerialNumber: 4C530001131013121031
> 
> Or maybe we can skip clearing the USB_PORT_FEAT_C_CONNECTION bit in
> case of hub_port_reset completely without any other check?

See above.

Alan Stern