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

[RFC PATCH V3 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

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

[Lan Tianyu]

Change since v2: add ABI descripters for new sysfs files and with some small
modification.

Change since v1: move sysfs files from usb hub device sysfs directory to usb
hub interface sysfs directory.

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 three options. "auto", "on" and "off"
"auto" - For ports with device, there is a proposal that if the device was not
	in the suspend at that pointer, the power would remain on but it would
	power off when it was suspended. If the the device was in the suspend,
	"auto" means the device could be put into much lower state so the device
	need to resume and suspend again.
"on" - port must be power on.
"off" - port must be power off.

state reports usb ports' 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 |   37 +++++++
 drivers/usb/core/hub.c                  |  172 +++++++++++++++++++++++++++++++
 2 files changed, 209 insertions(+), 0 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
index 6df4e6f..62c396b 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -208,3 +208,40 @@ 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)/port1, port2,...portX
+Date:		June 2012
+Contact		Lan Tianyu <tianyu.lan@intel.com>
+Description:
+		The /sys/bus/usb/device/.../(hub interface)/portX directory
+		contains attributes allowing the user space to check and modify
+		some usb port related properties of associated 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 attribue
+		allows the user space to control the power policy on the usb port
+
+		All ports have one of the following three values for portX/control
+		"auto" - For ports with device, if the device was not in the suspend
+			at that pointer, the power would remain on but it would power
+			off when it was suspended. If the the device was in the suspend,
+			"auto" means the device could be put into much lower state so
+			the device would be resumed and suspended again.
+		"on" - port must be power on.
+		"off" - port must be power 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 attribue
+		allows the 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..445cd6e 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -41,12 +41,22 @@
 #endif
 #endif
 
+enum port_power_policy {
+	USB_PORT_POWER_ON = 0,
+	USB_PORT_POWER_OFF,
+	USB_PORT_POWER_AUTO,
+};
+
 struct usb_hub_port {
 	void			*port_owner;
 	struct usb_device	*child;
 #ifdef CONFIG_ACPI
 	acpi_handle		port_acpi_handle;
 #endif
+	struct attribute_group	port_attr_group;
+	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 +107,13 @@ struct usb_hub {
 	struct usb_hub_port	*port_data;
 };
 
+static const char on_string[] = "on";
+static const char off_string[] = "off";
+static const char auto_string[] = "auto";
+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);
@@ -1493,6 +1510,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 +1549,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 +2577,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 +4537,139 @@ 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);
+	int power_state;
+	const char *result;
+	int port1;
+
+	if (sscanf(hub_port->port_attr_group.name, "port%d", &port1) != 1)
+		return -EINVAL;
+
+	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_AUTO:
+		result = auto_string;
+		break;
+	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;
+	int port1;
+	int len = count;
+
+	if (sscanf(hub_port->port_attr_group.name, "port%d", &port1) != 1)
+		return -EINVAL;
+
+	cp = memchr(buf, '\n', count);
+	if (cp)
+		len = cp - buf;
+
+	if (len == sizeof auto_string - 1 &&
+		strncmp(buf, auto_string, len) == 0) {
+		hub_port->port_power_policy = USB_PORT_POWER_AUTO;
+	} else 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];
+
+	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;
+		hub->port_data[i].port_attr_group = (struct attribute_group) {
+			.name = port_name[i],
+			.attrs = port_attrs
+		};
+
+		rc = sysfs_create_group(&hub->intfdev->kobj,
+			&hub->port_data[i].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];
+
+	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;
+		hub->port_data[i].port_attr_group.attrs = port_attrs;
+		sysfs_remove_group(&hub->intfdev->kobj,
+			&hub->port_data[i].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 +4702,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: [RFC PATCH V3 3/3] usb : Add sysfs files to control usb port's power

[Alan Stern]

On Mon, 4 Jun 2012, Lan Tianyu wrote:

> Change since v2: add ABI descripters for new sysfs files and with some small
> modification.

Better, but it still can be improved.

> Change since v1: move sysfs files from usb hub device sysfs directory to usb
> hub interface sysfs directory.
> 
> 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 three options. "auto", "on" and "off"
> "auto" - For ports with device, there is a proposal that if the device was not
> 	in the suspend at that pointer, the power would remain on but it would

s/in the suspend/suspended/ -- this change is needed throughout the 
patch.

s/pointer/point/

> 	power off when it was suspended. If the the device was in the suspend,
> 	"auto" means the device could be put into much lower state so the device
> 	need to resume and suspend again.

Get rid of "so the device...".  You don't need to resume a suspended 
port before powering it off.

> "on" - port must be power on.
> "off" - port must be power off.
> 
> state reports usb ports' power state

s/ports'/port's/

> "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 |   37 +++++++
>  drivers/usb/core/hub.c                  |  172 +++++++++++++++++++++++++++++++
>  2 files changed, 209 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
> index 6df4e6f..62c396b 100644
> --- a/Documentation/ABI/testing/sysfs-bus-usb
> +++ b/Documentation/ABI/testing/sysfs-bus-usb
> @@ -208,3 +208,40 @@ 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)/port1, port2,...portX

