usbtouchscreen: Implement runtime power management

PATCH In review Oliver Neukum 6 participants 8 messages Jun 7, 2010 – Jun 8, 2010 5 messages not in the archive

Subsystems (3)

USB HID Input drivers

lore

Share & tools

lore

Oliver Neukum

author

2010-06-07 13:13 UTC #0 lore raw

PATCH 2/3 usbtouchscreen: Implement runtime power management

Quoted 1 line

From 6a5112cb162bf9075a4ad65692494465f5ced331 Mon Sep 17 00:00:00 2001

From: Oliver Neukum <[email protected]>
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 <[email protected]>

---
 drivers/input/touchscreen/usbtouchscreen.c |   24 +++++++++++++++++++++---
 1 files changed, 21 insertions(+), 3 deletions(-)

drivers/input/touchscreen/usbtouchscreen.c +21 -3

index 6d23c0e..0c2d7c1 100644
@@ -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)

Signature · 6 lines

1.6.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dmitry Torokhov

maintainer

2010-06-07 16:23 UTC #1 lore raw

drivers/input/touchscreen/usbtouchscreen.c exit: · line 1280

Quoted 8 lines · Oliver Neukum

static int usbtouch_open(struct input_dev *input) {

drivers/input/touchscreen/usbtouchscreen.c +2 -0

 	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.

Signature · 1 line

Dmitry

A message that is not in the archive (sent off-list or not yet received): 20100607162316.GB7706-WlK9ik9hQGAhIp7JRqBPierSzoNAToWh@public.gmane.org

Oliver Neukum

2010-06-07 19:31 UTC #3 lore raw

Am Montag, 7. Juni 2010 18:23:16 schrieb Dmitry Torokhov:

Quoted 6 lines · Oliver Neukum

Quoted 5 lines
static int usbtouch_open(struct input_dev *input) {
(quoted diff) +1 -0
 	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
+	int r;

Quoted 6 lines

drivers/input/touchscreen/usbtouchscreen.c exit: · line 1280
Quoted 3 lines
drivers/input/touchscreen/usbtouchscreen.c +1 -0
 	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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 201006072131.18949.oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org

Alan Stern

2010-06-07 20:22 UTC #5 lore raw

Quoted 9 lines · Oliver Neukum

Am Montag, 7. Juni 2010 18:23:16 schrieb Dmitry Torokhov:
Quoted 8 lines · Oliver Neukum
Quoted 7 lines
static int usbtouch_open(struct input_dev *input) {
drivers/input/touchscreen/usbtouchscreen.c +1 -0
 	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
+	int r;
 
 	usbtouch->irq->dev = interface_to_usbdev(usbtouch->interface);

Quoted 8 lines

Quoted 4 lines
drivers/input/touchscreen/usbtouchscreen.c exit: · line 1280
Quoted 1 line
drivers/input/touchscreen/usbtouchscreen.c +1 -0
+	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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): Pine.LNX.4.44L0.1006071622260.1332-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org

Oliver Neukum

2010-06-07 20:47 UTC #7 lore raw

Am Montag, 7. Juni 2010 22:22:54 schrieb Alan Stern:

Quoted 16 lines · Oliver Neukum

Quoted 15 lines
Am Montag, 7. Juni 2010 18:23:16 schrieb Dmitry Torokhov:
Quoted 12 lines · Oliver Neukum
Review
Quoted 8 lines
static int usbtouch_open(struct input_dev *input) {
(quoted diff) +2 -0
 	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).

Quoted 5 lines

Quoted 3 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 201006072247.33068.oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org

Dmitry Torokhov

2010-06-08 08:37 UTC #9 lore raw

Quoted 21 lines · Oliver Neukum

Am Montag, 7. Juni 2010 22:22:54 schrieb Alan Stern:
Quoted 20 lines · Oliver Neukum
Quoted 18 lines
Am Montag, 7. Juni 2010 18:23:16 schrieb Dmitry Torokhov:
Quoted 6 lines · Oliver Neukum
Quoted 5 lines
static int usbtouch_open(struct input_dev *input) {
(quoted diff) +1 -0
 	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
+	int r;
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +1 -0
 	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.

Quoted 6 lines

Quoted 1 line
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.

Signature · 5 lines

Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Oliver Neukum

2010-06-08 10:14 UTC #10 lore raw

Am Dienstag, 8. Juni 2010 10:37:51 schrieb Dmitry Torokhov:

Quoted 11 lines · Oliver Neukum

Quoted 9 lines
Am Montag, 7. Juni 2010 22:22:54 schrieb Alan Stern:
Quoted 3 lines · Oliver Neukum
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.

Quoted 4 lines

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

A message that is not in the archive (sent off-list or not yet received): 20100608083751.GA27348-WlK9ik9hQGAhIp7JRqBPierSzoNAToWh@public.gmane.org

Oliver Neukum

2010-06-08 14:12 UTC #12 lore raw

Am Dienstag, 8. Juni 2010 10:37:51 schrieb Dmitry Torokhov:

Quoted 11 lines · Oliver Neukum

Quoted 5 lines
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

Quoted 1 line

From f4c742fa1ec8a368bbaeb129fb502aecb9cf9208 Mon Sep 17 00:00:00 2001

From: Oliver Neukum <[email protected]>
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 <[email protected]>

---
 drivers/usb/core/usb.h |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

drivers/usb/core/usb.h +12 -0

index cd88220..b070549 100644
@@ -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;

Signature · 6 lines

1.6.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html