[rft]power management for usbtouch

[rft]power management for usbtouch

[Oliver Neukum]

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);
+	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);
 }
 
+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;
+}
+
+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;
+}
+
 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,
+	.pre_reset	= usbtouch_pre_reset,
+	.post_reset	= usbtouch_post_reset,
 	.id_table	= usbtouch_devices,
+	.supports_autosuspend = 1,
 };
 
 static int __init usbtouch_init(void)
--
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: [rft]power management for usbtouch

[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.

> +	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 :)

> +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.

> +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.

>  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.

> +	.pre_reset	= usbtouch_pre_reset,
> +	.post_reset	= usbtouch_post_reset,
>  	.id_table	= usbtouch_devices,
> +	.supports_autosuspend = 1,
>  };
>  
>  static int __init usbtouch_init(void)

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [rft]power management for usbtouch

[Oliver Neukum]

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-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: [rft]power management for usbtouch

[Oliver Neukum]

Hi,

this patch should fix everything you complained about. As I don't have
the hardware, please test.

	Regards
		Oliver

---

--- linux-2.6.26-sierra/drivers/input/touchscreen/usbtouchscreen.c.alt	2008-06-26 15:23:34.000000000 +0200
+++ linux-2.6.26-sierra/drivers/input/touchscreen/usbtouchscreen.c	2008-06-27 10:38:49.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,20 +824,34 @@ 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)
 {
 	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
 
+	mutex_lock(&usbtouch->lock);
+	usbtouch->open = 0;
+	mutex_unlock(&usbtouch->lock);
 	usb_kill_urb(usbtouch->irq);
+	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,75 @@ static void usbtouch_disconnect(struct u
 
 	dbg("%s - usbtouch is initialized, cleaning up", __FUNCTION__);
 	usb_set_intfdata(intf, NULL);
-	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);
 }
 
+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;
+}
+
+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;
+}
+
 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,
+	.pre_reset	= usbtouch_pre_reset,
+	.post_reset	= usbtouch_post_reset,
 	.id_table	= usbtouch_devices,
+	.supports_autosuspend = 1,
 };
 
 static int __init usbtouch_init(void)
--
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: [rft]power management for usbtouch

[Ville Syrjälä]

On Fri, Jun 27, 2008 at 10:50:00AM +0200, Oliver Neukum wrote:
> Hi,
> 
> this patch should fix everything you complained about. As I don't have
> the hardware, please test.

I don't have the hardware either so can't help you there.
  
>  static void usbtouch_close(struct input_dev *input)
>  {
>  	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
>  
> +	mutex_lock(&usbtouch->lock);
> +	usbtouch->open = 0;
> +	mutex_unlock(&usbtouch->lock);
>  	usb_kill_urb(usbtouch->irq);

usb_kill_urb() is still not protected by the mutex but a resume
happening here wouldn't resubmit the urb so I suppose it's OK to leave
it like that.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [rft]power management for usbtouch

[Ville Syrjälä]

On Thu, Jun 26, 2008 at 04:56:05PM +0200, Oliver Neukum wrote:
> 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:
> > >  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.

That's the case with USB_PERSIST and persist_enabled turned on right?
So the core will simply disconnect+reconnect the device if persistance
is not enabled (or if reset_resume is not provided). Hmm. I suppose it's
better to have a reset_resume than do the disconnect+reconnect trick
since then the input device will not disappear, making it more friendly
towards non-hotlug aware applications.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [rft]power management for usbtouch

[Ville Syrjälä]

On Fri, Jun 27, 2008 at 10:50:00AM +0200, Oliver Neukum wrote:
> @@ -820,20 +824,34 @@ 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;
>  }

What if the device is already suspended when open() is called?

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [rft]power management for usbtouch

[Oliver Neukum]

Am Freitag 27 Juni 2008 18:10:36 schrieb Ville Syrjälä:
> On Fri, Jun 27, 2008 at 10:50:00AM +0200, Oliver Neukum wrote:
> > @@ -820,20 +824,34 @@ 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;
> >  }
> 
> What if the device is already suspended when open() is called?
> 

It cannot be. usb_autopm_get_interface() is called, which will resume
the device.

	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: [rft]power management for usbtouch

