Am Donnerstag 26 Juni 2008 16:45:45 schrieb Ville Syrjälä:
> On Thu, Jun 26, 2008 at 03:57:17PM +0200, Oliver Neukum wrote:
> > Hi,
> >
> > this is power management for usbtouch. Could somebody test it?
> >
> > Regards
> > Oliver
> >
> > ---
> >
> > --- linux-2.6.26-sierra/drivers/input/touchscreen/usbtouchscreen.c.alt2 2008-06-26 15:23:38.000000000 +0200
> > +++ linux-2.6.26-sierra/drivers/input/touchscreen/usbtouchscreen.c 2008-06-26 15:48:44.000000000 +0200
> > @@ -89,13 +89,17 @@ struct usbtouch_usb {
> > int buf_len;
> > struct urb *irq;
> > struct usb_device *udev;
> > + struct usb_interface *intf;
> > struct input_dev *input;
> > struct usbtouch_device_info *type;
> > + struct mutex lock;
> > char name[128];
> > char phys[64];
> >
> > int x, y;
> > int touch, press;
> > +
> > + char open:1;
> > };
> >
> >
> > @@ -820,13 +824,23 @@ exit:
> > static int usbtouch_open(struct input_dev *input)
> > {
> > struct usbtouch_usb *usbtouch = input_get_drvdata(input);
> > + int rv;
> >
> > + rv = usb_autopm_get_interface(usbtouch->intf);
> > + if (rv < 0)
> > + goto bail;
> > + mutex_lock(&usbtouch->lock);
> > usbtouch->irq->dev = usbtouch->udev;
> >
> > - if (usb_submit_urb(usbtouch->irq, GFP_KERNEL))
> > - return -EIO;
> > -
> > - return 0;
> > + if (usb_submit_urb(usbtouch->irq, GFP_KERNEL)) {
> > + rv = -EIO;
> > + usb_autopm_put_interface(usbtouch->intf);
> > + } else {
> > + usbtouch->open = 1;
> > + }
> > + mutex_unlock(&usbtouch->lock);
> > +bail:
> > + return rv;
> > }
> >
> > static void usbtouch_close(struct input_dev *input)
> > @@ -834,6 +848,10 @@ static void usbtouch_close(struct input_
> > struct usbtouch_usb *usbtouch = input_get_drvdata(input);
> >
> > usb_kill_urb(usbtouch->irq);
>
> If usbtouch_resume() happens here it will resubmit the urb.
Thanks. That's a bug.
> > + mutex_lock(&usbtouch->lock);
> > + usbtouch->open = 0;
> > + mutex_unlock(&usbtouch->lock);
> > + usb_autopm_put_interface(usbtouch->intf);
> > }
> >
> >
> > @@ -889,6 +907,8 @@ static int usbtouch_probe(struct usb_int
> >
> > usbtouch->udev = udev;
> > usbtouch->input = input_dev;
> > + mutex_init(&usbtouch->lock);
> > + usbtouch->intf = intf;
> >
> > if (udev->manufacturer)
> > strlcpy(usbtouch->name, udev->manufacturer, sizeof(usbtouch->name));
> > @@ -973,20 +993,76 @@ static void usbtouch_disconnect(struct u
> >
> > dbg("%s - usbtouch is initialized, cleaning up", __FUNCTION__);
> > usb_set_intfdata(intf, NULL);
> > - input_unregister_device(usbtouch->input);
> > usb_kill_urb(usbtouch->irq);
> > + input_unregister_device(usbtouch->input);
> > usb_free_urb(usbtouch->irq);
> > usbtouch_free_buffers(interface_to_usbdev(intf), usbtouch);
> > kfree(usbtouch);
> > }
>
> Already discussed :)
OK.
> > +static int usbtouch_suspend(struct usb_interface *intf, pm_message_t message)
> > +{
> > + struct usbtouch_usb *usbtouch = usb_get_intfdata(intf);
> > +
> > + dbg("%s - called", __FUNCTION__);
> > +
> > + mutex_lock(&usbtouch->lock);
> > + usb_kill_urb(usbtouch->irq);
> > + mutex_unlock(&usbtouch->lock);
> > + return 0;
> > +}
>
> Shouldn't all the usb_kill_urb() callers check the usbtouch->open flag
> first? Perhaps it's not considered an error to kill an unsubmitted urb
> though.
It's no problem to call usb_kill_urb() on an unsubmitted urb.
That must be the case as the urb can always finish.
> > +static int usbtouch_resume(struct usb_interface *intf)
> > +{
> > + struct usbtouch_usb *usbtouch = usb_get_intfdata(intf);
> > + int rv = 0;
> > +
> > + dbg("%s - called", __FUNCTION__);
> > +
> > + mutex_lock(&usbtouch->lock);
> > + if (usbtouch->open)
> > + rv = usb_submit_urb(usbtouch->irq, GFP_NOIO);
> > + mutex_unlock(&usbtouch->lock);
> > + return rv;
> > +}
> > +
> > +static int usbtouch_pre_reset(struct usb_interface *intf)
> > +{
> > + struct usbtouch_usb *usbtouch = usb_get_intfdata(intf);
> > +
> > + dbg("%s - called", __FUNCTION__);
> > +
> > + mutex_lock(&usbtouch->lock);
> > + usb_kill_urb(usbtouch->irq);
> > + return 0;
> > +}
> > +
> > +static int usbtouch_post_reset(struct usb_interface *intf)
> > +{
> > + struct usbtouch_usb *usbtouch = usb_get_intfdata(intf);
> > + int rv = 0;
> > +
> > + dbg("%s - called", __FUNCTION__);
> > +
> > + if (usbtouch->open)
> > + rv = usb_submit_urb(usbtouch->irq, GFP_NOIO);
> > + mutex_unlock(&usbtouch->lock);
> > + return rv;
> > +}
>
> Hmm. Are pre_reset and post_reset actually required in such simple cases?
> AFAICS device reset is already protected against pm and autopm. Or is
> resubmitting urbs necessary after a reset? I'm mainly interested since
> I didn't implement these callbacks in my ati_remote2 patch.
If somebody resets the device you can assume it was in a bad shape.
It's much safer to cleanly resubmit.
> > MODULE_DEVICE_TABLE(usb, usbtouch_devices);
> >
> > static struct usb_driver usbtouch_driver = {
> > .name = "usbtouchscreen",
> > .probe = usbtouch_probe,
> > .disconnect = usbtouch_disconnect,
> > + .suspend = usbtouch_suspend,
> > + .resume = usbtouch_resume,
> > + .reset_resume = usbtouch_resume,
>
> What is the purpose of providing identical resume and reset_resume?
> AFAICS reset_resume is only called if USB_QUIRK_RESET_RESUME is
> enabled.
It is also called if you resume from STD if power was lost.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
