[PATCH 0/3] USB/ACPI: Add usb port power off mechanism

[PATCH 0/3] USB/ACPI: Add usb port power off mechanism

[Lan Tianyu]

"portX/control" has two options
"on" - port power must be on.
"off" - port power must be off.

"portX/state" reports usb port's power state
"on" - power on
"off" - power off
"error" - can't get power state

[PATCH 1/3] xhci: Add clear PORT_POWER feature process in the
[PATCH 2/3] USB/ACPI: Add usb port's acpi power control in the xhci
[PATCH 3/3] usb : Add sysfs files to control usb port's power

This patchset is based on my previous patchset.
[PATCH V3 1/4] usb: add struct usb_hub_port to store
[PATCH V3 2/4] usb: move struct usb_device->children
[PATCH V3 3/4] usb/acpi: add the support of usb hub
[PATCH V3 4/4] usb/acpi: add usb check for the connection

git diff --stat
Documentation/ABI/testing/sysfs-bus-usb |   33 ++++++++++++++++++++++++++++++++
drivers/usb/core/hub.c                  |  168 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
drivers/usb/core/usb-acpi.c             |   28 ++++++++++++++++++++++++++++
drivers/usb/host/xhci-hub.c             |   14 ++++++++++++++
include/linux/usb.h                     |   10 ++++++++++
5 files changed, 252 insertions(+), 1 deletion(-)

[PATCH 1/3] xhci: Add clear PORT_POWER feature process in the xhci_hub_control()

[Lan Tianyu]

This patch is to add process of clearing PORT_POWER feature request
for xhci hub.

Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/host/xhci-hub.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 2732ef6..2c55fcf 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -827,6 +827,10 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			xhci_disable_port(hcd, xhci, wIndex,
 					port_array[wIndex], temp);
 			break;
+		case USB_PORT_FEAT_POWER:
+			xhci_writel(xhci, temp & ~PORT_POWER,
+				port_array[wIndex]);
+			break;
 		default:
 			goto error;
 		}
-- 
1.7.6.rc2.8.g28eb

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] USB/ACPI: Add usb port's acpi power control in the xhci PORT_POWER feature request process.

[Lan Tianyu]

On our developping machine, bios can provide usb port's  power control via
acpi. This patch is to provide usb port's power control way through setting
or clearing PORT_POWER feature requests. Add two functions usb_acpi_power_manageable()
and usb_acpi_set_power_state(). The first one is used to find whether the
usb port has acpi power resource and the second is to set the power state.
They are invoked in the xhci_hub_control() where clearing or setting PORT_POWER
feature requests are processed.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/usb/core/usb-acpi.c |   28 ++++++++++++++++++++++++++++
 drivers/usb/host/xhci-hub.c |   10 ++++++++++
 include/linux/usb.h         |   10 ++++++++++
 3 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 82c90d0..e95f26f 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -19,6 +19,34 @@
 
 #include "usb.h"
 
