On 2012年12月11日 00:53, Alan Stern wrote:
> On Mon, 10 Dec 2012, Lan Tianyu wrote:
>
>> Hi Alan:
>> I write a patch based on the needs_debounce flag. The flag will be set
>> when port has child device and power on successfully. Otherwise, I
>> separate resume port and wait for connect in the usb_port_resume().
>> Device will not be disconnected when power_is_on is false or
>> needs_debounce is set. Welcome comments.
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 86e595c..aa41d3a 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>
>> @@ -108,11 +109,14 @@ MODULE_PARM_DESC(use_both_schemes,
>> DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
>> EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
>>
>> +#define HUB_PORT_RECONNECT_TRIES 20
>
> 20 is an awful lot. Do you really need any more than one try? The
> debounce routine already does its own looping.
I tested a usb ssd which consumes about 1s to makes usb port status
enter into connect status after powering off and powering on the port.
So I set the tries 20 and the longest latency is larger than 2s.
The debounce routine only guarantees port status stable rather than
enter into connect status.
>
>> @@ -718,13 +722,26 @@ int usb_hub_set_port_power(struct usb_device
>> *hdev, int port1,
>> bool set)
>> {
>> int ret;
>> + struct usb_hub *hub = hdev_to_hub(hdev);
>> + struct usb_port *port_dev
>> + = hub->ports[port1 - 1];
>> +
>> + if (set) {
>> + if (port_dev->power_is_on)
>> + return 0;
>>
>> - if (set)
>> ret = set_port_feature(hdev, port1,
>> USB_PORT_FEAT_POWER);
>> - else
>> + } else {
>> + if (!port_dev->power_is_on)
>> + return 0;
>> +
>> ret = clear_port_feature(hdev, port1,
>> USB_PORT_FEAT_POWER);
>> + }
>
> Do we need these shortcuts? How often will this routine be called when
> the port is already in the right state
When the PM Qos NO_POWER_OFF is changed,
pm_runtime_get_sync/put(port_dev) will be invoked. This routine will be
called during port resume and suspend. If one device was suspended and
power off, the port's usage_count would be 0. After then, the port will
be resumed and suspend every time pm qos NO_POWER_OFF flag being
changed. So this depends on the user space.
>
>> @@ -2862,6 +2884,18 @@ int usb_port_suspend(struct usb_device *udev,
>> pm_message_t msg)
>> udev->port_is_suspended = 1;
>> msleep(10);
>> }
>> +
>> + /*
>> + * Check whether current status is meeting requirement of
>> + * usb port power off mechanism
>> + */
>> + if (!udev->do_remote_wakeup
>> + && (dev_pm_qos_flags(&port_dev->dev,
>> + PM_QOS_FLAG_NO_POWER_OFF) != PM_QOS_FLAGS_ALL)
>> + && udev->persist_enabled
>> + && !status)
>> + pm_runtime_put_sync(&port_dev->dev);
>
> You need to store somewhere the fact that you made this call, so that
> you will know whether or not to make the corresponding
> pm_runtime_get_sync call in usb_port_resume.
You mean I should add a new flag to keep the
pm_runtime_put_sync/put(port_dev) being called paired, right?
How about "needs_resume"?
>
>> @@ -2982,10 +3035,39 @@ static int finish_port_resume(struct usb_device
>> *udev)
>> int usb_port_resume(struct usb_device *udev, pm_message_t msg)
>> {
>> struct usb_hub *hub = hdev_to_hub(udev->parent);
>> + struct usb_port *port_dev = hub->ports[udev->portnum - 1];
>> int port1 = udev->portnum;
>> int status;
>> u16 portchange, portstatus;
>>
>> +
>> + set_bit(port1, hub->busy_bits);
>> +
>> + if (!port_dev->power_is_on) {
>
> This test is wrong. It's possible that the port power is still on even
> though you called pm_runtime_put_sync.
Above, we need to check the new flag, right?
>
>> + status = pm_runtime_get_sync(&port_dev->dev);
>> + if (status < 0) {
>> + dev_dbg(&udev->dev, "can't resume usb port, status
>> %d\n",
>> + status);
>> + clear_bit(port1, hub->busy_bits);
>> + return status;
>> + }
>
> Don't you want to call usb_port_wait_for_connected right here?
>
>> + }
>> +
>> +
>> + /*
>> + * Wait for usb hub port to be reconnected in order to make
>> + * the resume procedure successful.
>> + */
>> + if (port_dev->needs_debounce) {
>> + status = usb_port_wait_for_connected(hub, port1);
>
> If you move this call earlier then you won't have to test
> needs_debounce.
The port may have been power on when device is resumed. One case, after
device being suspended and power off, user may set the NO_POWER_OFF flag
and port will be power on before device being resumed. For this case,
port doesn't need to be resumed in the usb_port_resume() since port
already power on and "wait for connected" is enough. So I divide resume
port and wait for connected into two pieces. But from your rely, I
realize we should paired call pm_runtime_get_sync/put(port_dev). Now I
think this should be put earlier.
Something like this
if(port_dev->needs_resume) {
status = pm_runtime_get_sync(&port_dev->dev);
if (status < 0) {
dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
status);
clear_bit(port1, hub->busy_bits);
return status;
}
status = usb_port_wait_for_connected(hub, port1);
if (status < 0) {
dev_dbg(&udev->dev, "can't get reconnection after setting port
power
on, status %d\n", status);
clear_bit(port1, hub->busy_bits);
return status;
}
}
>
>> @@ -4175,6 +4256,11 @@ static void hub_port_connect_change(struct
>> usb_hub *hub, int port1,
>> }
>> }
>>
>> + if (!port_dev->power_is_on || port_dev->needs_debounce) {
>> + clear_bit(port1, hub->change_bits);
>> + return;
>> + }
>
> Doesn't the busy_bits flag take care of this for you already?
When port is power off or power on during PM Qos NO_POWER_OFF flag
changing , there is also an connect change event and busy_bits is not
set at that time.
>
> Also, what if the port is ganged, so even though you think you turned
> off the power, it really is still on? When that happens you probably
> don't want to ignore port events.
Even the power is still on but the PORT_POWER feature has been cleared.
So there should be no event from port, right?
>
> Alan Stern
>
--
Best regards
Tianyu Lan
--
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
