On Fri, 1 Jun 2012, Lan Tianyu wrote:
> 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
You need to add entries to Documentation/ABI explaining these new
files.
> 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".
> "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@xxxxxxxxx>
> ---
> drivers/usb/core/hub.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 181 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 2ed5244..612eb2d 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -41,12 +41,23 @@
> #endif
> #endif
>
> +enum port_power_policy {
> + USB_PORT_POWER_AUTO = 0,
> + USB_PORT_POWER_ON,
USB_PORT_POWER_ON should be 0, because that is the default policy.
> + 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 attribute_group port_attr_group;
> + struct attribute *port_attrs[3];
You don't really need these pointers. The attribute files can be added
to and removed from the group by hand, one at a time.
> @@ -97,6 +108,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][10];
Use [8] instead of [10]. The names are never more than 6 characters.
> @@ -466,6 +484,14 @@ static int set_port_feature(struct usb_device *hdev, int port1, int feature)
> NULL, 0, 1000);
> }
>
> +static 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);
> +}
This function isn't needed at all. Just call set_port_feature() or
clear_port_feature() directly.
> @@ -2558,6 +2586,27 @@ 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);
> + u16 portstatus;
> + int ret;
> +
> + mutex_lock(&hub->status_mutex);
> + ret = get_hub_status(hub->hdev, &hub->status->hub);
I told you before: Don't use get_hub_status(); use get_port_status()
instead. Then you don't need to lock the status_mutex.
> @@ -4499,6 +4548,137 @@ 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);
> + const char *result;
> + int port1;
> +
> + if (sscanf(hub_port->port_attr_group.name, "port%d", &port1) != 1)
> + return -EINVAL;
> +
> + if (usb_get_hub_port_power_state(udev, port1) == 1)
> + result = on_string;
> + else if (!usb_get_hub_port_power_state(udev, port1))
Don't call usb_get_hub_port_power_state() twice. Call it once and save
the 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);
> + enum port_power_policy policy;
> + int port1;
> + const char *result;
> +
> + if (sscanf(hub_port->port_attr_group.name, "port%d", &port1) != 1)
> + return -EINVAL;
Why is this here? port1 doesn't get used in this function.
> + policy = hub_port->port_power_policy;
> + switch (policy) {
You don't need a separate local variable for policy. Just do
switch (hub_port->port_power_policy) {
> +static void usb_hub_port_sysfs_add(struct usb_hub *hub)
> +{
> + int i, rc;
> +
> + for (i = 0; i < hub->hdev->maxchild; i++) {
> + hub->port_data[i].port_control_attr = (struct device_attribute)
> + __ATTR(control, 0777, show_port_power_control,
> + store_port_power_control);
> + hub->port_data[i].port_state_attr = (struct device_attribute)
> + __ATTR(state, 0777, show_port_power_state, NULL);
Setting the permissions to 0777 doesn't seem like a very good idea. Do
you want any user on the system to be able to turn off power to your
flash drive?
Also, why should the power_state attribute have write permission when
it doesn't have a store method?
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