+bool usb_acpi_power_manageable(struct usb_device *hdev, int port1)
+{
+	acpi_handle port_handle;
+
+	port_handle = usb_get_hub_port_acpi_handle(hdev,
+		port1);
+	return port_handle ? acpi_bus_power_manageable(port_handle) : false;
+}
+EXPORT_SYMBOL_GPL(usb_acpi_power_manageable);
+
+int usb_acpi_set_power_state(struct usb_device *hdev, int port1, bool enable)
+{
+	acpi_handle port_handle;
+	unsigned char state;
+	int error = -EINVAL;
+
+	port_handle = (acpi_handle)usb_get_hub_port_acpi_handle(hdev,
+		port1);
+	state = enable ? ACPI_STATE_D0 : ACPI_STATE_D3_COLD;
+	error = acpi_bus_set_power(port_handle, state);
+	if (!error)
+		dev_dbg(&hdev->dev, "The power of hub port %d was set to %s\n",
+			port1, enable ? "enable" : "disabe");
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(usb_acpi_set_power_state);
+
 static int usb_acpi_check_port_connect_type(struct usb_device *hdev,
 	acpi_handle handle, int port1)
 {
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 2c55fcf..0ce48b3 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -728,6 +728,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 
 			temp = xhci_readl(xhci, port_array[wIndex]);
 			xhci_dbg(xhci, "set port power, actual port %d status  = 0x%x\n", wIndex, temp);
+
+			if (usb_acpi_power_manageable(hcd->self.root_hub,
+					wIndex + 1))
+				usb_acpi_set_power_state(hcd->self.root_hub,
+					wIndex + 1, true);
 			break;
 		case USB_PORT_FEAT_RESET:
 			temp = (temp | PORT_RESET);
@@ -830,6 +835,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		case USB_PORT_FEAT_POWER:
 			xhci_writel(xhci, temp & ~PORT_POWER,
 				port_array[wIndex]);
+
+			if (usb_acpi_power_manageable(hcd->self.root_hub,
+					wIndex + 1))
+				usb_acpi_set_power_state(hcd->self.root_hub,
+					wIndex + 1, false);
 			break;
 		default:
 			goto error;
diff --git a/include/linux/usb.h b/include/linux/usb.h
index feb0a04..92f8898 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -599,6 +599,16 @@ extern int usb_lock_device_for_reset(struct usb_device *udev,
 extern int usb_reset_device(struct usb_device *dev);
 extern void usb_queue_reset_device(struct usb_interface *dev);
 
+#ifdef CONFIG_ACPI
+extern int usb_acpi_set_power_state(struct usb_device *hdev, int port,
+	bool enable);
+extern bool usb_acpi_power_manageable(struct usb_device *hdev, int port);
+#else
+static inline int usb_acpi_set_power_state(struct usb_device *hdev, int port,
+	bool enable) { return 0; }
+static inline bool usb_acpi_power_manageable(struct usb_device *hdev, int port)
+	{ return 0; }
+#endif
 
 /* USB autosuspend and autoresume */
 #ifdef CONFIG_USB_SUSPEND
-- 
1.7.6.rc2.8.g28eb

[PATCH 3/3] usb : Add sysfs files to control usb port's power

[Lan Tianyu]

This patch is to add two sys files for each usb hub ports to control port's power.
Control port's power through setting or clearing PORT_POWER feature.

The new sys files is under the usb hub interface sysfs portx directory.
ls /sys/bus/usb/devices/1-0:1.0
bAlternateSetting  bInterfaceClass  bInterfaceNumber  bInterfaceProtocol
bInterfaceSubClass  bNumEndpoints  driver  ep_81  modalias  power
subsystem  supports_autosuspend  uevent port1 port2 port3 port4

ls /sys/bus/usb/devices/1-0:1.0/port1
control state

control has two options. "on" and "off"
"on" - port power must be on.
"off" - port power must be off.

state reports usb port's power state
"on" - power on
"off" - power off
"error" - can't get power state

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-usb |   33 ++++++
 drivers/usb/core/hub.c                  |  168 ++++++++++++++++++++++++++++++-
 2 files changed, 200 insertions(+), 1 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
index 6df4e6f..c2fb9d7 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -208,3 +208,36 @@ Description:
 		such as ACPI. This file will read either "removable" or
 		"fixed" if the information is available, and "unknown"
 		otherwise.
+
+What:		/sys/bus/usb/devices/.../(hub interface)/portX
+Date:		June 2012
+Contact		Lan Tianyu <tianyu.lan@intel.com>
+Description:
+		The /sys/bus/usb/device/.../(hub interface)/portX directory
+		contains attributes allowing user space to check and modify
+		properties of the associated USB port.
+
+What:		/sys/bus/usb/devices/.../(hub interface)/portX/control
+Date:		June 2012
+Contact		Lan Tianyu <tianyu.lan@intel.com>
+Description:
+		The /sys/bus/usb/devices/.../(hub interface)/portX/control
+		attribute allows user space to control the power policy on
+		the usb port.
+
+		All ports have one of the following two values for control
+		"on" - port power must be on.
+		"off" - port power must be off.
+
+What:		/sys/bus/usb/devices/.../(hub interface)/portX/state
+Date:		June 2012
+Contact		Lan Tianyu <tianyu.lan@intel.com>
+Description:
+		The /sys/bus/usb/devices/.../(hub interface)/portX/state
+		attribute allows user space to check hub port's power state.
+
+		All ports have three following states
+		"on"	  -    port power on
+		"off"	  -    port power off
+		"error"	  -    can't get the hub port's power state
+
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 2ed5244..e83b0f5 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -41,12 +41,20 @@
 #endif
 #endif
 
+enum port_power_policy {
+	USB_PORT_POWER_ON = 0,
+	USB_PORT_POWER_OFF,
+};
+
 struct usb_hub_port {
 	void			*port_owner;
 	struct usb_device	*child;
 #ifdef CONFIG_ACPI
 	acpi_handle		port_acpi_handle;
 #endif
+	struct device_attribute	port_control_attr;
+	struct device_attribute	port_state_attr;
+	enum port_power_policy port_power_policy;
 	enum usb_port_connect_type connect_type;
 };
 
@@ -97,6 +105,12 @@ struct usb_hub {
 	struct usb_hub_port	*port_data;
 };
 
+static const char on_string[] = "on";
+static const char off_string[] = "off";
+static char port_name[USB_MAXCHILDREN][8];
+static void usb_hub_port_sysfs_add(struct usb_hub *hub);
+static void usb_hub_port_sysfs_remove(struct usb_hub *hub);
+
 static inline int hub_is_superspeed(struct usb_device *hdev)
 {
 	return (hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS);
@@ -847,7 +861,9 @@ static unsigned hub_power_on(struct usb_hub *hub, bool do_delay)
 		dev_dbg(hub->intfdev, "trying to enable port power on "
 				"non-switchable hub\n");
 	for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++)
-		set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
+		if (hub->port_data[port1 - 1].port_power_policy
+				== USB_PORT_POWER_ON)
+			set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
 
 	/* Wait at least 100 msec for power to become stable */
 	delay = max(pgood_delay, (unsigned) 100);
@@ -1493,6 +1509,7 @@ static int hub_configure(struct usb_hub *hub,
 	if (hub->has_indicators && blinkenlights)
 		hub->indicator [0] = INDICATOR_CYCLE;
 
+	usb_hub_port_sysfs_add(hub);
 	usb_acpi_bind_hub_ports(hdev);
 	hub_activate(hub, HUB_INIT);
 	return 0;
@@ -1531,6 +1548,7 @@ static void hub_disconnect(struct usb_interface *intf)
 	hub->error = 0;
 	hub_quiesce(hub, HUB_DISCONNECT);
 
+	usb_hub_port_sysfs_remove(hub);
 	usb_set_intfdata (intf, NULL);
 	hub->hdev->maxchild = 0;
 
@@ -2558,6 +2576,25 @@ static int port_is_power_on(struct usb_hub *hub, unsigned portstatus)
 	return ret;
 }
 
+static int usb_get_hub_port_power_state(struct usb_device *hdev, int port1)
+{
+	struct usb_hub *hub = hdev_to_hub(hdev);
+	struct usb_port_status data;
+	u16 portstatus;
+	int ret;
+
+	ret = get_port_status(hub->hdev, port1, &data);
+	if (ret < 4) {
+		dev_err(hub->intfdev,
+			"%s failed (err = %d)\n", __func__, ret);
+		if (ret >= 0)
+			ret = -EIO;
+		return ret;
+	} else
+		portstatus = le16_to_cpu(data.wPortStatus);
+	return port_is_power_on(hub, portstatus);
+}
+
 #ifdef	CONFIG_PM
 
 /* Check if a port is suspended(USB2.0 port) or in U3 state(USB3.0 port) */
@@ -4499,6 +4536,134 @@ static int hub_thread(void *__unused)
 	return 0;
 }
 
+static ssize_t show_port_power_state(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	struct usb_device *udev = to_usb_device(dev->parent);
+	struct usb_hub_port *hub_port = container_of(attr,
+		struct usb_hub_port, port_state_attr);
+	struct usb_hub *hub = dev_get_drvdata(dev);
+	int port1 = hub_port - hub->port_data + 1;
+	int power_state;
+	const char *result;
+
+	power_state = usb_get_hub_port_power_state(udev, port1);
+	if (power_state == 1)
+		result = on_string;
+	else if (!power_state)
+		result = off_string;
+	else
+		result = "error";
+	return sprintf(buf, "%s\n", result);
+}
+
+static ssize_t show_port_power_control(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	struct usb_hub_port *hub_port = container_of(attr,
+		struct usb_hub_port, port_control_attr);
+	const char *result;
+
+	switch (hub_port->port_power_policy) {
+	case USB_PORT_POWER_ON:
+		result = on_string;
+		break;
+	case USB_PORT_POWER_OFF:
+		result = off_string;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return sprintf(buf, "%s\n", result);
+}
+
+static ssize_t
+store_port_power_control(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	struct usb_device *udev = to_usb_device(dev->parent);
+	struct usb_hub_port *hub_port = container_of(attr,
+		struct usb_hub_port, port_control_attr);
+	char *cp;
+	struct usb_hub *hub = dev_get_drvdata(dev);
+	int port1 = hub_port - hub->port_data + 1;
+	int len = count;
+
+	cp = memchr(buf, '\n', count);
+	if (cp)
+		len = cp - buf;
+
+	 if (len == sizeof on_string - 1
+		&& strncmp(buf, on_string, len) == 0) {
+		hub_port->port_power_policy = USB_PORT_POWER_ON;
+		set_port_feature(udev, port1, USB_PORT_FEAT_POWER);
+	} else if (len == sizeof off_string - 1
+		&& strncmp(buf, off_string, len) == 0) {
+		hub_port->port_power_policy = USB_PORT_POWER_OFF;
+		clear_port_feature(udev, port1, USB_PORT_FEAT_POWER);
+	} else
+		return -EINVAL;
+
+	return count;
+}
+
+static void usb_hub_port_sysfs_add(struct usb_hub *hub)
+{
+	int i, rc;
+	struct attribute *port_attrs[3];
+	struct attribute_group port_attr_group;
+
+	for (i = 0; i < hub->hdev->maxchild; i++) {
+		hub->port_data[i].port_control_attr = (struct device_attribute)
+			__ATTR(control, S_IRUGO | S_IWUSR,
+			show_port_power_control,
+			store_port_power_control);
+		hub->port_data[i].port_state_attr = (struct device_attribute)
+			__ATTR(state, S_IRUGO, show_port_power_state, NULL);
+
+		port_attrs[0] = &hub->port_data[i].port_control_attr.attr;
+		port_attrs[1] = &hub->port_data[i].port_state_attr.attr;
+		port_attrs[2] = NULL;
+		port_attr_group = (struct attribute_group) {
+			.name = port_name[i],
+			.attrs = port_attrs
+		};
+
+		rc = sysfs_create_group(&hub->intfdev->kobj,
+			&port_attr_group);
+		if (rc < 0)
+			dev_err(hub->intfdev, "can't create sys " \
+				"files for port%d\n", i + 1);
+	}
+}
+
+static void usb_hub_port_sysfs_remove(struct usb_hub *hub)
+{
+	int i;
+	struct attribute *port_attrs[3];
+	struct attribute_group port_attr_group;
+
+	for (i = 0; i < hub->hdev->maxchild; i++) {
+		port_attrs[0] = &hub->port_data[i].port_control_attr.attr;
+		port_attrs[1] = &hub->port_data[i].port_state_attr.attr;
+		port_attrs[2] = NULL;
+		port_attr_group = (struct attribute_group) {
+			.name = port_name[i],
+			.attrs = port_attrs
+		};
+		sysfs_remove_group(&hub->intfdev->kobj,
+			&port_attr_group);
+	}
+}
+
+static void usb_hub_port_sysfs_init(void)
+{
+	int i;
+
+	for (i = 0; i < USB_MAXCHILDREN; i++)
+		sprintf(port_name[i], "port%d", i + 1);
+}
+
 static const struct usb_device_id hub_id_table[] = {
     { .match_flags = USB_DEVICE_ID_MATCH_DEV_CLASS,
       .bDeviceClass = USB_CLASS_HUB},
@@ -4531,6 +4696,7 @@ int usb_hub_init(void)
 		return -1;
 	}
 
+	usb_hub_port_sysfs_init();
 	khubd_task = kthread_run(hub_thread, NULL, "khubd");
 	if (!IS_ERR(khubd_task))
 		return 0;
-- 
1.7.6.rc2.8.g28eb

Re: [PATCH 3/3] usb : Add sysfs files to control usb port's power

[Alan Stern]

On Mon, 11 Jun 2012, Lan Tianyu wrote:

> This patch is to add two sys files for each usb hub ports to control port's power.
> Control port's power through setting or clearing PORT_POWER feature.
> 
> The new sys files is under the usb hub interface sysfs portx directory.
> ls /sys/bus/usb/devices/1-0:1.0
> bAlternateSetting  bInterfaceClass  bInterfaceNumber  bInterfaceProtocol
> bInterfaceSubClass  bNumEndpoints  driver  ep_81  modalias  power
> subsystem  supports_autosuspend  uevent port1 port2 port3 port4
> 
> ls /sys/bus/usb/devices/1-0:1.0/port1
> control state
> 
> control has two options. "on" and "off"
> "on" - port power must be on.
> "off" - port power must be off.
> 
> state reports usb port's power state
> "on" - power on
> "off" - power off
> "error" - can't get power state
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH 2/3] USB/ACPI: Add usb port's acpi power control in the xhci PORT_POWER feature request process.

[Greg KH]

On Mon, Jun 11, 2012 at 10:24:33AM +0800, Lan Tianyu wrote:
> On our developping machine, bios can provide usb port's  power control via
> acpi. This patch is to provide usb port's power control way through setting
> or clearing PORT_POWER feature requests. Add two functions usb_acpi_power_manageable()
> and usb_acpi_set_power_state(). The first one is used to find whether the
> usb port has acpi power resource and the second is to set the power state.
> They are invoked in the xhci_hub_control() where clearing or setting PORT_POWER
> feature requests are processed.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  drivers/usb/core/usb-acpi.c |   28 ++++++++++++++++++++++++++++
>  drivers/usb/host/xhci-hub.c |   10 ++++++++++
>  include/linux/usb.h         |   10 ++++++++++
>  3 files changed, 48 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
> index 82c90d0..e95f26f 100644
> --- a/drivers/usb/core/usb-acpi.c
> +++ b/drivers/usb/core/usb-acpi.c
> @@ -19,6 +19,34 @@
>  
>  #include "usb.h"
>  
> +bool usb_acpi_power_manageable(struct usb_device *hdev, int port1)
> +{
> +	acpi_handle port_handle;
> +
> +	port_handle = usb_get_hub_port_acpi_handle(hdev,
> +		port1);
> +	return port_handle ? acpi_bus_power_manageable(port_handle) : false;

Ick, I _really_ hate the ? : usage in C, please use real if statements
so that everyone can read and understand them easier.  You do that a lot
here, please fix them all.

> +}
> +EXPORT_SYMBOL_GPL(usb_acpi_power_manageable);
> +
> +int usb_acpi_set_power_state(struct usb_device *hdev, int port1, bool enable)
> +{
> +	acpi_handle port_handle;
> +	unsigned char state;
> +	int error = -EINVAL;
> +
> +	port_handle = (acpi_handle)usb_get_hub_port_acpi_handle(hdev,
> +		port1);
> +	state = enable ? ACPI_STATE_D0 : ACPI_STATE_D3_COLD;
> +	error = acpi_bus_set_power(port_handle, state);

You forgot to check port_handle here.

Why not call usb_acpi_power_manageable() to ensure that you can do this?

> +	if (!error)
> +		dev_dbg(&hdev->dev, "The power of hub port %d was set to %s\n",
> +			port1, enable ? "enable" : "disabe");

Why not report the error if debugging as well?


> +
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(usb_acpi_set_power_state);
> +
>  static int usb_acpi_check_port_connect_type(struct usb_device *hdev,
>  	acpi_handle handle, int port1)
>  {
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 2c55fcf..0ce48b3 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -728,6 +728,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>  
>  			temp = xhci_readl(xhci, port_array[wIndex]);
>  			xhci_dbg(xhci, "set port power, actual port %d status  = 0x%x\n", wIndex, temp);
> +
> +			if (usb_acpi_power_manageable(hcd->self.root_hub,
> +					wIndex + 1))

Why +1?  If you have to do this everywhere, then do it only in the
function, so you can be 0 based properly.

Also, minor coding style nit, please rewrite as:
			if (usb_acpi_power_manageable(hcd->self.root_hub,
						      wIndex + 1))
Or even better yet, use a temp variable for the value returned and then
check that, it's clearer and easier to read, right?

> +				usb_acpi_set_power_state(hcd->self.root_hub,
> +					wIndex + 1, true);

Same question about +1 here.

>  			break;
>  		case USB_PORT_FEAT_RESET:
>  			temp = (temp | PORT_RESET);
> @@ -830,6 +835,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>  		case USB_PORT_FEAT_POWER:
>  			xhci_writel(xhci, temp & ~PORT_POWER,
>  				port_array[wIndex]);
> +
> +			if (usb_acpi_power_manageable(hcd->self.root_hub,
> +					wIndex + 1))
> +				usb_acpi_set_power_state(hcd->self.root_hub,
> +					wIndex + 1, false);
>  			break;
>  		default:
>  			goto error;
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index feb0a04..92f8898 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -599,6 +599,16 @@ extern int usb_lock_device_for_reset(struct usb_device *udev,
>  extern int usb_reset_device(struct usb_device *dev);
>  extern void usb_queue_reset_device(struct usb_interface *dev);
>  
> +#ifdef CONFIG_ACPI
> +extern int usb_acpi_set_power_state(struct usb_device *hdev, int port,
> +	bool enable);
> +extern bool usb_acpi_power_manageable(struct usb_device *hdev, int port);
> +#else
> +static inline int usb_acpi_set_power_state(struct usb_device *hdev, int port,
> +	bool enable) { return 0; }
> +static inline bool usb_acpi_power_manageable(struct usb_device *hdev, int port)
> +	{ return 0; }

is 0 a bool?  :)

Please get the types right.

greg k-h

Re: [PATCH 3/3] usb : Add sysfs files to control usb port's power

[Greg KH]

On Mon, Jun 11, 2012 at 10:24:34AM +0800, Lan Tianyu wrote:
> This patch is to add two sys files for each usb hub ports to control port's power.
> Control port's power through setting or clearing PORT_POWER feature.
> 
> The new sys files is under the usb hub interface sysfs portx directory.
> ls /sys/bus/usb/devices/1-0:1.0
> bAlternateSetting  bInterfaceClass  bInterfaceNumber  bInterfaceProtocol
> bInterfaceSubClass  bNumEndpoints  driver  ep_81  modalias  power
> subsystem  supports_autosuspend  uevent port1 port2 port3 port4
> 
> ls /sys/bus/usb/devices/1-0:1.0/port1
> control state
> 
> control has two options. "on" and "off"
> "on" - port power must be on.
> "off" - port power must be off.
> 
> state reports usb port's power state
> "on" - power on
> "off" - power off
> "error" - can't get power state
> 
> Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  Documentation/ABI/testing/sysfs-bus-usb |   33 ++++++
>  drivers/usb/core/hub.c                  |  168 ++++++++++++++++++++++++++++++-
>  2 files changed, 200 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
> index 6df4e6f..c2fb9d7 100644
> --- a/Documentation/ABI/testing/sysfs-bus-usb
> +++ b/Documentation/ABI/testing/sysfs-bus-usb
> @@ -208,3 +208,36 @@ Description:
>  		such as ACPI. This file will read either "removable" or
>  		"fixed" if the information is available, and "unknown"
>  		otherwise.
> +
> +What:		/sys/bus/usb/devices/.../(hub interface)/portX
> +Date:		June 2012
> +Contact		Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> +Description:
> +		The /sys/bus/usb/device/.../(hub interface)/portX directory
> +		contains attributes allowing user space to check and modify
> +		properties of the associated USB port.
> +
> +What:		/sys/bus/usb/devices/.../(hub interface)/portX/control
> +Date:		June 2012
> +Contact		Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> +Description:
> +		The /sys/bus/usb/devices/.../(hub interface)/portX/control
> +		attribute allows user space to control the power policy on
> +		the usb port.
> +
> +		All ports have one of the following two values for control
> +		"on" - port power must be on.
> +		"off" - port power must be off.
> +
> +What:		/sys/bus/usb/devices/.../(hub interface)/portX/state
> +Date:		June 2012
> +Contact		Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> +Description:
> +		The /sys/bus/usb/devices/.../(hub interface)/portX/state
> +		attribute allows user space to check hub port's power state.
> +
> +		All ports have three following states
> +		"on"	  -    port power on
> +		"off"	  -    port power off
> +		"error"	  -    can't get the hub port's power state
> +
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 2ed5244..e83b0f5 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -41,12 +41,20 @@
>  #endif
>  #endif
>  
> +enum port_power_policy {
> +	USB_PORT_POWER_ON = 0,
> +	USB_PORT_POWER_OFF,
> +};
> +
>  struct usb_hub_port {
>  	void			*port_owner;
>  	struct usb_device	*child;
>  #ifdef CONFIG_ACPI
>  	acpi_handle		port_acpi_handle;
>  #endif
> +	struct device_attribute	port_control_attr;
> +	struct device_attribute	port_state_attr;
> +	enum port_power_policy port_power_policy;

Why do you need an attribute per port here?  Shouldn't they just be
static variables?  Why duplicate them for every port?

>  	enum usb_port_connect_type connect_type;
>  };
>  
> @@ -97,6 +105,12 @@ struct usb_hub {
>  	struct usb_hub_port	*port_data;
>  };
>  
> +static const char on_string[] = "on";
> +static const char off_string[] = "off";
> +static char port_name[USB_MAXCHILDREN][8];
> +static void usb_hub_port_sysfs_add(struct usb_hub *hub);
> +static void usb_hub_port_sysfs_remove(struct usb_hub *hub);
> +
>  static inline int hub_is_superspeed(struct usb_device *hdev)
>  {
>  	return (hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS);
> @@ -847,7 +861,9 @@ static unsigned hub_power_on(struct usb_hub *hub, bool do_delay)
>  		dev_dbg(hub->intfdev, "trying to enable port power on "
>  				"non-switchable hub\n");
>  	for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++)
> -		set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
> +		if (hub->port_data[port1 - 1].port_power_policy
> +				== USB_PORT_POWER_ON)
> +			set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
>  
>  	/* Wait at least 100 msec for power to become stable */
>  	delay = max(pgood_delay, (unsigned) 100);
> @@ -1493,6 +1509,7 @@ static int hub_configure(struct usb_hub *hub,
>  	if (hub->has_indicators && blinkenlights)
>  		hub->indicator [0] = INDICATOR_CYCLE;
>  
> +	usb_hub_port_sysfs_add(hub);
>  	usb_acpi_bind_hub_ports(hdev);
>  	hub_activate(hub, HUB_INIT);
>  	return 0;
> @@ -1531,6 +1548,7 @@ static void hub_disconnect(struct usb_interface *intf)
>  	hub->error = 0;
>  	hub_quiesce(hub, HUB_DISCONNECT);
>  
> +	usb_hub_port_sysfs_remove(hub);
>  	usb_set_intfdata (intf, NULL);
>  	hub->hdev->maxchild = 0;
>  
> @@ -2558,6 +2576,25 @@ static int port_is_power_on(struct usb_hub *hub, unsigned portstatus)
>  	return ret;
>  }
>  
> +static int usb_get_hub_port_power_state(struct usb_device *hdev, int port1)
> +{
> +	struct usb_hub *hub = hdev_to_hub(hdev);
> +	struct usb_port_status data;
> +	u16 portstatus;
> +	int ret;
> +
> +	ret = get_port_status(hub->hdev, port1, &data);
> +	if (ret < 4) {
> +		dev_err(hub->intfdev,
> +			"%s failed (err = %d)\n", __func__, ret);
> +		if (ret >= 0)
> +			ret = -EIO;
> +		return ret;
> +	} else
> +		portstatus = le16_to_cpu(data.wPortStatus);
> +	return port_is_power_on(hub, portstatus);
> +}
> +
>  #ifdef	CONFIG_PM
>  
>  /* Check if a port is suspended(USB2.0 port) or in U3 state(USB3.0 port) */
> @@ -4499,6 +4536,134 @@ static int hub_thread(void *__unused)
>  	return 0;
>  }
>  
> +static ssize_t show_port_power_state(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct usb_device *udev = to_usb_device(dev->parent);
> +	struct usb_hub_port *hub_port = container_of(attr,
> +		struct usb_hub_port, port_state_attr);
> +	struct usb_hub *hub = dev_get_drvdata(dev);
> +	int port1 = hub_port - hub->port_data + 1;
> +	int power_state;
> +	const char *result;
> +
> +	power_state = usb_get_hub_port_power_state(udev, port1);
> +	if (power_state == 1)
> +		result = on_string;
> +	else if (!power_state)
> +		result = off_string;
> +	else
> +		result = "error";
> +	return sprintf(buf, "%s\n", result);
> +}
> +
> +static ssize_t show_port_power_control(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct usb_hub_port *hub_port = container_of(attr,
> +		struct usb_hub_port, port_control_attr);
> +	const char *result;
> +
> +	switch (hub_port->port_power_policy) {
> +	case USB_PORT_POWER_ON:
> +		result = on_string;
> +		break;
> +	case USB_PORT_POWER_OFF:
> +		result = off_string;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return sprintf(buf, "%s\n", result);
> +}
> +
> +static ssize_t
> +store_port_power_control(struct device *dev, struct device_attribute *attr,
> +		const char *buf, size_t count)
> +{
> +	struct usb_device *udev = to_usb_device(dev->parent);
> +	struct usb_hub_port *hub_port = container_of(attr,
> +		struct usb_hub_port, port_control_attr);
> +	char *cp;
> +	struct usb_hub *hub = dev_get_drvdata(dev);
> +	int port1 = hub_port - hub->port_data + 1;
> +	int len = count;
> +
> +	cp = memchr(buf, '\n', count);
> +	if (cp)
> +		len = cp - buf;
> +
> +	 if (len == sizeof on_string - 1
> +		&& strncmp(buf, on_string, len) == 0) {
> +		hub_port->port_power_policy = USB_PORT_POWER_ON;
> +		set_port_feature(udev, port1, USB_PORT_FEAT_POWER);
> +	} else if (len == sizeof off_string - 1
> +		&& strncmp(buf, off_string, len) == 0) {
> +		hub_port->port_power_policy = USB_PORT_POWER_OFF;
> +		clear_port_feature(udev, port1, USB_PORT_FEAT_POWER);
> +	} else
> +		return -EINVAL;
> +
> +	return count;
> +}
> +
> +static void usb_hub_port_sysfs_add(struct usb_hub *hub)
> +{
> +	int i, rc;
> +	struct attribute *port_attrs[3];
> +	struct attribute_group port_attr_group;
> +
> +	for (i = 0; i < hub->hdev->maxchild; i++) {
> +		hub->port_data[i].port_control_attr = (struct device_attribute)
> +			__ATTR(control, S_IRUGO | S_IWUSR,
> +			show_port_power_control,
> +			store_port_power_control);
> +		hub->port_data[i].port_state_attr = (struct device_attribute)
> +			__ATTR(state, S_IRUGO, show_port_power_state, NULL);
> +
> +		port_attrs[0] = &hub->port_data[i].port_control_attr.attr;
> +		port_attrs[1] = &hub->port_data[i].port_state_attr.attr;
> +		port_attrs[2] = NULL;
> +		port_attr_group = (struct attribute_group) {
> +			.name = port_name[i],
> +			.attrs = port_attrs
> +		};
> +
> +		rc = sysfs_create_group(&hub->intfdev->kobj,
> +			&port_attr_group);
> +		if (rc < 0)
> +			dev_err(hub->intfdev, "can't create sys " \
> +				"files for port%d\n", i + 1);
> +	}
> +}
> +
> +static void usb_hub_port_sysfs_remove(struct usb_hub *hub)
> +{
> +	int i;
> +	struct attribute *port_attrs[3];
> +	struct attribute_group port_attr_group;
> +
> +	for (i = 0; i < hub->hdev->maxchild; i++) {
> +		port_attrs[0] = &hub->port_data[i].port_control_attr.attr;
> +		port_attrs[1] = &hub->port_data[i].port_state_attr.attr;
> +		port_attrs[2] = NULL;
> +		port_attr_group = (struct attribute_group) {
> +			.name = port_name[i],
> +			.attrs = port_attrs
> +		};
> +		sysfs_remove_group(&hub->intfdev->kobj,
> +			&port_attr_group);
> +	}
> +}
> +
> +static void usb_hub_port_sysfs_init(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < USB_MAXCHILDREN; i++)
> +		sprintf(port_name[i], "port%d", i + 1);
> +}
> +
>  static const struct usb_device_id hub_id_table[] = {
>      { .match_flags = USB_DEVICE_ID_MATCH_DEV_CLASS,
>        .bDeviceClass = USB_CLASS_HUB},
> @@ -4531,6 +4696,7 @@ int usb_hub_init(void)
>  		return -1;
>  	}
>  
> +	usb_hub_port_sysfs_init();

Did you just race userspace with these attributes?  Are you sure?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] USB/ACPI: Add usb port's acpi power control in the xhci PORT_POWER feature request process.

[Sarah Sharp]

On Wed, Jun 13, 2012 at 12:30:38PM -0700, Greg KH wrote:
> On Mon, Jun 11, 2012 at 10:24:33AM +0800, Lan Tianyu wrote:
> > +
> > +			if (usb_acpi_power_manageable(hcd->self.root_hub,
> > +					wIndex + 1))
> 
> Why +1?  If you have to do this everywhere, then do it only in the
> function, so you can be 0 based properly.
> 
> Also, minor coding style nit, please rewrite as:
> 			if (usb_acpi_power_manageable(hcd->self.root_hub,
> 						      wIndex + 1))

This is an arbitrarily applied rule, and I don't follow it in the xHCI
driver.  The code indentation should be left at the default indentation
in the xHCI driver.  Let's keep the code style in the driver consistent,
please.

I don't understand why people want to move the trailing arguments to
align with the function parenthesis.  I can see that it might add to
readability for some people, but it's just more work on both the
developer's and maintainer's side.

Checkpatch will complain about mixed tabs and spaces, so that's more
work for me to edit my git pre-commit hook when the checkpatch error
stops git-am.  Plus the developer has to actually pause and think about
indentation, rather than letting their editor take care of it.

I'm not even sure when you would want the alignment rule applied.  In
all function calls with trailing arguments?  That seems really
excessive.

> Or even better yet, use a temp variable for the value returned and then
> check that, it's clearer and easier to read, right?

A temp variable would be fine until that function can just be
refactored.  The case statements should really just be broken out into
separate functions.

Sarah Sharp

Re: [PATCH 2/3] USB/ACPI: Add usb port's acpi power control in the xhci PORT_POWER feature request process.

[Greg KH]

On Wed, Jun 13, 2012 at 01:53:29PM -0700, Sarah Sharp wrote:
> On Wed, Jun 13, 2012 at 12:30:38PM -0700, Greg KH wrote:
> > On Mon, Jun 11, 2012 at 10:24:33AM +0800, Lan Tianyu wrote:
> > > +
> > > +			if (usb_acpi_power_manageable(hcd->self.root_hub,
> > > +					wIndex + 1))
> > 
> > Why +1?  If you have to do this everywhere, then do it only in the
> > function, so you can be 0 based properly.
> > 
> > Also, minor coding style nit, please rewrite as:
> > 			if (usb_acpi_power_manageable(hcd->self.root_hub,
> > 						      wIndex + 1))
> 
> This is an arbitrarily applied rule, and I don't follow it in the xHCI
> driver.  The code indentation should be left at the default indentation
> in the xHCI driver.  Let's keep the code style in the driver consistent,
> please.

Ok, fair enough, I didn't notice.

> I don't understand why people want to move the trailing arguments to
> align with the function parenthesis.  I can see that it might add to
> readability for some people, but it's just more work on both the
> developer's and maintainer's side.
> 
> Checkpatch will complain about mixed tabs and spaces, so that's more
> work for me to edit my git pre-commit hook when the checkpatch error
> stops git-am.  Plus the developer has to actually pause and think about
> indentation, rather than letting their editor take care of it.

No, checkpatch should not complain about the above line, as it's tabs
and then spaces.  Well, it better not, as I use it all the time :)

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] USB/ACPI: Add usb port's acpi power control in the xhci PORT_POWER feature request process.