[Ville Syrjälä]

On Fri, Jun 27, 2008 at 09:07:13PM +0200, Oliver Neukum wrote:
> Am Freitag 27 Juni 2008 18:10:36 schrieb Ville Syrjälä:
> > On Fri, Jun 27, 2008 at 10:50:00AM +0200, Oliver Neukum wrote:
> > > @@ -820,20 +824,34 @@ 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;
> > >  }
> > 
> > What if the device is already suspended when open() is called?
> > 
> 
> It cannot be. usb_autopm_get_interface() is called, which will resume
> the device.

Ah, so it resumes the device even if it was not autosuspended but
suspended for another reason?

If I understood correctly usb_autpm_get_interface() will only prevent
further autosuspend requests. But is there anything protecting this
against another (non-auto) suspend request coming in between
usb_autopm_get_interface() and mutex_lock()?

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [rft]power management for usbtouch

[Oliver Neukum]

Am Freitag 27 Juni 2008 22:20:27 schrieb Ville Syrjälä:
> On Fri, Jun 27, 2008 at 09:07:13PM +0200, Oliver Neukum wrote:
> > Am Freitag 27 Juni 2008 18:10:36 schrieb Ville Syrjälä:
> > > On Fri, Jun 27, 2008 at 10:50:00AM +0200, Oliver Neukum wrote:
> > > > @@ -820,20 +824,34 @@ 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;
> > > >  }
> > > 
> > > What if the device is already suspended when open() is called?
> > > 
> > 
> > It cannot be. usb_autopm_get_interface() is called, which will resume
> > the device.
> 
> Ah, so it resumes the device even if it was not autosuspended but
> suspended for another reason?

The other reason is either system wide, in which case nobody can call open,
or it is forcibly suspended via sysfs, causing usb_submit_urb() to fail later
on.
	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: [rft]power management for usbtouch

[Ville Syrjälä]

On Fri, Jun 27, 2008 at 10:41:20PM +0200, Oliver Neukum wrote:
> Am Freitag 27 Juni 2008 22:20:27 schrieb Ville Syrjälä:
> > On Fri, Jun 27, 2008 at 09:07:13PM +0200, Oliver Neukum wrote:
> > > Am Freitag 27 Juni 2008 18:10:36 schrieb Ville Syrjälä:
> > > > On Fri, Jun 27, 2008 at 10:50:00AM +0200, Oliver Neukum wrote:
> > > > > @@ -820,20 +824,34 @@ 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;
> > > > >  }
> > > > 
> > > > What if the device is already suspended when open() is called?
> > > > 
> > > 
> > > It cannot be. usb_autopm_get_interface() is called, which will resume
> > > the device.
> > 
> > Ah, so it resumes the device even if it was not autosuspended but
> > suspended for another reason?
> 
> The other reason is either system wide, in which case nobody can call open,
> or it is forcibly suspended via sysfs, causing usb_submit_urb() to fail later
> on.

Which will also fail the open() itself. Isn't that bad? That is, you can
suspend an already open device without the users the device noticing
but you can't open it if you happen to suspend it at just the right time.
That doesn't strike me as good behaviour for a driver.

-- 
Ville Syrjälä
syrjala-ORSVBvAovxo@public.gmane.org
http://www.sci.fi/~syrjala/
--
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: [rft]power management for usbtouch

[Oliver Neukum]

Am Freitag 27 Juni 2008 22:51:47 schrieben Sie:
> > > Ah, so it resumes the device even if it was not autosuspended but
> > > suspended for another reason?
> > 
> > The other reason is either system wide, in which case nobody can call open,
> > or it is forcibly suspended via sysfs, causing usb_submit_urb() to fail later
> > on.
> 
> Which will also fail the open() itself. Isn't that bad? That is, you can
> suspend an already open device without the users the device noticing
> but you can't open it if you happen to suspend it at just the right time.
> That doesn't strike me as good behaviour for a driver.

Root can also unbind the driver. There is no behavior pleasing everybody.

	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: [rft]power management for usbtouch

[Ville Syrjälä]

