Re: [PATCH v6 7/8, needs improvement] hid: add suspend/resume hooks for hid drivers

["Bruno Prémont" <bonbons@linux-vserver.org>]

On Wed, 31 March 2010 Jiri Kosina <jkosina@suse.cz> wrote:

> On Tue, 30 Mar 2010, Bruno Prémont wrote:
> 
> > Add suspend/resume hooks for HID drivers so these can do some
> > additional state adjustment when device gets suspended/resumed.
> > 
> > This patch calls these hooks from usbhid suspend/resume functions,
> > only calling suspend on plain suspend, not autosuspend.
> > (it might be worth adding an autosuspend parameter to suspend
> > hook and calling suspend in both cases)
> > 
> > Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> > ---
> > 
> > Note:
> >   this patch needs improvements as mentionned by Olivier Neukum:
> >   - suspend hook for both system suspend and autosuspend
> >   - no call of hook on USB-HID-resume failure
> 
> I agree with Oliver's comments.
> 
> We should also consider putting the hook calls into bluetooth 
> implementation as well (though there is currently no driver using it, but 
> I don't know, maybe Wacom could benefit from it as well).

Where do I find the bluetooth HID bits? I've been searching through kernel
sources (under drivers/) but did not find bluetooth code trying to
register a HID device...

Below an updated patch which also calls the hook for auto-suspend case.
The patch compiles but I've not runtime-tested it yet (especially for
the auto-suspend part, as I'm not sure what should trigger it).

As far as I understand the code, in auto-suspend case if the driver
sends commands to the device it will have to call usbhid_wait_io().
Please correct me if I'm wrong!

Thanks,
Bruno


---

Add suspend/resume hooks for HID drivers so these can do some
additional state adjustment when device gets suspended/resumed.

v2:
  - Adds auto_suspend parameter to suspend hook
  - Only calls HID driver's resume hooks when previous code
    did succeed

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
 drivers/hid/usbhid/hid-core.c |   24 +++++++++++++++++++++++-
 include/linux/hid.h           |    8 ++++++++
 2 files changed, 31 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 3e7909b..1f40695 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1291,6 +1291,11 @@ static int hid_suspend(struct usb_interface *intf, pm_message_t message)
 		{
 			set_bit(HID_REPORTED_IDLE, &usbhid->iofl);
 			spin_unlock_irq(&usbhid->lock);
+			if (hid->driver && hid->driver->suspend) {
+				status = hid->driver->suspend(hid, 1);
+				if (status < 0)
+					return status;
+			}
 		} else {
 			usbhid_mark_busy(usbhid);
 			spin_unlock_irq(&usbhid->lock);
@@ -1298,6 +1303,11 @@ static int hid_suspend(struct usb_interface *intf, pm_message_t message)
 		}
 
 	} else {
+		if (hid->driver && hid->driver->suspend) {
+			status = hid->driver->suspend(hid, 0);
+			if (status < 0)
+				return status;
+		}
 		spin_lock_irq(&usbhid->lock);
 		set_bit(HID_REPORTED_IDLE, &usbhid->iofl);
 		spin_unlock_irq(&usbhid->lock);
@@ -1352,6 +1362,11 @@ static int hid_resume(struct usb_interface *intf)
 		hid_io_error(hid);
 	usbhid_restart_queues(usbhid);
 
+	if (status >= 0 && hid->driver && hid->driver->resume) {
+		int ret = hid->driver->resume(hid);
+		if (ret < 0)
+			status = ret;
+	}
 	dev_dbg(&intf->dev, "resume status %d\n", status);
 	return 0;
 }
@@ -1360,9 +1375,16 @@ static int hid_reset_resume(struct usb_interface *intf)
 {
 	struct hid_device *hid = usb_get_intfdata(intf);
 	struct usbhid_device *usbhid = hid->driver_data;
+	int status;
 
 	clear_bit(HID_REPORTED_IDLE, &usbhid->iofl);
-	return hid_post_reset(intf);
+	status = hid_post_reset(intf);
+	if (status >= 0 && hid->driver && hid->driver->reset_resume) {
+		int ret = hid->driver->reset_resume(hid);
+		if (ret < 0)
+			status = ret;
+	}
+	return status;
 }
 
 #endif /* CONFIG_PM */
diff --git a/include/linux/hid.h b/include/linux/hid.h
index b1344ec..a2f3612 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -589,6 +589,9 @@ struct hid_usage_id {
  * @report_fixup: called before report descriptor parsing (NULL means nop)
  * @input_mapping: invoked on input registering before mapping an usage
  * @input_mapped: invoked on input registering after mapping an usage
+ * @suspend: invoked on suspend (NULL means nop)
+ * @resume: invoked on resume if device was not reset (NULL means nop)
+ * @reset_resume: invoked on resume if device was reset (NULL means nop)
  *
  * raw_event and event should return 0 on no action performed, 1 when no
  * further processing should be done and negative on error
@@ -629,6 +632,11 @@ struct hid_driver {
 	int (*input_mapped)(struct hid_device *hdev,
 			struct hid_input *hidinput, struct hid_field *field,
 			struct hid_usage *usage, unsigned long **bit, int *max);
+#ifdef CONFIG_PM
+	int (*suspend)(struct hid_device *hdev, int auto_suspend);
+	int (*resume)(struct hid_device *hdev);
+	int (*reset_resume)(struct hid_device *hdev);
+#endif
 /* private: */
 	struct device_driver driver;
 };
-- 
1.6.4.4