[Alan Stern]

On Wed, 13 Jun 2012, Greg KH wrote:

> > --- a/drivers/usb/host/xhci-hub.c
> > +++ b/drivers/usb/host/xhci-hub.c
> > @@ -728,6 +728,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> >  
> >  			temp = xhci_readl(xhci, port_array[wIndex]);
> >  			xhci_dbg(xhci, "set port power, actual port %d status  = 0x%x\n", wIndex, temp);
> > +
> > +			if (usb_acpi_power_manageable(hcd->self.root_hub,
> > +					wIndex + 1))
> 
> Why +1?  If you have to do this everywhere, then do it only in the
> function, so you can be 0 based properly.

This is an awkward matter.  The USB spec numbers hub ports starting
from 1, and so do several of the functions in usbcore, in order to
match the spec.  But of course, all our internal arrays are indexed
starting from 0.

The cleanest solution here might be to keep wIndex as an origin-0 value
and create an origin-1 temporary variable (like hub.c does with
"port1").

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] usb : Add sysfs files to control usb port's power

[Alan Stern]

On Wed, 13 Jun 2012, Greg KH wrote:

> > +	struct device_attribute	port_control_attr;
> > +	struct device_attribute	port_state_attr;
> > +	enum port_power_policy port_power_policy;
> 
> Why do you need an attribute per port here?  Shouldn't they just be
> static variables?  Why duplicate them for every port?

