linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alan Stern <stern@rowland.harvard.edu>
To: Dennis Wassenberg <dennis.wassenberg@secunet.com>
Cc: Mathias Nyman <mathias.nyman@intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Ravi Chandra Sadineni <ravisadineni@chromium.org>,
	Kuppuswamy Sathyanarayanan 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	Bin Liu <b-liu@ti.com>,
	Maxim Moseychuk <franchesko.salias.hudro.pedros@gmail.com>,
	Mike Looijmans <mike.looijmans@topic.nl>,
	Dominik Bozek <dominikx.bozek@intel.com>,
	USB list <linux-usb@vger.kernel.org>,
	Kernel development list <linux-kernel@vger.kernel.org>
Subject: Re: USB-C device hotplug issue
Date: Fri, 26 Oct 2018 10:07:52 -0400 (EDT)	[thread overview]
Message-ID: <Pine.LNX.4.44L0.1810261000040.1467-100000@iolanthe.rowland.org> (raw)
In-Reply-To: <8e750341-8fe5-9c66-0653-3c92d4a60505@secunet.com>

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


  reply	other threads:[~2018-10-26 14:07 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-25 12:20 Dennis Wassenberg
2018-10-25 12:28 ` Greg Kroah-Hartman
2018-10-25 12:38   ` Dennis Wassenberg
2018-10-25 14:46 ` Alan Stern
2018-10-26  9:44   ` Dennis Wassenberg
2018-10-26 14:07     ` Alan Stern [this message]
2018-11-05 15:35       ` Mathias Nyman
2018-11-07  9:08         ` Dennis Wassenberg
2018-11-09 13:47           ` Mathias Nyman
2018-11-13 13:38             ` Dennis Wassenberg
2018-11-13 13:40             ` [PATCH] usb: core: Fix hub port connection events lost Dennis Wassenberg
2018-11-13 13:55               ` Mathias Nyman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Pine.LNX.4.44L0.1810261000040.1467-100000@iolanthe.rowland.org \
    --to=stern@rowland.harvard.edu \
    --cc=b-liu@ti.com \
    --cc=dennis.wassenberg@secunet.com \
    --cc=dominikx.bozek@intel.com \
    --cc=franchesko.salias.hudro.pedros@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mathias.nyman@intel.com \
    --cc=mike.looijmans@topic.nl \
    --cc=ravisadineni@chromium.org \
    --cc=sathyanarayanan.kuppuswamy@linux.intel.com \
    --subject='Re: USB-C device hotplug issue' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).