On Fri, Jun 27, 2008 at 10:57:39PM +0200, Oliver Neukum wrote:
> Am Freitag 27 Juni 2008 22:51:47 schrieben Sie:
> > > > Ah, so it resumes the device even if it was not autosuspended but
> > > > suspended for another reason?
> > > 
> > > The other reason is either system wide, in which case nobody can call open,
> > > or it is forcibly suspended via sysfs, causing usb_submit_urb() to fail later
> > > on.
> > 
> > Which will also fail the open() itself. Isn't that bad? That is, you can
> > suspend an already open device without the users the device noticing
> > but you can't open it if you happen to suspend it at just the right time.
> > That doesn't strike me as good behaviour for a driver.
> 
> Root can also unbind the driver.

Sure, but in that case the behavior is consistent ie. the device
disappears.

> There is no behavior pleasing everybody.

I would just prefer all drivers of the same subsystem to behave in a
consistent manner. It would even make sense to document the desired
behavior so future driver writers wouldn't have to come up with their
own private rules for such things.

As it stands the open can fail in seemingly random fashion which would
be bad for the users. And my opinion (if it counts) is to allow the open
to succeed for suspended devices since already opened devices would not
notice the suspend either. I don't think there's any downside to handling
this issue apart from adding a couple of lines of code.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [rft]power management for usbtouch

[Oliver Neukum]

Am Freitag 27 Juni 2008 23:28:30 schrieb Ville Syrjälä:
> > > Which will also fail the open() itself. Isn't that bad? That is, you can
> > > suspend an already open device without the users the device noticing
> > > but you can't open it if you happen to suspend it at just the right time.
> > > That doesn't strike me as good behaviour for a driver.
> > 
> > Root can also unbind the driver.
> 
> Sure, but in that case the behavior is consistent ie. the device
> disappears.

Sorry, I chose a bad example. Root can do stupid things like sending
control requests that change the configuration, setting or address of
a device. The effects are undefined and will differ from driver to driver
and open or closedness.
Forcibly suspending a device is a testing feature. If root abuses it,
he'll have to take the consequences.

	Regards
		Oliver

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

Re: [rft]power management for usbtouch

[Ville Syrjälä]

On Sat, Jun 28, 2008 at 12:52:38PM +0200, Oliver Neukum wrote:
> Am Freitag 27 Juni 2008 23:28:30 schrieb Ville Syrjälä:
> > > > Which will also fail the open() itself. Isn't that bad? That is, you can
> > > > suspend an already open device without the users the device noticing
> > > > but you can't open it if you happen to suspend it at just the right time.
> > > > That doesn't strike me as good behaviour for a driver.
> > > 
> > > Root can also unbind the driver.
> > 
> > Sure, but in that case the behavior is consistent ie. the device
> > disappears.
> 
> Sorry, I chose a bad example. Root can do stupid things like sending
> control requests that change the configuration, setting or address of
> a device. The effects are undefined and will differ from driver to driver
> and open or closedness.
> Forcibly suspending a device is a testing feature. If root abuses it,
> he'll have to take the consequences.

Typically when you're testing something you expect a certain result. The
code as it stands adds another very subtle failure point.

I don't quite understand why you're so against handling this issue.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [rft]power management for usbtouch

[Oliver Neukum]

Am Samstag 28 Juni 2008 13:22:58 schrieb Ville Syrjälä:
> Typically when you're testing something you expect a certain result. The
> code as it stands adds another very subtle failure point.
> 
> I don't quite understand why you're so against handling this issue.

In hindsight adding that interface at all was an error. I was in favor
of it and that was a mistake. Maybe it should be removed.
Complicating good drivers for its sake I therefore consider an error.
Possibly we should consider a forcible suspension a form of disconnection.

	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: [rft]power management for usbtouch

[Alan Stern]

On Sat, 28 Jun 2008, Oliver Neukum wrote:

> In hindsight adding that interface at all was an error. I was in favor
> of it and that was a mistake. Maybe it should be removed.
> Complicating good drivers for its sake I therefore consider an error.
> Possibly we should consider a forcible suspension a form of disconnection.

Are you suggesting that the "off" value in /sys/.../power/level 
shouldn't be allowed?  I don't see it that way -- there's no other 
mechanism for userspace to force a USB device into the low-power state.

