On Sun, Sep 23, 2012 at 12:01 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Sat, 22 Sep 2012, Ming Lei wrote:
>
>> This patch sets hub device's default autosuspend delay as 0 to
>> speedup bus suspend, see comments in code for details.
>>
>> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxxxxx>
>> ---
>> drivers/usb/core/hub.c | 38 ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 38 insertions(+)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 97d6036..9bde1f5 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -1629,6 +1629,44 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
>> desc = intf->cur_altsetting;
>> hdev = interface_to_usbdev(intf);
>>
>> + /*
>> + * Set default autosuspend delay as 0 to speedup bus suspend.
>> + * Based on the below considerations:
>
> The following comments seem excessively long.
Sorry for that because the patch might be thought as a bit
aggressive and I just try my best to explain in detail that
it is safe and valid.
>
>> + *
>> + * - The remote wakeup from child device and new connected
>> + * device doesn't mean that there might be another new device
>> + * which is going to be connected or disconnected to the hub,
>> + * or there might be another wakeup from other devices, in
>> + * the following seconds.
>
> "remote wakeup from child device" is irrelevant, because the hub will
> be prevented from suspending as long as the child is awake.
I mean it is just one of wakeup sources.
>
> In fact, sometimes new connected device _does_ mean there will be
> another new connection in the next few seconds. This happens because
Looks I should have made it clearer, and I mean the physical device
connection or disconnection, eg, device plugging and unplugging.
> some devices have trouble initializing when they are first plugged in.
> They disconnect themselves from the bus and then reconnect a second or
> so later.
First, the situation should be less like to happen.
Secondly, the patch may only affect the case when the new
connected device is a hub.
Finally, the patch might not affect it if the later reconnect is just
logical disconnection and connection. If the device triggers
physical disconnect/connect in bus(such as, D+ changes) during
its initializing, the patch just makes one extra suspend/resume
on the hub as did in plugging and unplugging device.
>
>> + *
>> + * - If the hub device is accessed from user space context
>> + * (sysfs or libusb), it will be woke up first, so utilities
>> + * like 'lsusb' will work fine.
>
> This is true no matter what autosuspend delay we use. There's no point
> mentioning it.
OK, will remove it.
>
>> + *
>> + * - The wakeup event can be handled completely in hub_resume()
>> + * and khubd, and the submitted status URB is just to check
>> + * future changes on hub downstream ports.
>
> I'm not sure what you mean here. You seem to be saying that the hub
> driver is smart enough not to allow another autosuspend until the
> wakeup event has been processed. This is true no matter how long or
> short the autosuspend delay is.
For some drivers, the remote wakeup is handled by submitting new
URBs(such as HID, to read the key pressed) in its resume callback, so
changing autosuspend delay as 0 doesn't work, but that is not ture for
hub, so I mentioned that "the submitted status URB is just to check
future changes on hub downstream ports.", and the submitted URB
in resume() can be killed in latter auto suspend without any effect on
handling the current remote wakeup.
IMO, this point is important wrt. the change in this patch.
>
>> + *
>> + * - the introduced hub resume delay during the autosuspend
>> + * delay period doesn't matter for new device to be connected
>> + * into the hub.
>
> I don't understand. What is the hub resume delay? Is this any
> different for a 0-second autosuspend vs. a 2-second autosuspend delay?
I mean that 2-second autosuspend delay may delay the autosuspend of
the parent device. Looks it isn't needed.
>> + *
>> + * - the child device should depend its own autosuspend delay
>> + * to fit its use case and shoudn't depend on the delay of
>> + * parent hub device.
>
> Again, this is true no matter what autosuspend delay the hub uses.
OK, will remove it.
>
>> + *
>> + * - suppose there is one device which will remote wakeup
>> + * every 2 seconds periodically and does't care the resume
>> + * delay, the bus can't be suspended with previous default
>> + * autosuspend delay at all, not mention in case of several
>> + * hubs connected in the path from the device to root hub.
>
> This is true; however it is misleading. What matters is the total
> energy usage. Does it take more energy to suspend and resume a hub
> every 2 seconds or simply to leave it active the whole time?
In theory, the total energy should be decreased under the situation
since the whole bus might be in suspend state at most of times(maybe
more than 90%) in the situation. According to '7.2.3 Power Control
During Suspend/Resume' of USB 2.0 spec:
The average current cannot exceed the average suspend current
limit (ICCSH or ICCSL, see Table 7-7) during any 1.0-second interval.
>
> Also, what reason is there to think that some device will wake up every
> 2 seconds periodically?
I want to explain the patch may cause energy consumption decrease.
>
>> + *
>> + * - it is safe to do so because the hub can't be auto-suspended
>> + * until the remote wakeup is handled by khubd in the device
>> + * remote wakeup path.
>
> Isn't this the same as the third reason above?
Yes, will remove it.
>
>> + */
>> + pm_runtime_set_autosuspend_delay(&hdev->dev, 0);
>
> Did you consider restoring the autosuspend delay back to the default
> when the hub driver unbinds?
Looks not necessary to do it.
If the hub driver is unbound, all usb devices connected to the hub
will be disconnected and remote-wakeup won't be enabled, so
it is OK to let the hub suspend immediately.
IMO, maybe the autosuspend_delay of all USB devices should
be set as zero after they are unbound since they won't be used
before being bound again.
Thanks,
--
Ming Lei
--
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
