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.
> >> @@ -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?
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?
> >> @@ -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.
> >> @@ -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.
> 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.
> >> @@ -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.
> > 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.
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
--
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