Just portX, not port1, port2, ...

> +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 the user space to check and modify

s/the user/user/  -- here and below

> +		some usb port related properties of associated port.

Change to:	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:

Lines should not extend past column 76 or so.

> +		The /sys/bus/usb/devices/.../(hub interface)/portX/control attribue

s/attribue/attribute/  -- here and below

> +		allows the user space to control the power policy on the usb port
> +
> +		All ports have one of the following three values for portX/control
> +		"auto" - For ports with device, if the device was not in the suspend
> +			at that pointer, the power would remain on but it would power
> +			off when it was suspended. If the the device was in the suspend,
> +			"auto" means the device could be put into much lower state so
> +			the device would be resumed and suspended again.

The patch does not implement anything at all for "auto".  It should do 
_something_.

> +		"on" - port must be power on.

Change to: port power must be on.

> +		"off" - port must be power off.

Change to: 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 attribue
> +		allows the 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..445cd6e 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -41,12 +41,22 @@
>  #endif
>  #endif
>  
> +enum port_power_policy {
> +	USB_PORT_POWER_ON = 0,
> +	USB_PORT_POWER_OFF,
> +	USB_PORT_POWER_AUTO,
> +};
> +
>  struct usb_hub_port {
>  	void			*port_owner;
>  	struct usb_device	*child;
>  #ifdef CONFIG_ACPI
>  	acpi_handle		port_acpi_handle;
>  #endif
> +	struct attribute_group	port_attr_group;

I think you don't need the attribute_group at all.  See below.

> @@ -2558,6 +2577,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,

checkpatch will complain about the ' ' before the '('.  You should fix 
such problems before posting.

> @@ -4499,6 +4537,139 @@ 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);
> +	int power_state;
> +	const char *result;
> +	int port1;
> +
> +	if (sscanf(hub_port->port_attr_group.name, "port%d", &port1) != 1)
> +		return -EINVAL;

You can determine the port number without using the attribute group:

	struct usb_hub *hub = dev_get_drvdata(dev);
	int port1 = hub_port - hub->port_data + 1;

> +static void usb_hub_port_sysfs_add(struct usb_hub *hub)
> +{
> +	int i, rc;
> +	struct attribute* port_attrs[3];

s/e* p/e *p/

Make port_attr_group another local variable rather than part of 
hub->port_data[i].

> +
> +	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;
> +		hub->port_data[i].port_attr_group = (struct attribute_group) {
> +			.name = port_name[i],
> +			.attrs = port_attrs
> +		};
> +
> +		rc = sysfs_create_group(&hub->intfdev->kobj,
> +			&hub->port_data[i].port_attr_group);
> +		if (rc < 0)
> +			dev_err(hub->intfdev, "can't create sys " \
> +				"files for port%d\n", i + 1);
> +	}
> +}

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: [RFC PATCH V3 3/3] usb : Add sysfs files to control usb port's power

[Lan Tianyu]

On 2012年06月04日 23:02, Alan Stern wrote:
> On Mon, 4 Jun 2012, Lan Tianyu wrote:
> s/attribue/attribute/  -- here and below
>
>> +		allows the user space to control the power policy on the usb port
>> +
>> +		All ports have one of the following three values for portX/control
>> +		"auto" - For ports with device, if the device was not in the suspend
>> +			at that pointer, the power would remain on but it would power
>> +			off when it was suspended. If the the device was in the suspend,
>> +			"auto" means the device could be put into much lower state so
>> +			the device would be resumed and suspended again.
>
> The patch does not implement anything at all for "auto".  It should do
> _something_.
>
Yeah, if we don't power off port without device when echo "auto" > portX/control,
this patch does nothing about "auto". Original plan is to deal with port without
devices firstly. Do you mean I should add the process of port with device in this
patch?