If they were static, there would be no way for the store and show
methods to know which port they were called for.  That's because the
ports aren't separate kobjects; all these port attributes are bound to
the hub device.

> > +static ssize_t show_port_power_state(struct device *dev,
> > +	struct device_attribute *attr, char *buf)
> > +{
> > +	struct usb_device *udev = to_usb_device(dev->parent);
> > +	struct usb_hub_port *hub_port = container_of(attr,
> > +		struct usb_hub_port, port_state_attr);

You can see it here.  The only way to get the port is by seeing where
the attribute is stored.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] USB/ACPI: Add usb port's acpi power control in the xhci PORT_POWER feature request process.

[Sarah Sharp]

On Wed, Jun 13, 2012 at 05:10:43PM -0400, Alan Stern wrote:
> On Wed, 13 Jun 2012, Greg KH wrote:
> 
> > > --- a/drivers/usb/host/xhci-hub.c
> > > +++ b/drivers/usb/host/xhci-hub.c
> > > @@ -728,6 +728,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> > >  
> > >  			temp = xhci_readl(xhci, port_array[wIndex]);
> > >  			xhci_dbg(xhci, "set port power, actual port %d status  = 0x%x\n", wIndex, temp);
> > > +
> > > +			if (usb_acpi_power_manageable(hcd->self.root_hub,
> > > +					wIndex + 1))
> > 
> > Why +1?  If you have to do this everywhere, then do it only in the
> > function, so you can be 0 based properly.
> 
> This is an awkward matter.  The USB spec numbers hub ports starting
> from 1, and so do several of the functions in usbcore, in order to
> match the spec.  But of course, all our internal arrays are indexed
> starting from 0.
> 
> The cleanest solution here might be to keep wIndex as an origin-0 value
> and create an origin-1 temporary variable (like hub.c does with
> "port1").

