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

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

[Lan Tianyu]

portx_power_control has three options. "auto", "on" and "off"
"auto" - if port without device, power off the port.
"on" - port must be power on.
"off" - port must be power off

portx_power_state can be used to determine ports's power state.

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

 git diff --stat 
 drivers/usb/core/hub.c      |   38 ++++++++++
 drivers/usb/core/sysfs.c    |  167 ++++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/core/usb-acpi.c |   28 +++++++
 drivers/usb/core/usb.c      |    2 +
 drivers/usb/core/usb.h      |    6 ++
 drivers/usb/host/xhci-hub.c |   15 ++++
 include/linux/usb.h         |   22 ++++++
 7 files changed, 275 insertions(+), 3 deletions(-)

[RFC 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 |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 2732ef6..feb515e 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -827,6 +827,11 @@ 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:
+			temp = xhci_readl(xhci, port_array[wIndex])
+				& ~PORT_POWER;
+			xhci_writel(xhci, temp,	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

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

[Andiry Xu]

On 05/25/2012 01:48 PM, Lan Tianyu wrote:
> This patch is to add process of clearing PORT_POWER feature request
> for xhci hub.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  drivers/usb/host/xhci-hub.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 2732ef6..feb515e 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -827,6 +827,11 @@ 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:
> +			temp = xhci_readl(xhci, port_array[wIndex])
> +				& ~PORT_POWER;

You should not read the register again. Simply use temp &= ~PORT_POWER
is enough.

Otherwise you must call xhci_port_state_to_neutral(temp) again before
write it back to register.

Thanks,
Andiry

> +			xhci_writel(xhci, temp,	port_array[wIndex]);
> +			break;
>  		default:
>  			goto error;
>  		}

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

[Lan Tianyu]

On 2012年05月28日 18:28, Andiry Xu wrote:
> On 05/25/2012 01:48 PM, Lan Tianyu wrote:
>> This patch is to add process of clearing PORT_POWER feature request
>> for xhci hub.
>>
>> Signed-off-by: Lan Tianyu<tianyu.lan@intel.com>
>> ---
>>   drivers/usb/host/xhci-hub.c |    5 +++++
>>   1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>> index 2732ef6..feb515e 100644
>> --- a/drivers/usb/host/xhci-hub.c
>> +++ b/drivers/usb/host/xhci-hub.c
>> @@ -827,6 +827,11 @@ 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:
>> +			temp = xhci_readl(xhci, port_array[wIndex])
>> +				&  ~PORT_POWER;
>
> You should not read the register again. Simply use temp&= ~PORT_POWER
> is enough.
>
> Otherwise you must call xhci_port_state_to_neutral(temp) again before
> write it back to register.
hi andiry:
	Thanks for your reminder and I will take the first one.	
>
> Thanks,
> Andiry
>
>> +			xhci_writel(xhci, temp,	port_array[wIndex]);
>> +			break;
>>   		default:
>>   			goto error;
>>   		}
>
>

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

