On Wed, 31 March 2010 Jiri Kosina <[email protected]> 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 <[email protected]>
> > ---
> >
> > 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 <[email protected]>
---
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
Am Sonntag, 11. April 2010 13:02:26 schrieb Bruno Prémont:
> 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...
net/bluetooth/hidp/core.c
> 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).
Looks basically good.
> 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!
No, only if it wants to know that the command has finished.
hid_submit_out() will queue IO in the autosuspended case
and schedule a resumption.
> @@ -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);
It would be better to pass the full message.
Regards
Oliver
On Sun, 11 Apr 2010, Bruno Prémont wrote:
> > 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...
They are in net/bluetooth/hidp. Not really obvious, I know, but Marcel
wanted it this way.
> 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!
It's not needed. There is a queuing mechanism in place on
'resumption_waker' workqueue.
--
Jiri Kosina
SUSE Labs, Novell Inc.
On Sun, 11 April 2010 Jiri Kosina <[email protected]> wrote:
> > 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!
>
> It's not needed. There is a queuing mechanism in place on
> 'resumption_waker' workqueue.
So the driver is not allowed to ask the device to do something
at auto-suspend time (as that would abort the auto-suspension).
Am I right?
Thanks,
Bruno
Am Sonntag, 11. April 2010 20:40:02 schrieb Bruno Pr?mont:
> On Sun, 11 April 2010 Jiri Kosina <[email protected]> wrote:
> > > 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!
> >
> > It's not needed. There is a queuing mechanism in place on
> > 'resumption_waker' workqueue.
>
> So the driver is not allowed to ask the device to do something
> at auto-suspend time (as that would abort the auto-suspension).
>From within suspend()?
Generally if the driver wants to talk to a device that's a good
reason to wake up a device.
Regards
Oliver
On Sun, 11 Apr 2010, Bruno Pr?mont wrote:
> > > 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!
> >
> > It's not needed. There is a queuing mechanism in place on
> > 'resumption_waker' workqueue.
>
> So the driver is not allowed to ask the device to do something
> at auto-suspend time (as that would abort the auto-suspension).
What do you mean by "is not allowed" exactly?
Yes, it will wake the device up from auto-suspension. However, that's
usually what you want to happen, is there is anything you need to dispatch
to the device, isn't it?
What exactly is the situation you are trying to handle?
Thanks,
--
Jiri Kosina
SUSE Labs, Novell Inc.
On Mon, 12 April 2010 Jiri Kosina <[email protected]> wrote:
> On Sun, 11 Apr 2010, Bruno Prémont wrote:
>
> > > > 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!
> > >
> > > It's not needed. There is a queuing mechanism in place on
> > > 'resumption_waker' workqueue.
> >
> > So the driver is not allowed to ask the device to do something
> > at auto-suspend time (as that would abort the auto-suspension).
>
> What do you mean by "is not allowed" exactly?
>
> Yes, it will wake the device up from auto-suspension. However, that's
> usually what you want to happen, is there is anything you need to dispatch
> to the device, isn't it?
I thought of a report to send to device to ask the device to disable some
power using feature (e.g. turning off or dimming LEDs)
> What exactly is the situation you are trying to handle?
That's rather a generic question, not related to any implementation.
Maybe I'm mixing suspend and USB-autosuspend too much?
Thanks,
Bruno
Am Montag, 12. April 2010 18:56:07 schrieb Bruno Prémont:
> > Yes, it will wake the device up from auto-suspension. However, that's
> > usually what you want to happen, is there is anything you need to dispatch
> > to the device, isn't it?
>
> I thought of a report to send to device to ask the device to disable some
> power using feature (e.g. turning off or dimming LEDs)
The device should do this on its own. If it is really necessary you
can do this in suspend()
Regards
Oliver