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.huang@xxxxxxxxx
> > > ---
> > > 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
