On Thu, 12 Oct 2006, Oliver Neukum wrote:
> Hi,
>
> I can't boot 2.6.19-rc1, so here's an incomplete version of autosuspend
> for hid to invite comments on the basic approach.
>
> Regards
> Oliver
>
> --- linux-2.6.19-rc1/drivers/usb/input/hid.h 2006-10-05 04:57:05.000000000
> +0200
> +++ gregtree/drivers/usb/input/hid.h 2006-10-09 15:20:21.000000000 +0200
> @@ -384,6 +384,9 @@
> #define HID_IN_RUNNING 3
> #define HID_RESET_PENDING 4
> #define HID_SUSPENDED 5
> +#define HID_IN_IDLE 6
> +
> +#define HID_IDLE_TIME 200*HZ
This will need to be configurable separately for each device.
> int hid_open(struct hid_device *hid)
> {
> - ++hid->open;
> + clear_bit(HID_IN_IDLE, &hid->iofl);
> + if (!hid->open++)
> + usb_autopm_get_interface(hid->intf);
> if (hid_start_in(hid))
> hid_io_error(hid);
> return 0;
> @@ -1375,8 +1383,10 @@
>
> void hid_close(struct hid_device *hid)
> {
> - if (!--hid->open)
> + if (!--hid->open) {
> usb_kill_urb(hid->urbin);
> + set_bit(HID_IN_IDLE, &hid->iofl);
> + }
> }
There needs to be a call to usb_autopm_put_interface() here, to balance
the call in hid_open().
> +void hid_add_idle_timer(struct hid_device *hid)
> +{
> + hid->idle_timer.expires = jiffies + HID_IDLE_TIME;
> + add_timer(&hid->idle_timer);
> +}
> +
> +static void hid_mod_idle_timer(struct hid_device *hid)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&hid->inlock, flags);
> + if (usb_get_intfdata(hid->intf))
> + mod_timer(&hid->idle_timer, jiffies + HID_IDLE_TIME);
> + spin_unlock_irqrestore(&hid->inlock, flags);
> +}
It's not obvious why you need two separate functions (you can always call
mod_timer in place of add_timer), or why you need the spinlock.
usb_get_intfdata() here isn't safe; if the value can be 0 then it can also
be anything else, since a different driver may be bound to the interface.
It would be better to have an "unbound" flag in the hid_device structure.
On the other hand, why do you need to check this at all? This routine
won't get called after the driver is unbound.
> +static void __hid_report_idle(void *d)
> +{
> + struct usb_interface *intf = d;
> +
> + usb_autopm_put_interface(intf);
> +}
This call needs to be balanced somewhere. Otherwise intf->pm_usage_cnt
will be decremented at every timeout and never incremented. See below.
> +static void hid_report_idle(struct hid_device *hid)
> +{
> + struct usb_interface *intf = hid->intf;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&hid->inlock, flags);
> + if (!test_bit(HID_SUSPENDED, &hid->iofl))
> + if (usb_get_intfdata(intf))
> + schedule_work(&hid->idle_work);
> + spin_unlock_irqrestore(&hid->inlock, flags);
> +}
Again, usb_get_intfdata() isn't safe or appropriate.
> +static void hid_check_idle(unsigned long arg)
> +{
> + struct hid_device *hid = (struct hid_device *)arg;
> + int state;
> +
> + state = test_and_set_bit(HID_IN_IDLE, &hid->iofl);
> +
> + if (!state) {
> + hid_mod_idle_timer(hid);
> + } else {
> + hid_report_idle(hid);
> + }
> +}
Somewhere you will need to restart the idle timer after an autoresume.
Probably in the resume method. That may also be where you want to set
intf->pm_usage_cnt to 1 if the device is open.
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
