On Fri, 9 Nov 2012, Lan Tianyu wrote:
> This patch is to add usb port auto power off mechanism.
> When usb device is suspending, usb core will send clear usb port's
> POWER feature requst to power off port if all conditions were met.
> These conditions are remote wakeup enable, pm qos NO_POWER_OFF flags
> and persist feature. When device is suspended and power off, usb core
You got the conditions wrong. You need remote wakeup to be
disabled, NO_POWER_OFF clear, and persist enabled.
> will ignore port's connect and enable change event to keep the device
> not being disconnected. When it resumes, power on port again.
> @@ -47,6 +48,7 @@ struct usb_port {
> struct device dev;
> struct dev_state *port_owner;
> enum usb_port_connect_type connect_type;
> + unsigned power_state:1;
What's with the peculiar spacing?
> };
>
> struct usb_hub {
> @@ -166,6 +168,11 @@ MODULE_PARM_DESC(use_both_schemes,
> DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
> EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
>
> +#define USB_PORT_POWER_STATE_ON 0
> +#define USB_PORT_POWER_STATE_OFF 1
Just call the new field power_on (and make it bool rather than
unsigned). Then these symbols won't be needed.
> @@ -2970,6 +2984,23 @@ 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)
Why write it this complicated way? Just use !=.
> + && udev->persist_enabled
> + && !status) {
> + port_dev->power_state = USB_PORT_POWER_STATE_OFF;
> + if (clear_port_feature(udev->parent, port1,
> + USB_PORT_FEAT_POWER)) {
> + port_dev->power_state = USB_PORT_POWER_STATE_ON;
Do this the other way around: If clear_port_feature() succeeds, clear
port_dev->power_on.
> @@ -3094,6 +3144,25 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
> int status;
> u16 portchange, portstatus;
>
> +
> + set_bit(port1, hub->busy_bits);
> +
> + if (hub->ports[port1 - 1]->power_state == USB_PORT_POWER_STATE_OFF) {
> + set_port_feature(udev->parent, port1, USB_PORT_FEAT_POWER);
At this point you should set the port's power_on flag.
> +
> + /*
> + * Wait for usb hub port to be reconnected in order to make
> + * the resume procedure successful.
> + */
> + 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);
> + return status;
> + }
> + hub->ports[port1 - 1]->power_state = USB_PORT_POWER_STATE_ON;
Not here. The way you've got it, the power_on flag will be wrong if
usb_port_wait_for_connected fails.
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