Forcing a disconnection during suspend might be a good idea.  But if 
you do, is there any reason to consider manual suspension different 
from system sleep?

Alan Stern

Re: [rft]power management for usbtouch

[Oliver Neukum]

Am Samstag 28 Juni 2008 18:33:30 schrieb Alan Stern:
> On Sat, 28 Jun 2008, Oliver Neukum wrote:
> 
> > In hindsight adding that interface at all was an error. I was in favor
> > of it and that was a mistake. Maybe it should be removed.
> > Complicating good drivers for its sake I therefore consider an error.
> > Possibly we should consider a forcible suspension a form of disconnection.
> 
> Are you suggesting that the "off" value in /sys/.../power/level 
> shouldn't be allowed?  I don't see it that way -- there's no other 
> mechanism for userspace to force a USB device into the low-power state.

Unfortunately.

> Forcing a disconnection during suspend might be a good idea.  But if 
> you do, is there any reason to consider manual suspension different 
> from system sleep?

During system sleep user space is asleep and cannot make demands
on drivers. If you force a device to sleep, the driver cannot work. Therefore
it should not be bound to such a device. It's cleaner to disconnect.

	Regards
		Oliver

Re: [rft]power management for usbtouch

[Dmitry Torokhov]

On Sun, Jun 29, 2008 at 12:47:55AM +0200, Oliver Neukum wrote:
> Am Samstag 28 Juni 2008 18:33:30 schrieb Alan Stern:
> > On Sat, 28 Jun 2008, Oliver Neukum wrote:
> > 
> > > In hindsight adding that interface at all was an error. I was in favor
> > > of it and that was a mistake. Maybe it should be removed.
> > > Complicating good drivers for its sake I therefore consider an error.
> > > Possibly we should consider a forcible suspension a form of disconnection.
> > 
> > Are you suggesting that the "off" value in /sys/.../power/level 
> > shouldn't be allowed?  I don't see it that way -- there's no other 
> > mechanism for userspace to force a USB device into the low-power state.
> 
> Unfortunately.
> 
> > Forcing a disconnection during suspend might be a good idea.  But if 
> > you do, is there any reason to consider manual suspension different 
> > from system sleep?
> 
> During system sleep user space is asleep and cannot make demands
> on drivers. If you force a device to sleep, the driver cannot work. Therefore
> it should not be bound to such a device. It's cleaner to disconnect.

I'd agree with this. Otherwise we need somehow to be able to resync
the state of the device (like have LEDs and repeat rate reset,
re-upload force-feedback effects, etc, etc) once device is resumed.
So... what is the consensus? Do we need to worry about manual
device-level suspension or it will be reworked/removed?

-- 
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: [rft]power management for usbtouch

[Alan Stern]

On Mon, 30 Jun 2008, Dmitry Torokhov wrote:

> > > Forcing a disconnection during suspend might be a good idea.  But if 
> > > you do, is there any reason to consider manual suspension different 
> > > from system sleep?
> > 
> > During system sleep user space is asleep and cannot make demands
> > on drivers. If you force a device to sleep, the driver cannot work. Therefore
> > it should not be bound to such a device. It's cleaner to disconnect.
> 
> I'd agree with this. Otherwise we need somehow to be able to resync
> the state of the device (like have LEDs and repeat rate reset,
> re-upload force-feedback effects, etc, etc) once device is resumed.
> So... what is the consensus? Do we need to worry about manual
> device-level suspension or it will be reworked/removed?

My feeling is that we will leave manual suspension in the kernel.  The
code passed to the USB driver's suspend method will be changed so that
the method can tell whether it is being called for a system sleep vs.
an autosuspend vs. a manual suspend.

Alan Stern

Re: [rft]power management for usbtouch

[Dmitry Torokhov]

