[PATCH 2/3] usbtouchscreen: Implement runtime power management

[PATCH 2/3] usbtouchscreen: Implement runtime power management

[Oliver Neukum]

>From 6a5112cb162bf9075a4ad65692494465f5ced331 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>
Date: Mon, 7 Jun 2010 12:55:39 +0200
Subject: [PATCH 2/3] usbtouchscreen: Implement runtime power management

This implement USB autosuspend while the device is opened for
devices that do remote wakeup with a fallback to open/close for
those devices that don't.

Signed-off-by: Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org>
---
 drivers/input/touchscreen/usbtouchscreen.c |   24 +++++++++++++++++++++---
 1 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/usbtouchscreen.c b/drivers/input/touchscreen/usbtouchscreen.c
index 6d23c0e..0c2d7c1 100644
--- a/drivers/input/touchscreen/usbtouchscreen.c
+++ b/drivers/input/touchscreen/usbtouchscreen.c
@@ -1263,6 +1263,7 @@ static void usbtouch_irq(struct urb *urb)
 	usbtouch->type->process_pkt(usbtouch, usbtouch->data, urb->actual_length);
 
 exit:
+	usb_mark_last_busy(interface_to_usbdev(usbtouch->interface));
 	retval = usb_submit_urb(urb, GFP_ATOMIC);
 	if (retval)
 		err("%s - usb_submit_urb failed with result: %d",
@@ -1272,23 +1273,39 @@ exit:
 static int usbtouch_open(struct input_dev *input)
 {
 	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
+	int r;
 
 	usbtouch->irq->dev = interface_to_usbdev(usbtouch->interface);
 
+	r = usb_autopm_get_interface(usbtouch->interface) ? -EIO : 0;
+	if (r < 0)
+		goto out;
+	
 	if (!usbtouch->type->irq_always) {
-		if (usb_submit_urb(usbtouch->irq, GFP_KERNEL))
-		  return -EIO;
+		if (usb_submit_urb(usbtouch->irq, GFP_KERNEL)) {
+			r = -EIO;
+			goto out_put;
+		}
 	}
 
-	return 0;
+	usbtouch->interface->needs_remote_wakeup = 1;
+out_put:
+	usb_autopm_put_interface(usbtouch->interface);
+out:
+	return r;
 }
 
 static void usbtouch_close(struct input_dev *input)
 {
 	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
+	int r;
 
 	if (!usbtouch->type->irq_always)
 		usb_kill_urb(usbtouch->irq);
+	r = usb_autopm_get_interface(usbtouch->interface);
+	usbtouch->interface->needs_remote_wakeup = 0;
+	if (!r)
+		usb_autopm_put_interface(usbtouch->interface);
 }
 
 static int usbtouch_suspend
@@ -1505,6 +1522,7 @@ static struct usb_driver usbtouch_driver = {
 	.suspend	= usbtouch_suspend,
 	.resume		= usbtouch_resume,
 	.id_table	= usbtouch_devices,
+	.supports_autosuspend = 1,
 };
 
 static int __init usbtouch_init(void)
-- 
1.6.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usbtouchscreen: Implement runtime power management

[Dmitry Torokhov]

On Mon, Jun 07, 2010 at 03:13:42PM +0200, Oliver Neukum wrote:
>  static int usbtouch_open(struct input_dev *input)
>  {
>  	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
> +	int r;
>  
>  	usbtouch->irq->dev = interface_to_usbdev(usbtouch->interface);
>  
> +	r = usb_autopm_get_interface(usbtouch->interface) ? -EIO : 0;

Why are we clobbering error code with -EIO? We should propagate te code
returned to us (here and below).

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] usbtouchscreen: Implement runtime power management

[Oliver Neukum]

Am Montag, 7. Juni 2010 18:23:16 schrieb Dmitry Torokhov:
> On Mon, Jun 07, 2010 at 03:13:42PM +0200, Oliver Neukum wrote:
> >  static int usbtouch_open(struct input_dev *input)
> >  {
> >  	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
> > +	int r;
> >  
> >  	usbtouch->irq->dev = interface_to_usbdev(usbtouch->interface);
> >  
> > +	r = usb_autopm_get_interface(usbtouch->interface) ? -EIO : 0;
> 
> Why are we clobbering error code with -EIO? We should propagate te code
> returned to us (here and below).

usb_autopm_get_interface() uses internal USB error codes which with
a few exceptions, do not have the meaning error codes in user space
have.

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usbtouchscreen: Implement runtime power management

[Alan Stern]

On Mon, 7 Jun 2010, Oliver Neukum wrote:

> Am Montag, 7. Juni 2010 18:23:16 schrieb Dmitry Torokhov:
> > On Mon, Jun 07, 2010 at 03:13:42PM +0200, Oliver Neukum wrote:
> > >  static int usbtouch_open(struct input_dev *input)
> > >  {
> > >  	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
> > > +	int r;
> > >  
> > >  	usbtouch->irq->dev = interface_to_usbdev(usbtouch->interface);
> > >  
> > > +	r = usb_autopm_get_interface(usbtouch->interface) ? -EIO : 0;
> > 
> > Why are we clobbering error code with -EIO? We should propagate te code
> > returned to us (here and below).
> 
> usb_autopm_get_interface() uses internal USB error codes which with
> a few exceptions, do not have the meaning error codes in user space
> have.

But other drivers don't do this.  What's special about usbtouch?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usbtouchscreen: Implement runtime power management

[Oliver Neukum]

Am Montag, 7. Juni 2010 22:22:54 schrieb Alan Stern:
> On Mon, 7 Jun 2010, Oliver Neukum wrote:
> 
> > Am Montag, 7. Juni 2010 18:23:16 schrieb Dmitry Torokhov:
> > > On Mon, Jun 07, 2010 at 03:13:42PM +0200, Oliver Neukum wrote:
> > > >  static int usbtouch_open(struct input_dev *input)
> > > >  {
> > > >  	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
> > > > +	int r;
> > > >  
> > > >  	usbtouch->irq->dev = interface_to_usbdev(usbtouch->interface);
> > > >  
> > > > +	r = usb_autopm_get_interface(usbtouch->interface) ? -EIO : 0;
> > > 
> > > Why are we clobbering error code with -EIO? We should propagate te code
> > > returned to us (here and below).
> > 
> > usb_autopm_get_interface() uses internal USB error codes which with
> > a few exceptions, do not have the meaning error codes in user space
> > have.
> 
> But other drivers don't do this.  What's special about usbtouch?

Then I'd say the other drivers are wrong. We cannot leak USB specific
codes. Maybe we should pass -ENOMEM and -ENODEV, but the others
really don't mean anything as generic codes.

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usbtouchscreen: Implement runtime power management

[Dmitry Torokhov]

On Mon, Jun 07, 2010 at 10:47:33PM +0200, Oliver Neukum wrote:
> Am Montag, 7. Juni 2010 22:22:54 schrieb Alan Stern:
> > On Mon, 7 Jun 2010, Oliver Neukum wrote:
> > 
> > > Am Montag, 7. Juni 2010 18:23:16 schrieb Dmitry Torokhov:
> > > > On Mon, Jun 07, 2010 at 03:13:42PM +0200, Oliver Neukum wrote:
> > > > >  static int usbtouch_open(struct input_dev *input)
> > > > >  {
> > > > >  	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
> > > > > +	int r;
> > > > >  
> > > > >  	usbtouch->irq->dev = interface_to_usbdev(usbtouch->interface);
> > > > >  
> > > > > +	r = usb_autopm_get_interface(usbtouch->interface) ? -EIO : 0;
> > > > 
> > > > Why are we clobbering error code with -EIO? We should propagate te code
> > > > returned to us (here and below).
> > > 
> > > usb_autopm_get_interface() uses internal USB error codes which with
> > > a few exceptions, do not have the meaning error codes in user space
> > > have.
> > 
> > But other drivers don't do this.  What's special about usbtouch?
> 
> Then I'd say the other drivers are wrong. We cannot leak USB specific
> codes. Maybe we should pass -ENOMEM and -ENODEV, but the others
> really don't mean anything as generic codes.
> 

No, I'd say that usb_autopm_get_interface() is wrong - since it is
supposed to be used by drivers who are not concerned about USB-specific
codes these functions should not leak them to the callers but rather
provide ones suitable for reporting upstream.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usbtouchscreen: Implement runtime power management

[Oliver Neukum]

Am Dienstag, 8. Juni 2010 10:37:51 schrieb Dmitry Torokhov:
> On Mon, Jun 07, 2010 at 10:47:33PM +0200, Oliver Neukum wrote:
> > Am Montag, 7. Juni 2010 22:22:54 schrieb Alan Stern:
> > > On Mon, 7 Jun 2010, Oliver Neukum wrote:

> > > But other drivers don't do this.  What's special about usbtouch?
> > 
> > Then I'd say the other drivers are wrong. We cannot leak USB specific
> > codes. Maybe we should pass -ENOMEM and -ENODEV, but the others
> > really don't mean anything as generic codes.
> > 
> 
> No, I'd say that usb_autopm_get_interface() is wrong - since it is
> supposed to be used by drivers who are not concerned about USB-specific
> codes these functions should not leak them to the callers but rather
> provide ones suitable for reporting upstream.

But the driver may need to know why its request failed to handle errors
properly, for example it makes no sense to reset a device if you get -ENODEV.

	Regards
		Oliver

Re: [PATCH 2/3] usbtouchscreen: Implement runtime power management

[Oliver Neukum]

Am Dienstag, 8. Juni 2010 10:37:51 schrieb Dmitry Torokhov:
> On Mon, Jun 07, 2010 at 10:47:33PM +0200, Oliver Neukum wrote:
>
> > Then I'd say the other drivers are wrong. We cannot leak USB specific
> > codes. Maybe we should pass -ENOMEM and -ENODEV, but the others
> > really don't mean anything as generic codes.
> > 
> 
> No, I'd say that usb_autopm_get_interface() is wrong - since it is
> supposed to be used by drivers who are not concerned about USB-specific
> codes these functions should not leak them to the callers but rather
> provide ones suitable for reporting upstream.

How about a wrapper like this?

	Regards
		Oliver

>From f4c742fa1ec8a368bbaeb129fb502aecb9cf9208 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>
Date: Tue, 8 Jun 2010 16:02:51 +0200
Subject: [PATCH] usb: add helper for error code conversion

USB uses error codes in a particular manner. Such codes must
not be passed to user space as that violates POSIX. This helper
maps the error codes to error codes that may be passed to
user space

Signed-off-by: Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org>
---
 drivers/usb/core/usb.h |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index cd88220..b070549 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -124,6 +124,18 @@ static inline int is_usb_device_driver(struct device_driver *drv)
 			for_devices;
 }
 
+/* translate USB error codes to codes user space understands */
+static inline int usb_translate_errors(int error_code)
+{
+	switch (error_code) {
+	case 0:
+	case -ENOMEM:
+	case -ENODEV:
+		return error_code;
+	default:
+		return -EIO;
+	}
+}
 
 /* for labeling diagnostics */
 extern const char *usbcore_name;
-- 
1.6.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] usbtouchscreen: Implement runtime power management

[Oliver Neukum]

>From 71b07110871d7b9e754a3b56f3e914745c329be6 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>
Date: Thu, 24 Jun 2010 08:38:57 +0200
Subject: [PATCH 2/3] usbtouchscreen: Implement runtime power management

This implement USB autosuspend while the device is opened for
devices that do remote wakeup with a fallback to open/close for
those devices that don't. Devices that require the host to
constantly poll them are never autosuspended.

Signed-off-by: Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org>
---
 drivers/input/touchscreen/usbtouchscreen.c |   27 ++++++++++++++++++++++++---
 1 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/usbtouchscreen.c b/drivers/input/touchscreen/usbtouchscreen.c
index f51d2fd..3c57df8 100644
--- a/drivers/input/touchscreen/usbtouchscreen.c
+++ b/drivers/input/touchscreen/usbtouchscreen.c
@@ -1263,6 +1263,7 @@ static void usbtouch_irq(struct urb *urb)
 	usbtouch->type->process_pkt(usbtouch, usbtouch->data, urb->actual_length);
 
 exit:
+	usb_mark_last_busy(interface_to_usbdev(usbtouch->interface));
 	retval = usb_submit_urb(urb, GFP_ATOMIC);
 	if (retval)
 		err("%s - usb_submit_urb failed with result: %d",
@@ -1272,23 +1273,39 @@ exit:
 static int usbtouch_open(struct input_dev *input)
 {
 	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
+	int r;
 
 	usbtouch->irq->dev = interface_to_usbdev(usbtouch->interface);
 
+	r = usb_autopm_get_interface(usbtouch->interface) ? -EIO : 0;
+	if (r < 0)
+		goto out;
+	
 	if (!usbtouch->type->irq_always) {
-		if (usb_submit_urb(usbtouch->irq, GFP_KERNEL))
-		  return -EIO;
+		if (usb_submit_urb(usbtouch->irq, GFP_KERNEL)) {
+			r = -EIO;
+			goto out_put;
+		}
 	}
 
-	return 0;
+	usbtouch->interface->needs_remote_wakeup = 1;
+out_put:
+	usb_autopm_put_interface(usbtouch->interface);
+out:
+	return r;
 }
 
 static void usbtouch_close(struct input_dev *input)
 {
 	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
+	int r;
 
 	if (!usbtouch->type->irq_always)
 		usb_kill_urb(usbtouch->irq);
+	r = usb_autopm_get_interface(usbtouch->interface);
+	usbtouch->interface->needs_remote_wakeup = 0;
+	if (!r)
+		usb_autopm_put_interface(usbtouch->interface);
 }
 
 static int usbtouch_suspend
@@ -1450,8 +1467,11 @@ static int usbtouch_probe(struct usb_interface *intf,
 	usb_set_intfdata(intf, usbtouch);
 
 	if (usbtouch->type->irq_always) {
+		/* this can't fail */
+		usb_autopm_get_interface(intf);
 		err = usb_submit_urb(usbtouch->irq, GFP_KERNEL);
 		if (err) {
+			usb_autopm_put_interface(intf);
 			err("%s - usb_submit_urb failed with result: %d",
 			    __func__, err);
 			goto out_unregister_input;
@@ -1505,6 +1525,7 @@ static struct usb_driver usbtouch_driver = {
 	.suspend	= usbtouch_suspend,
 	.resume		= usbtouch_resume,
 	.id_table	= usbtouch_devices,
+	.supports_autosuspend = 1,
 };
 
 static int __init usbtouch_init(void)
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html