On Fri, 9 Feb 2007, Oliver Neukum wrote:
> Am Freitag, 9. Februar 2007 23:12 schrieb Alan Stern:
>
> Firstly, cool.
> I'll look at it closer again, but just some first comments.
>
> > +static ssize_t
> > +set_autosuspend(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct usb_device *udev;
> > + unsigned value, old;
> > +
> > + if (sscanf(buf, "%u", &value) != 1 || value >= INT_MAX/HZ)
> > + return -EINVAL;
> > + value *= HZ;
> > +
> > + if (is_usb_device(dev)) {
> > + udev = to_usb_device(dev);
> > + old = udev->autosuspend_delay;
> > + udev->autosuspend_delay = value;
> > + } else {
> > + struct usb_interface *intf = to_usb_interface(dev);
> > +
> > + udev = interface_to_usbdev(intf);
> > + old = intf->autosuspend_delay;
> > + intf->autosuspend_delay = value;
> > + }
> > + if (value > 0 && old == 0)
> > + usb_try_autosuspend_device(udev);
>
> You should retain symmetry. Writing 0 should wake up the device.
The problem is that after the device is suspended, we don't know whether
it was an autosuspend or the result of an external user command. (It
might even be both, if the user sends a suspend command to an already
autosuspended device.) If it's not an autosuspend, changing this value
shouldn't cause a resume. I figured it was best simply not to resume --
the user can always force a resume if desired.
Think of it this way: Writing a positive number causes an autosuspend
event to occur that many seconds later. Writing a zero prevents any
autosuspend events from occurring. But neither causes an _autoresume_
event; those happen only when needed.
> Secondly, how about exposing USB_IF_DEVICE_BUSY through sysfs?
It's constantly changing. The value exposed wouldn't be reliable. (Or
for devices where it's not constantly changing, the value is always 0 and
hence uninteresting.)
> > +static ssize_t
> > +set_suspended(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct usb_device *udev = to_usb_device(dev);
> > + unsigned value;
> > + int rc;
> > +
> > + if (sscanf(buf, "%u", &value) != 1 || value > 1)
> > + return -EINVAL;
> > + usb_lock_device(udev);
> > + if (value)
> > + rc = usb_bus_type.suspend(dev, PMSG_SUSPEND);
>
> Thirdly, doesn't this have security implications? You can shut down
> network interfaces and swap spaces this way.
Yes indeed -- just like many other sysfs interfaces, such as
bConfigurationValue. That also is capable of shutting down network
interfaces and swap spaces.
> > + else {
> > + rc = usb_autoresume_device(udev);
>
> This seems asymmetric. Here you use an autosuspend function, while
> the opposite value does a "hard" suspend.
It seemed a little odd to me too... It ought to work equivalently if it
were replaced with a "hard" resume followed by
usb_try_autosuspend_device(); I'll add that to the list of things to
change.
In fact the API really is slightly asymmetric in this respect.
usb_autosuspend_device() merely queues an autosuspend request, and it
might fail to do even that if the device or one of its interfaces is busy.
By contrast, usb_autoresume_device() always tries to resume the device
immediately and it can't fail because of some "busy" condition. There
isn't much difference between a "hard" and a "soft" resume (the chief
difference is what happens to pm_usage_cnt).
Alan Stern
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