On Mon, Jun 30, 2008 at 11:47:28AM -0400, Alan Stern wrote:
> On Mon, 30 Jun 2008, Dmitry Torokhov wrote:
> 
> > > > Forcing a disconnection during suspend might be a good idea.  But if 
> > > > you do, is there any reason to consider manual suspension different 
> > > > from system sleep?
> > > 
> > > During system sleep user space is asleep and cannot make demands
> > > on drivers. If you force a device to sleep, the driver cannot work. Therefore
> > > it should not be bound to such a device. It's cleaner to disconnect.
> > 
> > I'd agree with this. Otherwise we need somehow to be able to resync
> > the state of the device (like have LEDs and repeat rate reset,
> > re-upload force-feedback effects, etc, etc) once device is resumed.
> > So... what is the consensus? Do we need to worry about manual
> > device-level suspension or it will be reworked/removed?
> 
> My feeling is that we will leave manual suspension in the kernel.  The
> code passed to the USB driver's suspend method will be changed so that
> the method can tell whether it is being called for a system sleep vs.
> an autosuspend vs. a manual suspend.
> 

Ok, I see... So what does a device do when it is suspended and a
request comes in? Does it automatically resume and service the request
(and potentially goes back to sleep)? Does it queue the request for
later? Ignore it? Is it a system-wide policy or up to the device
[class]?

-- 
Dmitry

Re: [rft]power management for usbtouch

[Alan Stern]

On Mon, 30 Jun 2008, Dmitry Torokhov wrote:

> > My feeling is that we will leave manual suspension in the kernel.  The
> > code passed to the USB driver's suspend method will be changed so that
> > the method can tell whether it is being called for a system sleep vs.
> > an autosuspend vs. a manual suspend.
> > 
> 
> Ok, I see... So what does a device do when it is suspended and a
> request comes in? Does it automatically resume and service the request
> (and potentially goes back to sleep)? Does it queue the request for
> later? Ignore it? Is it a system-wide policy or up to the device
> [class]?

What sort of request do you mean?  If a device is suspended, it _can't_ 
receive any data (requests or otherwise) over the USB bus.

If a device receives data over some other channel (like a mouse getting
a button click) while it is suspended, its behavior depends on the
settings at the time it was suspended.  If remote wakeup was disabled
then the device does essentially nothing (maybe it remembers the data
for later, maybe not).  If remote wakeup was enabled then the device
should send a wakeup request to the host.

Remote wakeup is controlled by a setting in sysfs: 
/sys/bus/usb/device/.../power/wakeup.  It is enabled by default for all 
USB devices that support it.

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: [rft]power management for usbtouch

[Dmitry Torokhov]

On Mon, Jun 30, 2008 at 12:16:13PM -0400, Alan Stern wrote:
> On Mon, 30 Jun 2008, Dmitry Torokhov wrote:
> 
> > > My feeling is that we will leave manual suspension in the kernel.  The
> > > code passed to the USB driver's suspend method will be changed so that
> > > the method can tell whether it is being called for a system sleep vs.
> > > an autosuspend vs. a manual suspend.
> > > 
> > 
> > Ok, I see... So what does a device do when it is suspended and a
> > request comes in? Does it automatically resume and service the request
> > (and potentially goes back to sleep)? Does it queue the request for
> > later? Ignore it? Is it a system-wide policy or up to the device
> > [class]?
> 
> What sort of request do you mean?  If a device is suspended, it _can't_ 
> receive any data (requests or otherwise) over the USB bus.
> 
> If a device receives data over some other channel (like a mouse getting
> a button click) while it is suspended, its behavior depends on the
> settings at the time it was suspended.  If remote wakeup was disabled
> then the device does essentially nothing (maybe it remembers the data
> for later, maybe not).  If remote wakeup was enabled then the device
> should send a wakeup request to the host.
> 

I was talking about system-initiated requests... Setting LED on a keyboard,
setting rate on a mouse, uploading a force-feedback effect, writing a
sector on a disk, etc...

-- 
Dmitry

Re: [rft]power management for usbtouch

[Alan Stern]

On Mon, 30 Jun 2008, Dmitry Torokhov wrote:

