autosuspend for appletouch driver

autosuspend for appletouch driver

[Oliver Neukum]

Hi,

here's the first version of a patch to do autosuspend for the appletouch
driver. It is against 2.6.23-rc6. It compiles. More I can't guarantee.

	Regards
		Oliver

-----
--- a/drivers/input/mouse/appletouch.c	2007-09-20 14:14:46.000000000 +0200
+++ b/drivers/input/mouse/appletouch.c	2007-09-21 10:39:21.000000000 +0200
@@ -140,6 +140,7 @@ MODULE_DEVICE_TABLE (usb, atp_table);
 struct atp {
 	char			phys[64];
 	struct usb_device *	udev;		/* usb device */
+	struct usb_interface	intf;		/* usb interface */
 	struct urb *		urb;		/* usb request block */
 	signed char *		data;		/* transferred data */
 	int			open;		/* non-zero if opened */
@@ -324,6 +325,11 @@ static inline void atp_report_fingers(st
 	input_report_key(input, BTN_TOOL_TRIPLETAP, fingers > 2);
 }
 
+static void atp_mark_busy(struct atp *dev)
+{
+	usb_mark_last_busy(dev->udev);
+}
+
 static void atp_complete(struct urb* urb)
 {
 	int x, y, x_z, y_z, x_f, y_f;
@@ -362,6 +368,9 @@ static void atp_complete(struct urb* urb
 		goto exit;
 	}
 
+	/* mark busy for autosuspend purposes */
+	atp_mark_busy(dev);
+
 	/* reorder the sensors values */
 	if (atp_is_geyser_3(dev)) {
 		memset(dev->xy_cur, 0, sizeof(dev->xy_cur));
@@ -528,12 +537,20 @@ exit:
 static int atp_open(struct input_dev *input)
 {
 	struct atp *dev = input_get_drvdata(input);
+	int rv = 0;
 
-	if (usb_submit_urb(dev->urb, GFP_ATOMIC))
+	if (usb_autopm_get_interface(dev->intf) < 0)
 		return -EIO;
+	dev->intf->needs_remote_wakeup = 1;
+	if (usb_submit_urb(dev->urb, GFP_KERNEL)) {
+		rv = -EIO;
+		dev->intf->needs_remote_wakeup = 0;
+		goto err;
 
 	dev->open = 1;
-	return 0;
+err:
+	usb_autopm_put_interface(dev->intf);
+	return rv;
 }
 
 static void atp_close(struct input_dev *input)
@@ -543,6 +560,7 @@ static void atp_close(struct input_dev *
 	usb_kill_urb(dev->urb);
 	cancel_work_sync(&dev->work);
 	dev->open = 0;
+	dev->intf->needs_remote_wakeup = 0;
 }
 
 static int atp_probe(struct usb_interface *iface, const struct usb_device_id *id)
@@ -580,6 +598,7 @@ static int atp_probe(struct usb_interfac
 	}
 
 	dev->udev = udev;
+	dev->intf = iface;
 	dev->input = input_dev;
 	dev->overflowwarn = 0;
 	if (atp_is_geyser_3(dev))
@@ -714,7 +733,7 @@ static int atp_resume(struct usb_interfa
 {
 	struct atp *dev = usb_get_intfdata(iface);
 
-	if (dev->open && usb_submit_urb(dev->urb, GFP_ATOMIC))
+	if (dev->open && usb_submit_urb(dev->urb, GFP_NOIO))
 		return -EIO;
 
 	return 0;
@@ -727,6 +746,7 @@ static struct usb_driver atp_driver = {
 	.suspend	= atp_suspend,
 	.resume		= atp_resume,
 	.id_table	= atp_table,
+	.supports_autosuspend = 1,
 };
 
 static int __init atp_init(void)

Re: Fwd: [linux-usb-devel] autosuspend for appletouch driver

[Oliver Neukum]

Am Mittwoch 26 September 2007 schrieb Dmitry Torokhov:
> Guys,
> 
> Have you seen this? Is anyone willingto give this patch a try?

And promptly I see a bug. Please try this code. I don't understand why the
original even compiled.

	Regards
		Oliver
----

--- a/drivers/input/mouse/appletouch.c	2007-09-20 14:14:46.000000000 +0200
+++ b/drivers/input/mouse/appletouch.c	2007-09-27 08:22:13.000000000 +0200
@@ -140,6 +140,7 @@ MODULE_DEVICE_TABLE (usb, atp_table);
 struct atp {
 	char			phys[64];
 	struct usb_device *	udev;		/* usb device */
+	struct usb_interface *	intf;		/* usb interface */
 	struct urb *		urb;		/* usb request block */
 	signed char *		data;		/* transferred data */
 	int			open;		/* non-zero if opened */
@@ -324,6 +325,11 @@ static inline void atp_report_fingers(st
 	input_report_key(input, BTN_TOOL_TRIPLETAP, fingers > 2);
 }
 
+static void atp_mark_busy(struct atp *dev)
+{
+	usb_mark_last_busy(dev->udev);
+}
+
 static void atp_complete(struct urb* urb)
 {
 	int x, y, x_z, y_z, x_f, y_f;
@@ -362,6 +368,9 @@ static void atp_complete(struct urb* urb
 		goto exit;
 	}
 
+	/* mark busy for autosuspend purposes */
+	atp_mark_busy(dev);
+
 	/* reorder the sensors values */
 	if (atp_is_geyser_3(dev)) {
 		memset(dev->xy_cur, 0, sizeof(dev->xy_cur));
@@ -528,12 +537,20 @@ exit:
 static int atp_open(struct input_dev *input)
 {
 	struct atp *dev = input_get_drvdata(input);
+	int rv = 0;
 
-	if (usb_submit_urb(dev->urb, GFP_ATOMIC))
+	if (usb_autopm_get_interface(dev->intf) < 0)
 		return -EIO;
+	dev->intf->needs_remote_wakeup = 1;
+	if (usb_submit_urb(dev->urb, GFP_KERNEL)) {
+		rv = -EIO;
+		dev->intf->needs_remote_wakeup = 0;
+		goto err;
 
 	dev->open = 1;
-	return 0;
+err:
+	usb_autopm_put_interface(dev->intf);
+	return rv;
 }
 
 static void atp_close(struct input_dev *input)
@@ -543,6 +560,7 @@ static void atp_close(struct input_dev *
 	usb_kill_urb(dev->urb);
 	cancel_work_sync(&dev->work);
 	dev->open = 0;
+	dev->intf->needs_remote_wakeup = 0;
 }
 
 static int atp_probe(struct usb_interface *iface, const struct usb_device_id *id)
@@ -580,6 +598,7 @@ static int atp_probe(struct usb_interfac
 	}
 
 	dev->udev = udev;
+	dev->intf = iface;
 	dev->input = input_dev;
 	dev->overflowwarn = 0;
 	if (atp_is_geyser_3(dev))
@@ -714,7 +733,7 @@ static int atp_resume(struct usb_interfa
 {
 	struct atp *dev = usb_get_intfdata(iface);
 
-	if (dev->open && usb_submit_urb(dev->urb, GFP_ATOMIC))
+	if (dev->open && usb_submit_urb(dev->urb, GFP_NOIO))
 		return -EIO;
 
 	return 0;
@@ -727,6 +746,7 @@ static struct usb_driver atp_driver = {
 	.suspend	= atp_suspend,
 	.resume		= atp_resume,
 	.id_table	= atp_table,
+	.supports_autosuspend = 1,
 };
 
 static int __init atp_init(void)

Re: [linux-usb-devel] Fwd: autosuspend for appletouch driver

[Alan Stern]

On Thu, 27 Sep 2007, Oliver Neukum wrote:

> @@ -528,12 +537,20 @@ exit:
>  static int atp_open(struct input_dev *input)
>  {
>  	struct atp *dev = input_get_drvdata(input);
> +	int rv = 0;
>  
> -	if (usb_submit_urb(dev->urb, GFP_ATOMIC))
> +	if (usb_autopm_get_interface(dev->intf) < 0)
>  		return -EIO;
> +	dev->intf->needs_remote_wakeup = 1;
> +	if (usb_submit_urb(dev->urb, GFP_KERNEL)) {
> +		rv = -EIO;
> +		dev->intf->needs_remote_wakeup = 0;
> +		goto err;
>  
>  	dev->open = 1;
> -	return 0;
> +err:
> +	usb_autopm_put_interface(dev->intf);
> +	return rv;
>  }
>  
>  static void atp_close(struct input_dev *input)
> @@ -543,6 +560,7 @@ static void atp_close(struct input_dev *
>  	usb_kill_urb(dev->urb);
>  	cancel_work_sync(&dev->work);
>  	dev->open = 0;
> +	dev->intf->needs_remote_wakeup = 0;
>  }

Doesn't atp_close() need to call usb_autopm_put_interface(), to balance 
the usb_autopm_get_interface() call in atp_open()?

Alan Stern

Re: Fwd: autosuspend for appletouch driver

[Oliver Neukum]

Am Donnerstag 27 September 2007 schrieb Alan Stern:
> On Thu, 27 Sep 2007, Oliver Neukum wrote:
> 
> > @@ -528,12 +537,20 @@ exit:
> >  static int atp_open(struct input_dev *input)
> >  {
> >  	struct atp *dev = input_get_drvdata(input);
> > +	int rv = 0;
> >  
> > -	if (usb_submit_urb(dev->urb, GFP_ATOMIC))
> > +	if (usb_autopm_get_interface(dev->intf) < 0)
> >  		return -EIO;
> > +	dev->intf->needs_remote_wakeup = 1;
> > +	if (usb_submit_urb(dev->urb, GFP_KERNEL)) {
> > +		rv = -EIO;
> > +		dev->intf->needs_remote_wakeup = 0;
> > +		goto err;
> >  
> >  	dev->open = 1;
> > -	return 0;
> > +err:
> > +	usb_autopm_put_interface(dev->intf);
> > +	return rv;
> >  }
> >  
> >  static void atp_close(struct input_dev *input)
> > @@ -543,6 +560,7 @@ static void atp_close(struct input_dev *
> >  	usb_kill_urb(dev->urb);
> >  	cancel_work_sync(&dev->work);
> >  	dev->open = 0;
> > +	dev->intf->needs_remote_wakeup = 0;
> >  }
> 
> Doesn't atp_close() need to call usb_autopm_put_interface(), to balance 
> the usb_autopm_get_interface() call in atp_open()?

No, the get is balanced in atp_open() itself. The patch uses the last_busy
mechanism the same way the version for generic hid does. In fact, it is
only necessary because there's no nice API call for setting
needs_remote_wakeup.

	Regards
		Oliver

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] Fwd: autosuspend for appletouch driver

[Alan Stern]

On Thu, 27 Sep 2007, Oliver Neukum wrote:

> Am Donnerstag 27 September 2007 schrieb Alan Stern:
> > On Thu, 27 Sep 2007, Oliver Neukum wrote:
> > 
> > > @@ -528,12 +537,20 @@ exit:
> > >  static int atp_open(struct input_dev *input)
> > >  {
> > >  	struct atp *dev = input_get_drvdata(input);
> > > +	int rv = 0;
> > >  
> > > -	if (usb_submit_urb(dev->urb, GFP_ATOMIC))
> > > +	if (usb_autopm_get_interface(dev->intf) < 0)
> > >  		return -EIO;
> > > +	dev->intf->needs_remote_wakeup = 1;
> > > +	if (usb_submit_urb(dev->urb, GFP_KERNEL)) {
> > > +		rv = -EIO;
> > > +		dev->intf->needs_remote_wakeup = 0;
> > > +		goto err;
> > >  
> > >  	dev->open = 1;
> > > -	return 0;
> > > +err:
> > > +	usb_autopm_put_interface(dev->intf);
> > > +	return rv;
> > >  }
> > >  
> > >  static void atp_close(struct input_dev *input)
> > > @@ -543,6 +560,7 @@ static void atp_close(struct input_dev *
> > >  	usb_kill_urb(dev->urb);
> > >  	cancel_work_sync(&dev->work);
> > >  	dev->open = 0;
> > > +	dev->intf->needs_remote_wakeup = 0;
> > >  }
> > 
> > Doesn't atp_close() need to call usb_autopm_put_interface(), to balance 
> > the usb_autopm_get_interface() call in atp_open()?
> 
> No, the get is balanced in atp_open() itself.

I see; I misread the patch.

>  The patch uses the last_busy
> mechanism the same way the version for generic hid does. In fact, it is
> only necessary because there's no nice API call for setting
> needs_remote_wakeup.

That's okay; even if there were a nice API call for setting 
needs_remote_wakeup you would still have to call
usb_autopm_get_interface().  This is because we don't keep track of 
whether or not remote wakeup is currently enabled on a suspended 
device; we only keep track of whether it should be enabled when the 
next suspend occurs.

Alan Stern

Re: [linux-usb-devel] Fwd: autosuspend for appletouch driver

[Oliver Neukum]

Am Donnerstag 27 September 2007 schrieb Alan Stern:
> > mechanism the same way the version for generic hid does. In fact, it is
> > only necessary because there's no nice API call for setting
> > needs_remote_wakeup.
> 
> That's okay; even if there were a nice API call for setting 
> needs_remote_wakeup you would still have to call
> usb_autopm_get_interface().  This is because we don't keep track of 
> whether or not remote wakeup is currently enabled on a suspended

Is it worth doing so?

	Regards
		Oliver

Re: [linux-usb-devel] Fwd: autosuspend for appletouch driver

[Alan Stern]

On Thu, 27 Sep 2007, Oliver Neukum wrote:

> Am Donnerstag 27 September 2007 schrieb Alan Stern:
> > > mechanism the same way the version for generic hid does. In fact, it is
> > > only necessary because there's no nice API call for setting
> > > needs_remote_wakeup.
> > 
> > That's okay; even if there were a nice API call for setting 
> > needs_remote_wakeup you would still have to call
> > usb_autopm_get_interface().  This is because we don't keep track of 
> > whether or not remote wakeup is currently enabled on a suspended
> 
> Is it worth doing so?

If enough drivers need it.  ("Enough" might mean >= 1.)

Alan Stern