-- 
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: [RFC PATCH V3 3/3] usb : Add sysfs files to control usb port's power

[Alan Stern]

On Tue, 5 Jun 2012, Lan Tianyu wrote:

> > The patch does not implement anything at all for "auto".  It should do
> > _something_.
> >
> Yeah, if we don't power off port without device when echo "auto" > portX/control,
> this patch does nothing about "auto". Original plan is to deal with port without
> devices firstly. Do you mean I should add the process of port with device in this
> patch?

At least make a start.  For example, have "auto" turn off the power if
no device is attached to the port.

If "auto" does nothing, there's no reason to put it in this patch at 
all.  It could be added in a later patch, along with its 
implementation.

Alan Stern

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

[Sarah Sharp]

On Tue, Jun 05, 2012 at 10:18:50AM -0400, Alan Stern wrote:
> On Tue, 5 Jun 2012, Lan Tianyu wrote:
> 
> > > The patch does not implement anything at all for "auto".  It should do
> > > _something_.
> > >
> > Yeah, if we don't power off port without device when echo "auto" > portX/control,
> > this patch does nothing about "auto". Original plan is to deal with port without
> > devices firstly. Do you mean I should add the process of port with device in this
> > patch?
> 
> At least make a start.  For example, have "auto" turn off the power if
> no device is attached to the port.

Auto should only turn off power if no device is attached to the port
*and* we know it's an internal port through ACPI.  Don't forget that
we'll lose port connect events when the power is off.

Sarah Sharp
--
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: [RFC PATCH V3 3/3] usb : Add sysfs files to control usb port's power

[Lan Tianyu]

On 2012年06月05日 22:18, Alan Stern wrote:
> On Tue, 5 Jun 2012, Lan Tianyu wrote:
>
>>> The patch does not implement anything at all for "auto".  It should do
>>> _something_.
>>>
>> Yeah, if we don't power off port without device when echo "auto">  portX/control,
>> this patch does nothing about "auto". Original plan is to deal with port without
>> devices firstly. Do you mean I should add the process of port with device in this
>> patch?
>
> At least make a start.  For example, have "auto" turn off the power if
> no device is attached to the port.

In your reply for v2, hint that user space can power off port without
device, so I removed related code. Did I misunderstand?

>> control has three options. "auto", "on" and "off"
>> "auto" - if port without device, power off the port.
>> 	(This patch only cover ports without device)
> Is this option really needed?  Can't userspace check first to see
> whether there is a device, and then write "off" if there isn't?
>
>> 	For ports with device, there is a proposal that if the device was not
>> 	in the suspend at that pointer, the power would remain on but it would
>> 	power off when it was suspended. If the the device was in the suspend,
>> 	"auto" means the device could be put into much lower state so the device
>> 	need to resume and suspend again.
> That makes more sense.  Without this, however, there is no need for
> "auto".
>


>
> If "auto" does nothing, there's no reason to put it in this patch at
> all.  It could be added in a later patch, along with its
> implementation.
OK. I prefer to remove "auto" option in this patch. We can further discuss
the function of "auto".

>
> Alan Stern
>

-- 
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

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

[Alan Stern]

On Wed, 6 Jun 2012, Lan Tianyu wrote:

> > If "auto" does nothing, there's no reason to put it in this patch at
> > all.  It could be added in a later patch, along with its
> > implementation.
> OK. I prefer to remove "auto" option in this patch. We can further discuss
> the function of "auto".

Sounds good.

One more problem -- your implementation of "off" isn't quite right.  
You need to update hub_power_on() too.

Alan Stern

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

[Lan Tianyu]

On 2012年06月06日 22:21, Alan Stern wrote:
> On Wed, 6 Jun 2012, Lan Tianyu wrote:
>
>>> If "auto" does nothing, there's no reason to put it in this patch at
>>> all.  It could be added in a later patch, along with its
>>> implementation.
>> OK. I prefer to remove "auto" option in this patch. We can further discuss
>> the function of "auto".
>
> Sounds good.
>
> One more problem -- your implementation of "off" isn't quite right.
> You need to update hub_power_on() too.
>
How about this?

--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -861,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);


> Alan Stern
>

-- 
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

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

[Alan Stern]

On Thu, 7 Jun 2012, Lan Tianyu wrote:

> > One more problem -- your implementation of "off" isn't quite right.
> > You need to update hub_power_on() too.
> >
> How about this?
> 
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -861,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);

Yes, that's the idea.

Alan Stern