On Thu, 14 Jul 2016, Huang, Huki wrote:
> On Mon, 11 Jul 2016: Alan Stern wrote:
>
> > On Mon, 11 Jul 2016, Huang, Huki wrote:
>
> > > When end user inserts a usb3 device and put dut to s3.
>
> > What does "dut" mean?
>
> DUT : Device under test
Don't call it that in the patch description. "Device" here means "USB
device", whereas you mean "USB host system".
> > > Then hotplug the usb3 disk under s3.
>
> > Do you mean that the device is unplugged and the USB disk then is plugged
> > into the same port? Or do you mean that the device remains plugged in and
> > the disk is plugged into a different port?
>
> Yes, the USB disk is unplugged and plugged on the same port when the system
> under s3
Is the "usb3 device" you mentioned above the same as the "usb3 disk"?
If they are the same, why do you use two different terms for them?
> > > The device will be lost upon resuming from s3.
>
> > If the device is unplugged then it _should_ be lost.
>
> > > There is a corner case that the hub->change_bits for the usb3 port
> > > is not set when usb port change event happens.
>
> > Under what conditions does this corner case occur?
>
> The USB disk is unplugged and plugged on the same port under s3.
>
> > > This will cause hub driver ignore the device enumeration in the end
> > > of port_event in hub.c
> > >
> > > Signed-off-by: huki.hu...@intel.com
> > > ---
> > > drivers/usb/core/hub.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index
> > > bee1351..859adcb 100644
> > > --- a/drivers/usb/core/hub.c
> > > +++ b/drivers/usb/core/hub.c
> > > @@ -4747,6 +4747,7 @@ static void hub_port_connect(struct usb_hub
> > >*hub, int port1, u16 portstatus,
> > > if (hcd->usb_phy && !hdev->parent)
> > >
> > >usb_phy_notify_disconnect(hcd->usb_phy, udev->speed);
> > > usb_disconnect(&port_dev->child);
> > > + set_bit(port1, hub->change_bits);
>
> > Why is this needed? The only reason for setting hub->change_bits is
> > to tell hub_event() that it needs to call port_event() and to tell
> > port_event() that it needs to call hub_port_connect_change(). Once
> > hub_port_connect_change() is running, there is no reason to set
> > hub->change_bits.
>
> When the USB disk is unplugged and plugged on the same port under s3.
> You can see a series of disconnect/connect events happen.
> There is a chance driver receives a port_event but not enumerate device due
> to change_bits is not set.
>
> When usb_disconnect is called, it doesn't set_bit(port1,
> hub->change_bits); Then port_event is called again very soon after
> the above usb_disconnect is called.
>
> In the end of the port_event function.
> The connect_change is not true so that device is not enumerated.
> if (connect_change)
> hub_port_connect_change(hub, port1, portstatus, portchange);
I think what you need to do is change the code in hub_activate().
That's where we check for changes to a port's status that occurred
while the system was suspended. In particular, pay attention to the
code following the "init2:" label.
However, I think the following part of that function:
} else if (portstatus & USB_PORT_STAT_ENABLE) {
bool port_resumed = (portstatus &
USB_PORT_STAT_LINK_STATE) ==
USB_SS_PORT_LS_U0;
/* The power session apparently survived the resume.
* If there was an overcurrent or suspend change
* (i.e., remote wakeup request), have hub_wq
* take care of it. Look at the port link state
* for USB 3.0 hubs, since they don't have a suspend
* change bit, and they don't set the port link change
* bit on device-initiated resume.
*/
if (portchange || (hub_is_superspeed(hub->hdev) &&
port_resumed))
set_bit(port1, hub->change_bits);
} else if (udev->persist_enabled) {
#ifdef CONFIG_PM
udev->reset_resume = 1;
#endif
/* Don't set the change_bits when the device
* was powered off.
*/
if (test_bit(port1, hub->power_bits))
set_bit(port1, hub->change_bits);
should already be doing what you want. If it isn't, you need to figure
out why.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
