Re: [PATCH V5 8/8] usb : Add usb port's power control attributes

[Lan Tianyu <tianyu.lan@xxxxxxxxx>]

On 2012/6/29 22:16, Alan Stern wrote:
> On Fri, 29 Jun 2012, Lan Tianyu wrote:
>
> > Change since v4: Add clear PORT_POWER if power policy is "off" in the
> > hub_power_on(). Return -EIO if set/clear PORT_POWER fails in the store_port_power_control()
> >
> > This patch is to add two attributes for each usb hub ports to control port's power.
> > Control port's power through setting or clearing PORT_POWER feature.
> >
> > control has two options. "auto", "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@xxxxxxxxx>
>
> Some more thoughts on this...
>
> > +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->parent);
> > +	int port1, power_state;
> > +	const char *result;
> > +
> > +	sscanf(dev_name(dev), "port%d",&port1);
> > +	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 DEVICE_ATTR(state, S_IRUGO, show_port_power_state, NULL);
>
> When this routine runs, the hub might be suspended.  You should call
> usb_autopm_get_interface before getting the power state and
> usb_autopm_put_interface afterward.
Right. Thanks for your reminder.
> > +static ssize_t store_port_power_control(struct device *dev,
> > +		struct device_attribute *attr, const char *buf, size_t count)
> > +{
> > +	struct usb_device *hdev = to_usb_device(dev->parent->parent);
> > +	struct usb_port *hub_port = to_usb_port(dev);
> > +	int port1, ret, len = count;
> > +	char *cp;
> > +
> > +	sscanf(dev_name(dev), "port%d",&port1);
> > +	cp = memchr(buf, '\n', count);
> > +	if (cp)
> > +		len = cp - buf;
> > +	if (len == sizeof on_string - 1
> > +		&&  strncmp(buf, on_string, len) == 0) {
>
> Please indent the continuation line an extra tab stop so that it
> doesn't look like it's part of the body of the "if" statement.
>
> > +		hub_port->port_power_policy = USB_PORT_POWER_ON;
> > +		ret = set_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
> > +		if (ret<  0)
> > +			return -EIO;
> > +	} else if (len == sizeof off_string - 1
> > +		&&  strncmp(buf, off_string, len) == 0) {
>
> Same here.
>
> > +		hub_port->port_power_policy = USB_PORT_POWER_OFF;
> > +		ret = clear_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
> > +		if (ret<  0)
> > +			return -EIO;
> > +	} else
> > +		return -EINVAL;
> > +
> > +	return count;
> > +}
>
> The usb_autopm_* calls are also needed in this function.
>
> In addition, what happens if a device is attached to a port when the
> policy is changed to "off"?  We probably want to make sure the device
> gets disconnected when this happens.  You should call
> hub_port_logical_disconnect(hub, port1) before clearing the power
> feature.
Ok.
> Have you given any thought to locking?  For example, what would happen
> if the port power policy were changed at the same time as a new device
> was plugged into the port?
Do you mean while a new device was enumerating, the port powered off? I 
guess the enumerating process will fail.


> My off-hand impression is that it won't matter and locking isn't
> needed.  But you should think about it more carefully.
>
> Alan Stern


--
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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html