Oh, is that why it's named port1?  I could never understand why. :)

Sarah Sharp

Re: [PATCH 3/3] usb : Add sysfs files to control usb port's power

[Greg KH]

On Wed, Jun 13, 2012 at 05:15:08PM -0400, Alan Stern wrote:
> On Wed, 13 Jun 2012, Greg KH wrote:
> 
> > > +	struct device_attribute	port_control_attr;
> > > +	struct device_attribute	port_state_attr;
> > > +	enum port_power_policy port_power_policy;
> > 
> > Why do you need an attribute per port here?  Shouldn't they just be
> > static variables?  Why duplicate them for every port?
> 
> If they were static, there would be no way for the store and show
> methods to know which port they were called for.  That's because the
> ports aren't separate kobjects; all these port attributes are bound to
> the hub device.

Ports should be a struct device if we are going to hang attributes off
of them, otherwise userspace can get very confused.

> > > +static ssize_t show_port_power_state(struct device *dev,
> > > +	struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct usb_device *udev = to_usb_device(dev->parent);
> > > +	struct usb_hub_port *hub_port = container_of(attr,
> > > +		struct usb_hub_port, port_state_attr);
> 
> You can see it here.  The only way to get the port is by seeing where
> the attribute is stored.

Ick, that's nasty.

How about making a port a real device?  That should solve this problem,
right?  And I thought I recommended doing that a month or so ago, what
happened with that proposal?

thanks,

greg k-h

Re: [PATCH 2/3] USB/ACPI: Add usb port's acpi power control in the xhci PORT_POWER feature request process.

[Lan Tianyu]

On 2012年06月14日 03:30, Greg KH wrote:
> On Mon, Jun 11, 2012 at 10:24:33AM +0800, Lan Tianyu wrote:
>> On our developping machine, bios can provide usb port's  power control via
>> acpi. This patch is to provide usb port's power control way through setting
>> or clearing PORT_POWER feature requests. Add two functions usb_acpi_power_manageable()
>> and usb_acpi_set_power_state(). The first one is used to find whether the
>> usb port has acpi power resource and the second is to set the power state.
>> They are invoked in the xhci_hub_control() where clearing or setting PORT_POWER
>> feature requests are processed.
>>
>> Signed-off-by: Lan Tianyu<tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> ---
>>   drivers/usb/core/usb-acpi.c |   28 ++++++++++++++++++++++++++++
>>   drivers/usb/host/xhci-hub.c |   10 ++++++++++
>>   include/linux/usb.h         |   10 ++++++++++
>>   3 files changed, 48 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
>> index 82c90d0..e95f26f 100644
>> --- a/drivers/usb/core/usb-acpi.c
>> +++ b/drivers/usb/core/usb-acpi.c
>> @@ -19,6 +19,34 @@
>>
>>   #include "usb.h"
>>
>> +bool usb_acpi_power_manageable(struct usb_device *hdev, int port1)
>> +{
>> +	acpi_handle port_handle;
>> +
>> +	port_handle = usb_get_hub_port_acpi_handle(hdev,
>> +		port1);
>> +	return port_handle ? acpi_bus_power_manageable(port_handle) : false;
>
> Ick, I _really_ hate the ? : usage in C, please use real if statements
> so that everyone can read and understand them easier.  You do that a lot
> here, please fix them all.
>
Ok. But in some places, for example
dev_dbg(&hdev->dev, "The power of hub port %d was set to %s\n",
		port1, enable ? "enable" : "disable");
try to print two result. ?: is more convenient. If I use "if" statement,
that will be
if (enable)
	result = "enable";
else
	result = "disable";
dev_dbg(&hdev->dev, "The power of hub port %d was set to %s\n",
		port1, result);

This just looks a little complex.

>> +}
>> +EXPORT_SYMBOL_GPL(usb_acpi_power_manageable);
>> +
>> +int usb_acpi_set_power_state(struct usb_device *hdev, int port1, bool enable)
>> +{
>> +	acpi_handle port_handle;
>> +	unsigned char state;
>> +	int error = -EINVAL;
>> +
>> +	port_handle = (acpi_handle)usb_get_hub_port_acpi_handle(hdev,
>> +		port1);
>> +	state = enable ? ACPI_STATE_D0 : ACPI_STATE_D3_COLD;
>> +	error = acpi_bus_set_power(port_handle, state);
>
> You forgot to check port_handle here.
>
> Why not call usb_acpi_power_manageable() to ensure that you can do this?

In my code, usb_acpi_power_manageable() is invoked before 
usb_acpi_set_power_state().
Do you mean I should call usb_acpi_power_manageable() in the 
usb_acpi_set_power_state()?

>
>> +	if (!error)
>> +		dev_dbg(&hdev->dev, "The power of hub port %d was set to %s\n",
>> +			port1, enable ? "enable" : "disabe");
>
> Why not report the error if debugging as well?

Ok.
>
>
>> +
>> +	return error;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_acpi_set_power_state);
>> +
>>   static int usb_acpi_check_port_connect_type(struct usb_device *hdev,
>>   	acpi_handle handle, int port1)
>>   {
>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>> index 2c55fcf..0ce48b3 100644
>> --- a/drivers/usb/host/xhci-hub.c
>> +++ b/drivers/usb/host/xhci-hub.c
>> @@ -728,6 +728,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>>
>>   			temp = xhci_readl(xhci, port_array[wIndex]);
>>   			xhci_dbg(xhci, "set port power, actual port %d status  = 0x%x\n", wIndex, temp);
>> +
>> +			if (usb_acpi_power_manageable(hcd->self.root_hub,
>> +					wIndex + 1))
>
> Why +1?  If you have to do this everywhere, then do it only in the
> function, so you can be 0 based properly.
>
> Also, minor coding style nit, please rewrite as:
> 			if (usb_acpi_power_manageable(hcd->self.root_hub,
> 						      wIndex + 1))
> Or even better yet, use a temp variable for the value returned and then
> check that, it's clearer and easier to read, right?
>
Yeah. Thanks for you suggestion. I will do it.

>> +				usb_acpi_set_power_state(hcd->self.root_hub,
>> +					wIndex + 1, true);
>
> Same question about +1 here.
>
>>   			break;
>>   		case USB_PORT_FEAT_RESET:
>>   			temp = (temp | PORT_RESET);
>> @@ -830,6 +835,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>>   		case USB_PORT_FEAT_POWER:
>>   			xhci_writel(xhci, temp&  ~PORT_POWER,
>>   				port_array[wIndex]);
>> +
>> +			if (usb_acpi_power_manageable(hcd->self.root_hub,
>> +					wIndex + 1))
>> +				usb_acpi_set_power_state(hcd->self.root_hub,
>> +					wIndex + 1, false);
>>   			break;
>>   		default:
>>   			goto error;
>> diff --git a/include/linux/usb.h b/include/linux/usb.h
>> index feb0a04..92f8898 100644
>> --- a/include/linux/usb.h
>> +++ b/include/linux/usb.h
>> @@ -599,6 +599,16 @@ extern int usb_lock_device_for_reset(struct usb_device *udev,
>>   extern int usb_reset_device(struct usb_device *dev);
>>   extern void usb_queue_reset_device(struct usb_interface *dev);
>>
>> +#ifdef CONFIG_ACPI
>> +extern int usb_acpi_set_power_state(struct usb_device *hdev, int port,
>> +	bool enable);
>> +extern bool usb_acpi_power_manageable(struct usb_device *hdev, int port);
>> +#else
>> +static inline int usb_acpi_set_power_state(struct usb_device *hdev, int port,
>> +	bool enable) { return 0; }
>> +static inline bool usb_acpi_power_manageable(struct usb_device *hdev, int port)
>> +	{ return 0; }
>
> is 0 a bool?  :)
>
> Please get the types right.
So careless. Thanks for reminder.
>
> greg k-h

-- 
Best Regards
Tianyu Lan
linux kernel enabling team
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] usb : Add sysfs files to control usb port's power

[Alan Stern]

On Wed, 13 Jun 2012, Greg KH wrote:

> On Wed, Jun 13, 2012 at 05:15:08PM -0400, Alan Stern wrote:
> > On Wed, 13 Jun 2012, Greg KH wrote:
> > 
> > > > +	struct device_attribute	port_control_attr;
> > > > +	struct device_attribute	port_state_attr;
> > > > +	enum port_power_policy port_power_policy;
> > > 
> > > Why do you need an attribute per port here?  Shouldn't they just be
> > > static variables?  Why duplicate them for every port?
> > 
> > If they were static, there would be no way for the store and show
> > methods to know which port they were called for.  That's because the
> > ports aren't separate kobjects; all these port attributes are bound to
> > the hub device.
> 
> Ports should be a struct device if we are going to hang attributes off
> of them, otherwise userspace can get very confused.

Is that really appropriate?  Ports don't have their own driver, they
can't be probed or removed, they can't be suspended or resumed, you
can't do I/O to them, they don't have resources, they don't have 
children...

In short, they don't qualify as separate devices.  All they do have is
a few attributes (power, ownership, and connectivity).

> How about making a port a real device?  That should solve this problem,
> right?  And I thought I recommended doing that a month or so ago, what
> happened with that proposal?

I argued against it, for the reasons listed above.

Alan Stern

Re: [PATCH 3/3] usb : Add sysfs files to control usb port's power

[Greg KH]

On Thu, Jun 14, 2012 at 09:57:37AM -0400, Alan Stern wrote:
> On Wed, 13 Jun 2012, Greg KH wrote:
> 
> > On Wed, Jun 13, 2012 at 05:15:08PM -0400, Alan Stern wrote:
> > > On Wed, 13 Jun 2012, Greg KH wrote:
> > > 
> > > > > +	struct device_attribute	port_control_attr;
> > > > > +	struct device_attribute	port_state_attr;
> > > > > +	enum port_power_policy port_power_policy;
> > > > 
> > > > Why do you need an attribute per port here?  Shouldn't they just be
> > > > static variables?  Why duplicate them for every port?
> > > 
> > > If they were static, there would be no way for the store and show
> > > methods to know which port they were called for.  That's because the
> > > ports aren't separate kobjects; all these port attributes are bound to
> > > the hub device.
> > 
> > Ports should be a struct device if we are going to hang attributes off
> > of them, otherwise userspace can get very confused.
> 
> Is that really appropriate?  Ports don't have their own driver, they
> can't be probed or removed, they can't be suspended or resumed, you
> can't do I/O to them, they don't have resources, they don't have 
> children...

Ah, but they can have "children" if we want to move the devices attached
to them there.  But that's something to consider in the future, not for
now.

As for the other things, if we applied those rules to all other parts of
the device tree, well, it would be a lot smaller :)