> On Mon, Jun 30, 2008 at 12:16:13PM -0400, Alan Stern wrote:
> > On Mon, 30 Jun 2008, Dmitry Torokhov wrote:
> > 
> > > > My feeling is that we will leave manual suspension in the kernel.  The
> > > > code passed to the USB driver's suspend method will be changed so that
> > > > the method can tell whether it is being called for a system sleep vs.
> > > > an autosuspend vs. a manual suspend.
> > > > 
> > > 
> > > Ok, I see... So what does a device do when it is suspended and a
> > > request comes in? Does it automatically resume and service the request
> > > (and potentially goes back to sleep)? Does it queue the request for
> > > later? Ignore it? Is it a system-wide policy or up to the device
> > > [class]?
> > 
> > What sort of request do you mean?  If a device is suspended, it _can't_ 
> > receive any data (requests or otherwise) over the USB bus.
> > 
> > If a device receives data over some other channel (like a mouse getting
> > a button click) while it is suspended, its behavior depends on the
> > settings at the time it was suspended.  If remote wakeup was disabled
> > then the device does essentially nothing (maybe it remembers the data
> > for later, maybe not).  If remote wakeup was enabled then the device
> > should send a wakeup request to the host.
> > 
> 
> I was talking about system-initiated requests... Setting LED on a keyboard,
> setting rate on a mouse, uploading a force-feedback effect, writing a
> sector on a disk, etc...

Then I don't understand the original question, since it's impossible 
for the system to send a request to a suspended device.

Did you perhaps intend to ask what a _driver_ does when its device is
suspended and it receives an I/O request?  The action depends on the
reason for the suspend:

	If the device was suspended for a system sleep, the request
	should be deferred until the device is resumed when the
	system wakes up.

	If the device was autosuspended, the driver should autoresume
	the device and then transmit the request.

	If the device was manually suspended, the driver should fail
	the request.

In practice, what is most likely to happen is that the driver will 
always try to autoresume the device.  Then:

	If a system sleep is in progress, the autoresume is supposed to
	block until the system wakes up (but this behavior has not yet 
	been implemented -- generally no requests can arrive during a
	system sleep anyway because userspace is frozen).

	If the device is autosuspended, the autoresume works and the
	driver can transmit the request.

	If the device is manually suspended, the autoresume fails and
	so the driver fails the request.

Alan Stern

Re: [rft]power management for usbtouch

[Oliver Neukum]

Am Montag 30 Juni 2008 18:02:44 schrieb Dmitry Torokhov:
> On Mon, Jun 30, 2008 at 11:47:28AM -0400, Alan Stern wrote:
> > On Mon, 30 Jun 2008, Dmitry Torokhov wrote:
> > 
> > > > > Forcing a disconnection during suspend might be a good idea.  But if 
> > > > > you do, is there any reason to consider manual suspension different 
> > > > > from system sleep?
> > > > 
> > > > During system sleep user space is asleep and cannot make demands
> > > > on drivers. If you force a device to sleep, the driver cannot work. Therefore
> > > > it should not be bound to such a device. It's cleaner to disconnect.
> > > 
> > > I'd agree with this. Otherwise we need somehow to be able to resync
> > > the state of the device (like have LEDs and repeat rate reset,
> > > re-upload force-feedback effects, etc, etc) once device is resumed.
> > > So... what is the consensus? Do we need to worry about manual
> > > device-level suspension or it will be reworked/removed?
> > 
> > My feeling is that we will leave manual suspension in the kernel.  The
> > code passed to the USB driver's suspend method will be changed so that
> > the method can tell whether it is being called for a system sleep vs.
> > an autosuspend vs. a manual suspend.
> > 
> 
> Ok, I see... So what does a device do when it is suspended and a
> request comes in? Does it automatically resume and service the request
> (and potentially goes back to sleep)? Does it queue the request for
> later? Ignore it? Is it a system-wide policy or up to the device
> [class]?

Return an error. If you queue, how much would you queue?
There's no sane answer, so you better fail right away.

	Regards
		Oliver

Re: [rft]power management for usbtouch

[Jiri Kosina]

On Tue, 1 Jul 2008, Oliver Neukum wrote:

>> Ok, I see... So what does a device do when it is suspended and a 
>> request comes in? Does it automatically resume and service the request 
>> (and potentially goes back to sleep)? Does it queue the request for 
>> later? Ignore it? Is it a system-wide policy or up to the device 
>> [class]?
> Return an error. If you queue, how much would you queue?
> There's no sane answer, so you better fail right away.

