Am Donnerstag, 12. Oktober 2006 19:10 schrieb Alan Stern:
> 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.
I'd prefer to read it from sysfs managed by usbcore.
> > 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().
Why? The device is idle now.
> > +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.
OK, I'll unify this. It just seemed cleaner.
> 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.
Yes.
> On the other hand, why do you need to check this at all? This routine
> won't get called after the driver is unbound.
But during.
> > +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.
Right.
Regards
Oliver
-------------------------------------------------------------------------
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
