On Mon, 2014-03-03 at 16:18 -0500, Alan Stern wrote:
> On Fri, 28 Feb 2014, Dan Williams wrote:
>
> > A hub indicates whether it supports per-port power control via the
> > wHubCharacteristics field in its descriptor. If it is not supported
> > a hub will still emulate ClearPortPower(PORT_POWER) requests by
> > stopping the link state machine. However, since this does not save
> > power do not bother suspending.
> >
> > This also consolidates support checks into a
> > hub_is_port_power_switchable() helper.
> >
> > Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
>
> This patch looks pretty good. I would change only a comment:
>
> > --- a/drivers/usb/core/port.c
> > +++ b/drivers/usb/core/port.c
> > @@ -171,12 +171,14 @@ int usb_hub_create_port_device(struct usb_hub *hub,
> > int port1)
> >
> > pm_runtime_set_active(&port_dev->dev);
> >
> > - /* It would be dangerous if user space couldn't
> > - * prevent usb device from being powered off. So don't
> > - * enable port runtime pm if failed to expose port's pm qos.
> > + /* It would be dangerous if user space couldn't prevent usb
>
> The comment style should be fixed:
>
> /*
> * It would...
>
> > + * device from being powered off. So don't enable port runtime
> > + * pm if failed to expose port's pm qos, or if the hub does not
> > + * support power switching
>
> "if failed" lacks a subject. And the final sentence lacks a period.
>
> > */
> > - if (!dev_pm_qos_expose_flags(&port_dev->dev,
> > - PM_QOS_FLAG_NO_POWER_OFF))
> > + if (hub_is_port_power_switchable(hub)
> > + && dev_pm_qos_expose_flags(&port_dev->dev,
> > + PM_QOS_FLAG_NO_POWER_OFF) == 0)
> > pm_runtime_enable(&port_dev->dev);
>
> The order of the tests described in the comment should match the order
> of the tests in the code.
Yeah, I just tacked on to the existing comment. Rewritten properly now.
>
> Aside from these things,
>
> Acked-by: Alan Stern <st...@rowland.harvard.edu>
>
Thanks.
8<----------
Subject: usb: disable port power control if not supported in wHubCharacteristics
From: Dan Williams <dan.j.willi...@intel.com>
A hub indicates whether it supports per-port power control via the
wHubCharacteristics field in its descriptor. If it is not supported
a hub will still emulate ClearPortPower(PORT_POWER) requests by
stopping the link state machine. However, since this does not save
power do not bother suspending.
This also consolidates support checks into a
hub_is_port_power_switchable() helper.
Acked-by: Alan Stern <st...@rowland.harvard.edu>
Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
---
drivers/usb/core/hub.c | 8 ++------
drivers/usb/core/hub.h | 10 ++++++++++
drivers/usb/core/port.c | 13 ++++++++-----
3 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 519f2c3594b2..4e967f613e70 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -810,8 +810,6 @@ static unsigned hub_power_on(struct usb_hub *hub, bool
do_delay)
int port1;
unsigned pgood_delay = hub->descriptor->bPwrOn2PwrGood * 2;
unsigned delay;
- u16 wHubCharacteristics =
- le16_to_cpu(hub->descriptor->wHubCharacteristics);
/* Enable power on each port. Some hubs have reserved values
* of LPSM (> 2) in their descriptors, even though they are
@@ -819,7 +817,7 @@ static unsigned hub_power_on(struct usb_hub *hub, bool
do_delay)
* but only emulate it. In all cases, the ports won't work
* unless we send these messages to the hub.
*/
- if ((wHubCharacteristics & HUB_CHAR_LPSM) < 2)
+ if (hub_is_port_power_switchable(hub))
dev_dbg(hub->intfdev, "enabling power on all ports\n");
else
dev_dbg(hub->intfdev, "trying to enable port power on "
@@ -4383,8 +4381,6 @@ static void hub_port_connect_change(struct usb_hub *hub,
int port1,
struct usb_device *hdev = hub->hdev;
struct device *hub_dev = hub->intfdev;
struct usb_hcd *hcd = bus_to_hcd(hdev->bus);
- unsigned wHubCharacteristics =
- le16_to_cpu(hub->descriptor->wHubCharacteristics);
struct usb_device *udev;
int status, i;
unsigned unit_load;
@@ -4469,7 +4465,7 @@ static void hub_port_connect_change(struct usb_hub *hub,
int port1,
test_bit(port1, hub->removed_bits)) {
/* maybe switch power back on (e.g. root hub was reset) */
- if ((wHubCharacteristics & HUB_CHAR_LPSM) < 2
+ if (hub_is_port_power_switchable(hub)
&& !port_is_power_on(hub, portstatus))
set_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index df629a310e44..baf5b48b79f7 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -111,6 +111,16 @@ extern int hub_port_debounce(struct usb_hub *hub, int
port1,
extern int usb_clear_port_feature(struct usb_device *hdev,
int port1, int feature);
+static inline bool hub_is_port_power_switchable(struct usb_hub *hub)
+{
+ __le16 hcs;
+
+ if (!hub)
+ return false;
+ hcs = hub->descriptor->wHubCharacteristics;
+ return (le16_to_cpu(hcs) & HUB_CHAR_LPSM) < HUB_CHAR_NO_LPSM;
+}
+
static inline int hub_port_debounce_be_connected(struct usb_hub *hub,
int port1)
{
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 51542f852393..3a7dd99706aa 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -171,12 +171,15 @@ int usb_hub_create_port_device(struct usb_hub *hub, int
port1)
pm_runtime_set_active(&port_dev->dev);
- /* It would be dangerous if user space couldn't
- * prevent usb device from being powered off. So don't
- * enable port runtime pm if failed to expose port's pm qos.
+ /*
+ * Do not enable port runtime pm if the hub does not support
+ * power switching. Also, userspace must have final say of
+ * whether a port is permitted to power-off. Do not enable
+ * runtime pm if we fail to expose pm_qos_no_power_off.
*/
- if (!dev_pm_qos_expose_flags(&port_dev->dev,
- PM_QOS_FLAG_NO_POWER_OFF))
+ if (hub_is_port_power_switchable(hub)
+ && dev_pm_qos_expose_flags(&port_dev->dev,
+ PM_QOS_FLAG_NO_POWER_OFF) == 0)
pm_runtime_enable(&port_dev->dev);
device_enable_async_suspend(&port_dev->dev);
--
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
