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