You are showing something "logical" in the tree, that exists, and has
attributes (power, etc.).  Because of that, userspace needs to be
notified of them, as it should be doing things with them.  So, make them
a device so that userspace can actually see them.  Otherwise things
break down really quickly, and you can not use the "standard" userspace
tools to control them (i.e. libusb and friends.)

thanks,

greg k-h

Re: [PATCH 2/3] USB/ACPI: Add usb port's acpi power control in the xhci PORT_POWER feature request process.

[Greg KH]

On Thu, Jun 14, 2012 at 03:22:42PM +0800, Lan Tianyu wrote:
> On 2012年06月14日 03:30, Greg KH wrote:
> >On Mon, Jun 11, 2012 at 10:24:33AM +0800, Lan Tianyu wrote:
> >>On our developping machine, bios can provide usb port's  power control via
> >>acpi. This patch is to provide usb port's power control way through setting
> >>or clearing PORT_POWER feature requests. Add two functions usb_acpi_power_manageable()
> >>and usb_acpi_set_power_state(). The first one is used to find whether the
> >>usb port has acpi power resource and the second is to set the power state.
> >>They are invoked in the xhci_hub_control() where clearing or setting PORT_POWER
> >>feature requests are processed.
> >>
> >>Signed-off-by: Lan Tianyu<tianyu.lan@intel.com>
> >>---
> >>  drivers/usb/core/usb-acpi.c |   28 ++++++++++++++++++++++++++++
> >>  drivers/usb/host/xhci-hub.c |   10 ++++++++++
> >>  include/linux/usb.h         |   10 ++++++++++
> >>  3 files changed, 48 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
> >>index 82c90d0..e95f26f 100644
> >>--- a/drivers/usb/core/usb-acpi.c
> >>+++ b/drivers/usb/core/usb-acpi.c
> >>@@ -19,6 +19,34 @@
> >>
> >>  #include "usb.h"
> >>
> >>+bool usb_acpi_power_manageable(struct usb_device *hdev, int port1)
> >>+{
> >>+	acpi_handle port_handle;
> >>+
> >>+	port_handle = usb_get_hub_port_acpi_handle(hdev,
> >>+		port1);
> >>+	return port_handle ? acpi_bus_power_manageable(port_handle) : false;
> >
> >Ick, I _really_ hate the ? : usage in C, please use real if statements
> >so that everyone can read and understand them easier.  You do that a lot
> >here, please fix them all.
> >
> Ok. But in some places, for example
> dev_dbg(&hdev->dev, "The power of hub port %d was set to %s\n",
> 		port1, enable ? "enable" : "disable");
> try to print two result. ?: is more convenient. If I use "if" statement,
> that will be
> if (enable)
> 	result = "enable";
> else
> 	result = "disable";
> dev_dbg(&hdev->dev, "The power of hub port %d was set to %s\n",
> 		port1, result);
> 
> This just looks a little complex.

I understand that, which is why I didn't complain about that.  But even
then, you could just change the line to:
	dev_dbg(&hdev->dev, "The power of hub port %d was set to %d\n", port1, enable);
and you would be able to tell what is going on.


> >>+}
> >>+EXPORT_SYMBOL_GPL(usb_acpi_power_manageable);
> >>+
> >>+int usb_acpi_set_power_state(struct usb_device *hdev, int port1, bool enable)
> >>+{
> >>+	acpi_handle port_handle;
> >>+	unsigned char state;
> >>+	int error = -EINVAL;
> >>+
> >>+	port_handle = (acpi_handle)usb_get_hub_port_acpi_handle(hdev,
> >>+		port1);
> >>+	state = enable ? ACPI_STATE_D0 : ACPI_STATE_D3_COLD;
> >>+	error = acpi_bus_set_power(port_handle, state);
> >
> >You forgot to check port_handle here.
> >
> >Why not call usb_acpi_power_manageable() to ensure that you can do this?
> 
> In my code, usb_acpi_power_manageable() is invoked before
> usb_acpi_set_power_state().

Are you sure that all future callers will always do this?

> Do you mean I should call usb_acpi_power_manageable() in the
> usb_acpi_set_power_state()?

Maybe, if it makes sense to.  But don't fail, like you currently do, if
someone doesn't do that, as you aren't telling the caller that they are
required to do so.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] USB/ACPI: Add usb port's acpi power control in the xhci PORT_POWER feature request process.