[RFC 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-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;
+}
+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 feb515e..231f64d 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);
@@ -831,6 +836,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			temp = xhci_readl(xhci, port_array[wIndex])
 				& ~PORT_POWER;
 			xhci_writel(xhci, temp,	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

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

[RFC 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,
e.g port1_power_control and port1_power_state both for port one. Control port's
power through setting or clearing PORT_POWER feature.

port1_power_control has three options. "auto", "on" and "off"
"auto" - if port without device, power off the port.
"on" - port must be power on.
"off" - port must be power off.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/usb/core/hub.c   |   38 +++++++++++
 drivers/usb/core/sysfs.c |  167 +++++++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/core/usb.c   |    2 +
 drivers/usb/core/usb.h   |    6 ++
 include/linux/usb.h      |   12 +++
 5 files changed, 222 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index cec5bd1..7f0adf6 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -48,6 +48,7 @@ struct usb_hub_port {
 	acpi_handle		port_acpi_handle;
 #endif
 	enum usb_port_connect_type connect_type;
+	enum port_power_policy port_power_policy;
 };
 
 struct usb_hub {
@@ -1493,6 +1494,12 @@ static int hub_configure(struct usb_hub *hub,
 	if (hub->has_indicators && blinkenlights)
 		hub->indicator [0] = INDICATOR_CYCLE;
 
+	/*
+	 * Create the ports' power related sys files.
+	 * The function requires the hub's port number.
+	 */
+	usb_create_sysfs_dev_files(hdev);
+
 	usb_acpi_bind_hub_ports(hdev);
 	hub_activate(hub, HUB_INIT);
 	return 0;
@@ -4985,6 +4992,37 @@ usb_get_hub_port_connect_type(struct usb_device *hdev,	int port1)
 	return hub->port_data[port1 - 1].connect_type;
 }
 
+enum port_power_policy
+usb_get_hub_port_power_policy(struct usb_device *hdev, int port1)
+{
+	struct usb_hub *hub = hdev_to_hub(hdev);
+	return hub->port_data[port1 - 1].port_power_policy;
+}
+
+void usb_set_hub_port_power_policy(struct usb_device *hdev, int port1,
+	enum port_power_policy value)
+{
+	struct usb_hub *hub = hdev_to_hub(hdev);
+	hub->port_data[port1 - 1].port_power_policy = value;
+}
+
+void usb_set_hub_port_power(struct usb_device *hdev, int port1, bool enabled)
+{
+	if (enabled)
+		set_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
+	else
+		clear_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
+}
+
+int usb_get_hub_port_power_state(struct usb_device *hdev, int port1)
+{
+	struct usb_hub *hub = hdev_to_hub(hdev);
+	u16 portstatus, portchange;
+
+	hub_port_status(hub, port1, &portstatus, &portchange);
+	return port_is_power_on(hub, portstatus);
+}
+
 #ifdef CONFIG_ACPI
 /**
  * usb_set_hub_port_acpi_handle - Set the usb port's acpi handle
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index 566d9f9..8703c7e 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -16,6 +16,14 @@
 #include <linux/usb/quirks.h>
 #include "usb.h"
 
+static const char on_string[] = "on";
+static const char off_string[] = "off";
+static const char auto_string[] = "auto";
+static char power_control_name[USB_MAXCHILDREN][25];
+static char power_state_name[USB_MAXCHILDREN][25];
+static struct device_attribute power_control_attr[USB_MAXCHILDREN];
+static struct device_attribute power_state_attr[USB_MAXCHILDREN];
+
 /* Active configuration fields */
 #define usb_actconfig_show(field, multiplier, format_string)		\
 static ssize_t  show_##field(struct device *dev,			\
@@ -376,9 +384,6 @@ set_autosuspend(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR(autosuspend, S_IRUGO | S_IWUSR,
 		show_autosuspend, set_autosuspend);
 
-static const char on_string[] = "on";
-static const char auto_string[] = "auto";
-
 static void warn_level(void) {
 	static int level_warned;
 
@@ -524,6 +529,85 @@ static void remove_power_attributes(struct device *dev)
 
 #endif	/* CONFIG_USB_SUSPEND */
 
+static ssize_t show_port_power_state(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	struct usb_device *udev = to_usb_device(dev);
+	const char *result;
+	int port1;
+
+	if (sscanf(attr->attr.name, "port%d_power_control", &port1) != 1)
+		return -EINVAL;
+
+	if (usb_get_hub_port_power_state(udev, port1))
+		result = on_string;
+	else
+		result = off_string;
+	return sprintf(buf, "%s\n", result);
+}
+
+static ssize_t show_port_power_control(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	struct usb_device *udev = to_usb_device(dev);
+	enum port_power_policy policy;
+	int port1;
+	const char *result;
+
+	if (sscanf(attr->attr.name, "port%d_power_control", &port1) != 1)
+		return -EINVAL;
+
+	policy = usb_get_hub_port_power_policy(udev, port1);
+	switch (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);
+	char *cp;
+	int port1;
+	int len = count;
+
+	if (sscanf(attr->attr.name, "port%d_power_control", &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) {
+		if (!usb_get_hub_child(udev, port1))
+			usb_set_hub_port_power(udev, port1, false);
+		usb_set_hub_port_power_policy(udev, port1, USB_PORT_POWER_AUTO);
+	} else if (len == sizeof on_string - 1
+		&& strncmp(buf, on_string, len) == 0) {
+		usb_set_hub_port_power_policy(udev, port1, USB_PORT_POWER_ON);
+		usb_set_hub_port_power(udev, port1, true);
+	} else if (len == sizeof off_string - 1
+		&& strncmp(buf, off_string, len) == 0) {
+		usb_set_hub_port_power_policy(udev, port1, USB_PORT_POWER_OFF);
+		usb_set_hub_port_power(udev, port1, false);
+	} else
+		return -EINVAL;
+
+	return count;
+}
 
 /* Descriptor fields */
 #define usb_descriptor_attr_le16(field, format_string)			\
@@ -771,6 +855,9 @@ void usb_remove_sysfs_dev_files(struct usb_device *udev)
 	remove_power_attributes(dev);
 	remove_persist_attributes(dev);
 	device_remove_bin_file(dev, &dev_bin_attr_descriptors);
+
+	if (udev->descriptor.bDeviceClass == USB_CLASS_HUB)
+		usb_remove_sysfs_hub_port_power(udev);
 }
 
 /* Interface Accociation Descriptor fields */
@@ -945,3 +1032,77 @@ void usb_remove_sysfs_intf_files(struct usb_interface *intf)
 	device_remove_file(&intf->dev, &dev_attr_interface);
 	intf->sysfs_files_created = 0;
 }
+
+void usb_create_sysfs_hub_port_power(struct usb_device *udev)
+{
+	int i, rc;
+
+	if (udev->descriptor.bDeviceClass == USB_CLASS_HUB) {
+		for (i = 0; i < udev->maxchild && !rc; i++) {
+			rc = sysfs_add_file_to_group(&udev->dev.kobj,
+				&power_control_attr[i].attr,
+				power_group_name);
+			if (rc) {
+				while (--i > 0)
+					sysfs_remove_file_from_group(&udev->dev.kobj,
+						&power_control_attr[i].attr,
+						power_group_name);
+				break;
+			}
+		}
+
+		for (i = 0; i < udev->maxchild && !rc; i++) {
+			rc = sysfs_add_file_to_group(&udev->dev.kobj,
+				&power_state_attr[i].attr,
+				power_group_name);
+			if (rc) {
+				while (--i > 0)
+					sysfs_remove_file_from_group(&udev->dev.kobj,
+						&power_state_attr[i].attr,
+						power_group_name);
+				break;
+			}
+		}
+
+	}
+}
+
+void usb_remove_sysfs_hub_port_power(struct usb_device *udev)
+{
+	int i;
+
+	for (i = 0; i < udev->maxchild; i++) {
+		sysfs_remove_file_from_group(&udev->dev.kobj,
+			&power_control_attr[i].attr,
+			power_group_name);
+		sysfs_remove_file_from_group(&udev->dev.kobj,
+			&power_state_attr[i].attr,
+			power_group_name);
+	}
+}
+
+int __init usb_sysfs_init(void)
+{
+	int i;
+	char name[25];
+
+	for (i = 0; i < USB_MAXCHILDREN; i++) {
+		sprintf(name, "port%d_power_control", i + 1);
+		strcpy(power_control_name[i], name);
+		power_control_attr[i] = (struct device_attribute) {
+			.attr = { .name = power_control_name[i], .mode = 0644 }, \
+			.show	= show_port_power_control, \
+			.store	= store_port_power_control \
+		};
+
+		sprintf(name, "port%d_power_state", i + 1);
+		strcpy(power_state_name[i], name);
+		power_state_attr[i] = (struct device_attribute) {
+			.attr = { .name = power_state_name[i], .mode = 0644 }, \
+			.show	= show_port_power_state, \
+		};
+
+	}
+
+	return 0;
+}
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 25d0c61..b8908d5 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -1011,6 +1011,8 @@ static int __init usb_init(void)
 		return 0;
 	}
 
+	usb_sysfs_init();
+
 	retval = usb_debugfs_init();
 	if (retval)
 		goto out;
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index da78c15..5737f46 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -158,10 +158,16 @@ extern void usb_notify_add_device(struct usb_device *udev);
 extern void usb_notify_remove_device(struct usb_device *udev);
 extern void usb_notify_add_bus(struct usb_bus *ubus);
 extern void usb_notify_remove_bus(struct usb_bus *ubus);
+extern int usb_sysfs_init(void);
 extern enum usb_port_connect_type
 	usb_get_hub_port_connect_type(struct usb_device *hdev, int port1);
 extern void usb_set_hub_port_connect_type(struct usb_device *hdev, int port1,
 	enum usb_port_connect_type type);
+extern void usb_create_sysfs_hub_port_power(struct usb_device *udev);
+extern void usb_remove_sysfs_hub_port_power(struct usb_device *udev);
+extern enum port_power_policy
+	usb_get_hub_port_power_policy(struct usb_device *hdev, int port1);
+extern int usb_get_hub_port_power_state(struct usb_device *hdev, int port1);
 
 #ifdef CONFIG_ACPI
 extern int usb_acpi_register(void);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 92f8898..58fa7b8 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -424,6 +424,12 @@ enum usb_port_connect_type {
 	USB_PORT_NOT_USED,
 };
 
+enum port_power_policy {
+	USB_PORT_POWER_AUTO = 0,
+	USB_PORT_POWER_ON,
+	USB_PORT_POWER_OFF,
+};
+
 /**
  * struct usb_device - kernel's representation of a USB device
  * @devnum: device number; address on a USB bus
@@ -570,6 +576,12 @@ static inline struct usb_device *interface_to_usbdev(struct usb_interface *intf)
 	return to_usb_device(intf->dev.parent);
 }
 
+extern enum port_power_policy
+usb_get_hub_port_power_policy(struct usb_device *hdev, int port1);
+extern void usb_set_hub_port_power_policy(struct usb_device *hdev,
+	int port1, enum port_power_policy value);
+extern void usb_set_hub_port_power(struct usb_device *hdev,
+	int port1, bool enabled);
 extern struct usb_device *usb_get_dev(struct usb_device *dev);
 extern void usb_put_dev(struct usb_device *dev);
 extern struct usb_device *usb_get_hub_child(struct usb_device *hdev,
-- 
1.7.6.rc2.8.g28eb

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

[Alan Stern]

On Fri, 25 May 2012, Lan Tianyu wrote:

> This patch is to add two sys files for each usb hub ports to control port's power,
> e.g port1_power_control and port1_power_state both for port one. Control port's
> power through setting or clearing PORT_POWER feature.
> 
> port1_power_control has three options. "auto", "on" and "off"
> "auto" - if port without device, power off the port.
> "on" - port must be power on.
> "off" - port must be power off.

Remind me again -- what's the point of the "auto" setting?  It doesn't 
seem to mean anything as a port power state.

That is, if you write "auto" to the sysfs file then the power will
remain on if it is already on and a device is plugged in, or it will go
off if it is already off or no device is plugged in.  Either way, it
won't remain at "auto" because once the power is off, there's no way to
tell when a new device gets plugged in.

Given this, do the power_control and power_state files need to be
separate?

> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c

> @@ -4985,6 +4992,37 @@ usb_get_hub_port_connect_type(struct usb_device *hdev,	int port1)
>  	return hub->port_data[port1 - 1].connect_type;
>  }
>  
> +enum port_power_policy
> +usb_get_hub_port_power_policy(struct usb_device *hdev, int port1)
> +{
> +	struct usb_hub *hub = hdev_to_hub(hdev);
> +	return hub->port_data[port1 - 1].port_power_policy;
> +}
> +
> +void usb_set_hub_port_power_policy(struct usb_device *hdev, int port1,
> +	enum port_power_policy value)
> +{
> +	struct usb_hub *hub = hdev_to_hub(hdev);
> +	hub->port_data[port1 - 1].port_power_policy = value;
> +}

I suspect these routines aren't needed at all.

> +void usb_set_hub_port_power(struct usb_device *hdev, int port1, bool enabled)
> +{
> +	if (enabled)
> +		set_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
> +	else
> +		clear_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
> +}
> +
> +int usb_get_hub_port_power_state(struct usb_device *hdev, int port1)
> +{
> +	struct usb_hub *hub = hdev_to_hub(hdev);
> +	u16 portstatus, portchange;
> +
> +	hub_port_status(hub, port1, &portstatus, &portchange);

Use get_port_status(), not hub_port_status().

> +	return port_is_power_on(hub, portstatus);
> +}

These routines don't belong here at the end of the file.  They belong
near the start, close to set_port_feature() and get_port_status().


> --- a/drivers/usb/core/sysfs.c
> +++ b/drivers/usb/core/sysfs.c

No, no, no!  sysfs.c is meant for attributes that apply to all USB
devices.  Your new attributes apply only to hubs, so they don't belong
here.

Furthermore, the attributes defined in sysfs.c go in the device's
sysfs directory, but your attributes belong in the hub's interface
directory.  That is, the files should go into
/sys/bus/usb/devices/1-0:1.0, not /sys/bus/usb/devices/usb1.

And they probably don't belong in the power/ subdirectory either.  In
fact, you might want to create a separate subdirectory for each port
with one or two files in each subdirectory.

Alan Stern

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

[Lan Tianyu]

hi alan:
	Great thanks for you review. Sorry for later reply due to my travel :)
On 2012年05月25日 23:41, Alan Stern wrote:
> On Fri, 25 May 2012, Lan Tianyu wrote:
>
>> This patch is to add two sys files for each usb hub ports to control port's power,
>> e.g port1_power_control and port1_power_state both for port one. Control port's
>> power through setting or clearing PORT_POWER feature.
>>
>> port1_power_control has three options. "auto", "on" and "off"
>> "auto" - if port without device, power off the port.
>> "on" - port must be power on.
>> "off" - port must be power off.
>
> Remind me again -- what's the point of the "auto" setting?  It doesn't
> seem to mean anything as a port power state.
>
> That is, if you write "auto" to the sysfs file then the power will
> remain on if it is already on and a device is plugged in, or it will go
> off if it is already off or no device is plugged in.  Either way, it
> won't remain at "auto" because once the power is off, there's no way to
> tell when a new device gets plugged in.
>
> Given this, do the power_control and power_state files need to be
> separate?
 From my opinion, power_control determines the work pattern.
Currently, we just consider port without device. When "auto", power off
the port directly. For port 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 need to resume and suspend again.

Different patterns have different time of power off.
I think their relation just like power/control and power/runtime_status.
when power/control => "auto", power_/runtime_status maybe "active" or "suspended".

>
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>
>> @@ -4985,6 +4992,37 @@ usb_get_hub_port_connect_type(struct usb_device *hdev,	int port1)
>>   	return hub->port_data[port1 - 1].connect_type;
>>   }
>>
>> +enum port_power_policy
>> +usb_get_hub_port_power_policy(struct usb_device *hdev, int port1)
>> +{
>> +	struct usb_hub *hub = hdev_to_hub(hdev);
>> +	return hub->port_data[port1 - 1].port_power_policy;
>> +}
>> +
>> +void usb_set_hub_port_power_policy(struct usb_device *hdev, int port1,
>> +	enum port_power_policy value)
>> +{
>> +	struct usb_hub *hub = hdev_to_hub(hdev);
>> +	hub->port_data[port1 - 1].port_power_policy = value;
>> +}
>
> I suspect these routines aren't needed at all.
>
>> +void usb_set_hub_port_power(struct usb_device *hdev, int port1, bool enabled)
>> +{
>> +	if (enabled)
>> +		set_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
>> +	else
>> +		clear_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
>> +}
>> +
>> +int usb_get_hub_port_power_state(struct usb_device *hdev, int port1)
>> +{
>> +	struct usb_hub *hub = hdev_to_hub(hdev);
>> +	u16 portstatus, portchange;
>> +
>> +	hub_port_status(hub, port1,&portstatus,&portchange);
>
> Use get_port_status(), not hub_port_status().
>
>> +	return port_is_power_on(hub, portstatus);
>> +}
>
> These routines don't belong here at the end of the file.  They belong
> near the start, close to set_port_feature() and get_port_status().
>
Ok.
>
>> --- a/drivers/usb/core/sysfs.c
>> +++ b/drivers/usb/core/sysfs.c
>
> No, no, no!  sysfs.c is meant for attributes that apply to all USB
> devices.  Your new attributes apply only to hubs, so they don't belong
> here.
>
> Furthermore, the attributes defined in sysfs.c go in the device's
> sysfs directory, but your attributes belong in the hub's interface
> directory.  That is, the files should go into
> /sys/bus/usb/devices/1-0:1.0, not /sys/bus/usb/devices/usb1.
>
Ok. So I should put these code in the hub.c or create a hub-sysfs.c?

> And they probably don't belong in the power/ subdirectory either.  In
> fact, you might want to create a separate subdirectory for each port
> with one or two files in each subdirectory.
Do you mean
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
power_control power_state
?

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

[Alan Stern]

On Mon, 28 May 2012, Lan Tianyu wrote:

> >> port1_power_control has three options. "auto", "on" and "off"
> >> "auto" - if port without device, power off the port.
> >> "on" - port must be power on.
> >> "off" - port must be power off.
> >
> > Remind me again -- what's the point of the "auto" setting?  It doesn't
> > seem to mean anything as a port power state.
> >
> > That is, if you write "auto" to the sysfs file then the power will
> > remain on if it is already on and a device is plugged in, or it will go
> > off if it is already off or no device is plugged in.  Either way, it
> > won't remain at "auto" because once the power is off, there's no way to
> > tell when a new device gets plugged in.
> >
> > Given this, do the power_control and power_state files need to be
> > separate?
>  From my opinion, power_control determines the work pattern.
> Currently, we just consider port without device. When "auto", power off
> the port directly. For port 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 need to resume and suspend again.

Okay, this is different from what you wrote above.  "auto" means power 
off the port whenever there is no device or the device is suspended 
(and wakeup isn't enabled).

> > No, no, no!  sysfs.c is meant for attributes that apply to all USB
> > devices.  Your new attributes apply only to hubs, so they don't belong
> > here.
> >
> > Furthermore, the attributes defined in sysfs.c go in the device's
> > sysfs directory, but your attributes belong in the hub's interface
> > directory.  That is, the files should go into
> > /sys/bus/usb/devices/1-0:1.0, not /sys/bus/usb/devices/usb1.
> >
> Ok. So I should put these code in the hub.c or create a hub-sysfs.c?

For now, put them into hub.c.  hub.c needs to be broken up into several 
files anyway.

> > And they probably don't belong in the power/ subdirectory either.  In
> > fact, you might want to create a separate subdirectory for each port
> > with one or two files in each subdirectory.
> Do you mean
> 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
> power_control power_state
> ?

Yes.  Except that perhaps you can leave out the "power_" prefixes.

Alan Stern