I guess that Dmitry's question was about the behavior of the driver, 
right? For example in situation when the LED state changes on PS/2 
keyboard, what should the USB keyboard driver do for a suspended USB 
keyboard.

I really don't think we want to return error in such cases. Either resume 
the device, or queue some requests (maybe depending on the type of 
requests), right?

-- 
Jiri Kosina
SUSE Labs
--
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: [rft]power management for usbtouch

[Oliver Neukum]

Am Dienstag 01 Juli 2008 09:59:01 schrieb Jiri Kosina:
> On Tue, 1 Jul 2008, Oliver Neukum wrote:
> 
> >> Ok, I see... So what does a device do when it is suspended and a 
> >> request comes in? Does it automatically resume and service the request 
> >> (and potentially goes back to sleep)? Does it queue the request for 
> >> later? Ignore it? Is it a system-wide policy or up to the device 
> >> [class]?
> > Return an error. If you queue, how much would you queue?
> > There's no sane answer, so you better fail right away.
> 
> I guess that Dmitry's question was about the behavior of the driver, 
> right? For example in situation when the LED state changes on PS/2 
> keyboard, what should the USB keyboard driver do for a suspended USB 
> keyboard.

I understood it that way.

> I really don't think we want to return error in such cases. Either resume 
> the device, or queue some requests (maybe depending on the type of 
> requests), right?

We are talking about forcibly suspended devices, so they cannot be
resumed. We could queue, but where would we end queuing? And we
have to decide to stop it at some point, or we create a DoS problem.

The user has decided to put the device in a state in which the driver
cannot properly function. If the user wanted the driver to work, he should
select autosuspend.

	Regards
		Oliver

Re: [rft]power management for usbtouch

[Dmitry Torokhov]

On Tue, Jul 01, 2008 at 11:08:47AM +0200, Oliver Neukum wrote:
> Am Dienstag 01 Juli 2008 09:59:01 schrieb Jiri Kosina:
> > On Tue, 1 Jul 2008, Oliver Neukum wrote:
> > 
> > >> Ok, I see... So what does a device do when it is suspended and a 
> > >> request comes in? Does it automatically resume and service the request 
> > >> (and potentially goes back to sleep)? Does it queue the request for 
> > >> later? Ignore it? Is it a system-wide policy or up to the device 
> > >> [class]?
> > > Return an error. If you queue, how much would you queue?
> > > There's no sane answer, so you better fail right away.
> > 
> > I guess that Dmitry's question was about the behavior of the driver, 
> > right? For example in situation when the LED state changes on PS/2 
> > keyboard, what should the USB keyboard driver do for a suspended USB 
> > keyboard.
> 
> I understood it that way.
> 
> > I really don't think we want to return error in such cases. Either resume 
> > the device, or queue some requests (maybe depending on the type of 
> > requests), right?
> 
> We are talking about forcibly suspended devices, so they cannot be
> resumed. We could queue, but where would we end queuing? And we
> have to decide to stop it at some point, or we create a DoS problem.
> 
> The user has decided to put the device in a state in which the driver
> cannot properly function. If the user wanted the driver to work, he should
> select autosuspend.
>

That is the thing with the forced suspend - it leaves device out of
sync with the rest of the system. If user really does not want to use
the device it should probably be completely disconnected?

-- 
Dmitry

Re: [rft]power management for usbtouch

[Alan Stern]

On Tue, 1 Jul 2008, Dmitry Torokhov wrote:

> > The user has decided to put the device in a state in which the driver
> > cannot properly function. If the user wanted the driver to work, he should
> > select autosuspend.
> >
> 
> That is the thing with the forced suspend - it leaves device out of
> sync with the rest of the system. If user really does not want to use
> the device it should probably be completely disconnected?

Some devices are rather difficult to disconnect (the cabling may be 
inaccessible).

Also, devices can be suspended by software, but generally speaking they 
cannot be disconnected by software.

Alan Stern

Re: [rft]power management for usbtouch

[Dmitry Torokhov]