[Lan Tianyu]

On 2012年06月14日 23:44, Greg KH wrote:
> On Thu, Jun 14, 2012 at 03:22:42PM +0800, Lan Tianyu wrote:
>> On 2012年06月14日 03:30, Greg KH wrote:
>>> On Mon, Jun 11, 2012 at 10:24:33AM +0800, Lan Tianyu wrote:
>>>> On our developping machine, bios can provide usb port's  power control via
>>>> acpi. This patch is to provide usb port's power control way through setting
>>>> or clearing PORT_POWER feature requests. Add two functions usb_acpi_power_manageable()
>>>> and usb_acpi_set_power_state(). The first one is used to find whether the
>>>> usb port has acpi power resource and the second is to set the power state.
>>>> They are invoked in the xhci_hub_control() where clearing or setting PORT_POWER
>>>> feature requests are processed.
>>>>
>>>> Signed-off-by: Lan Tianyu<tianyu.lan@intel.com>
>>>> ---
>>>>   drivers/usb/core/usb-acpi.c |   28 ++++++++++++++++++++++++++++
>>>>   drivers/usb/host/xhci-hub.c |   10 ++++++++++
>>>>   include/linux/usb.h         |   10 ++++++++++
>>>>   3 files changed, 48 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
>>>> index 82c90d0..e95f26f 100644
>>>> --- a/drivers/usb/core/usb-acpi.c
>>>> +++ b/drivers/usb/core/usb-acpi.c
>>>> @@ -19,6 +19,34 @@
>>>>
>>>>   #include "usb.h"
>>>>
>>>> +bool usb_acpi_power_manageable(struct usb_device *hdev, int port1)
>>>> +{
>>>> +	acpi_handle port_handle;
>>>> +
>>>> +	port_handle = usb_get_hub_port_acpi_handle(hdev,
>>>> +		port1);
>>>> +	return port_handle ? acpi_bus_power_manageable(port_handle) : false;
>>>
>>> Ick, I _really_ hate the ? : usage in C, please use real if statements
>>> so that everyone can read and understand them easier.  You do that a lot
>>> here, please fix them all.
>>>
>> Ok. But in some places, for example
>> dev_dbg(&hdev->dev, "The power of hub port %d was set to %s\n",
>> 		port1, enable ? "enable" : "disable");
>> try to print two result. ?: is more convenient. If I use "if" statement,
>> that will be
>> if (enable)
>> 	result = "enable";
>> else
>> 	result = "disable";
>> dev_dbg(&hdev->dev, "The power of hub port %d was set to %s\n",
>> 		port1, result);
>>
>> This just looks a little complex.
>
> I understand that, which is why I didn't complain about that.  But even
> then, you could just change the line to:
> 	dev_dbg(&hdev->dev, "The power of hub port %d was set to %d\n", port1, enable);
> and you would be able to tell what is going on.
>
Ok. I get it.
>
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(usb_acpi_power_manageable);
>>>> +
>>>> +int usb_acpi_set_power_state(struct usb_device *hdev, int port1, bool enable)
>>>> +{
>>>> +	acpi_handle port_handle;
>>>> +	unsigned char state;
>>>> +	int error = -EINVAL;
>>>> +
>>>> +	port_handle = (acpi_handle)usb_get_hub_port_acpi_handle(hdev,
>>>> +		port1);
>>>> +	state = enable ? ACPI_STATE_D0 : ACPI_STATE_D3_COLD;
>>>> +	error = acpi_bus_set_power(port_handle, state);
>>>
>>> You forgot to check port_handle here.
>>>
>>> Why not call usb_acpi_power_manageable() to ensure that you can do this?
>>
>> In my code, usb_acpi_power_manageable() is invoked before
>> usb_acpi_set_power_state().
>
> Are you sure that all future callers will always do this?
>
>> Do you mean I should call usb_acpi_power_manageable() in the
>> usb_acpi_set_power_state()?
>
> Maybe, if it makes sense to.  But don't fail, like you currently do, if
> someone doesn't do that, as you aren't telling the caller that they are
> required to do so.

How about to add kernel-doc to make future user notice it? On the other side,
I check the acpi_bus_set_power and if the port didn't have acpi power resource,
it would return error and no harm.

>
> thanks,
>
> greg k-h

-- 
Best Regards
Tianyu Lan
linux kernel enabling team
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html