On 2012年12月12日 00:35, Alan Stern wrote:
> On Tue, 11 Dec 2012, Lan Tianyu wrote:
>
>>>> @@ -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.
>
> Then a better solution would be to first wait (up to 2 seconds) for a
> connect status and then call the debounce routine.
But some devices don't need to wait 2s such long time, 200ms is enough
for them. So I try to check status everytime after debounce. If it's
connected, go away.
>
>>>> @@ -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.
>
> You did not understand my question. When usb_hub_set_port_power is
> called, won't the Set-Feature request almost always be necessary?
>
Oh. Sorry. Thanks for reminders. :)
> For example, how often will it happen that set and
> port_dev->power_is_on are both true? Or both false? It seems to me
> that this will almost never happen. So why bother testing for it?
Ok. I get. You are right and will remove the check.
>
>>>> @@ -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"?
>
> What you need isn't a resume; it's pm_runtime_get_sync.
How about "needs_runtime_get"?
>
>>>> @@ -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?
>
> Yes.
>
>>>> + 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.
>
> You are confusing pm_runtime_get_sync with resume. They are not the
> same thing. You always need to call pm_runtime_get_sync here if
> pm_runtime_put_sync was called earlier, even if the port is already
> powered on.
Yeah.
>
>> 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;
>> }
>>
>> }
>
> Actually, it looks like you will want to call wait_for_connected in the
> port's runtime-resume routine. After all, if the user changes the PM
> QOS flag while the port is powered off, you will need to start paying
> attention to the connect status.
Yeah. This make sense.
>
>>>> @@ -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.
>
> This means you should set busy_bits in the port's runtime-resume
> routine and keep it set until wait_for_connected is finished.
Yeah. I have done some test. It works.
>
>>> 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?
>
> "should be" -- but buggy hardware might send an event anyway. Also,
> many root hubs don't pay attention to the power feature. If this
> happens, you probably should handle the connect change properly. I
> don't see any point in ignoring it.
How to deal with these connect change event or how to identify whether
the connect change event is trigger by real power off or not?
>
> One other thing to watch out for: If the device is unplugged while it
> is suspended, you may need to call pm_runtime_get_sync from somewhere
> in the device-removal path.
>
> 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