On Tue, Jul 01, 2008 at 11:00:10AM -0400, Alan Stern wrote:
> On Tue, 1 Jul 2008, Dmitry Torokhov wrote:
> 
> > > The user has decided to put the device in a state in which the driver
> > > cannot properly function. If the user wanted the driver to work, he should
> > > select autosuspend.
> > >
> > 
> > That is the thing with the forced suspend - it leaves device out of
> > sync with the rest of the system. If user really does not want to use
> > the device it should probably be completely disconnected?
> 
> Some devices are rather difficult to disconnect (the cabling may be 
> inaccessible).
> 
> Also, devices can be suspended by software, but generally speaking they 
> cannot be disconnected by software.

By disconnecting I meant unbinding the driver really.

-- 
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: [rft]power management for usbtouch

[Oliver Neukum]

Am Dienstag 01 Juli 2008 14:56:30 schrieb Dmitry Torokhov:
> On Tue, Jul 01, 2008 at 11:08:47AM +0200, Oliver Neukum wrote:
> > Am Dienstag 01 Juli 2008 09:59:01 schrieb Jiri Kosina:

> > > I really don't think we want to return error in such cases. Either resume 
> > > the device, or queue some requests (maybe depending on the type of 
> > > requests), right?
> > 
> > We are talking about forcibly suspended devices, so they cannot be
> > resumed. We could queue, but where would we end queuing? And we
> > have to decide to stop it at some point, or we create a DoS problem.
> > 
> > The user has decided to put the device in a state in which the driver
> > cannot properly function. If the user wanted the driver to work, he should
> > select autosuspend.
> >
> 
> That is the thing with the forced suspend - it leaves device out of
> sync with the rest of the system. If user really does not want to use
> the device it should probably be completely disconnected?

Yes, therefore I suggested doing the logical disconnect.

The drawback is that we lose all settings made on the device. But
to keep those we have autosuspend. It's true that currently many drivers lack
support for autosuspend, but the API should be designed for the final state.

	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: [rft]power management for usbtouch

[Ville Syrjälä]

On Tue, Jul 01, 2008 at 06:57:01PM +0200, Oliver Neukum wrote:
> Am Dienstag 01 Juli 2008 14:56:30 schrieb Dmitry Torokhov:
> > On Tue, Jul 01, 2008 at 11:08:47AM +0200, Oliver Neukum wrote:
> > > Am Dienstag 01 Juli 2008 09:59:01 schrieb Jiri Kosina:
> 
> > > > I really don't think we want to return error in such cases. Either resume 
> > > > the device, or queue some requests (maybe depending on the type of 
> > > > requests), right?
> > > 
> > > We are talking about forcibly suspended devices, so they cannot be
> > > resumed. We could queue, but where would we end queuing? And we
> > > have to decide to stop it at some point, or we create a DoS problem.
> > > 
> > > The user has decided to put the device in a state in which the driver
> > > cannot properly function. If the user wanted the driver to work, he should
> > > select autosuspend.
> > >
> > 
> > That is the thing with the forced suspend - it leaves device out of
> > sync with the rest of the system. If user really does not want to use
> > the device it should probably be completely disconnected?
> 
> Yes, therefore I suggested doing the logical disconnect.

Would the persistence flag affect this? That is would the forced
suspend cause a reset_resume instead if the driver supports it?

-- 
Ville Syrjälä
syrjala-ORSVBvAovxo@public.gmane.org
http://www.sci.fi/~syrjala/
--
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: [rft]power management for usbtouch

[Alan Stern]

On Tue, 1 Jul 2008, Ville Syrjälä wrote:

> > > That is the thing with the forced suspend - it leaves device out of
> > > sync with the rest of the system. If user really does not want to use
> > > the device it should probably be completely disconnected?
> > 
> > Yes, therefore I suggested doing the logical disconnect.
> 
> Would the persistence flag affect this? That is would the forced
> suspend cause a reset_resume instead if the driver supports it?

There are only two reasons why a reset-resume might occur:

	A normal resume couldn't be carried out (for example, if
	the USB host controller had to be reset) or failed;

	The RESET_RESUME quirk flag is set for the device.

The first reason doesn't apply if the usb_persist flag isn't set, but 
the second reason does.

Alan Stern

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