On Jul 24 2017 or thereabouts, Lyude Paul wrote: > On Mon, 2017-07-24 at 10:15 +0200, Benjamin Tissoires wrote: > > On Jul 22 2017 or thereabouts, Lyude wrote: > > > So it looks like that suspend/resume has actually always been > > > broken on > > > hid-rmi. The fact it worked was a rather silly coincidence that was > > > relying on the HID device to already be opened upon resume. This > > > means > > > that so long as anything was reading the /dev/input/eventX node for > > > for > > > an RMI device, it would suspend and resume correctly. As well, if > > > nothing happened to be keeping the HID device away it would shut > > > off, > > > then the RMI driver would get confused on resume when it stopped > > > responding and explode. > > > > Oh, good finding. However, given that there are few other drivers not > > calling hid_hw_open during their .reset_resume() callback and those > > drivers also are communicating with the device, I wonder if we should > > not have something more generic, that will call hid_hw_open/close in > > the > > transport layer directly. > This sounds like a good idea, especially since a call like this is > rather easy to miss. I will look into doing that for v2 I know I requested a v2, but meanwhile some users are experiencing delays at resume: https://bugzilla.kernel.org/show_bug.cgi?id=196851 Jiri, could you take this one in v4.14 (maybe with @stable) and we'll figure out a way to fix this properly later (looks like the .resume and .reset_resume are called from the transport drivers, not hid-core, so we might need a little bit of testing). In its current form (with the nitpick Andy noticed): Reviewed-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> Cheers, Benjamin > > > > I do not recall having seen bugs for Wacom devices, so maybe this is > > something i2c-hid related, but it wouldn't hurt I guess to open/close > > the device before calling reset_resume. > > > > Cheers, > > Benjamin > > > > > > > > So, call hid_hw_open() in rmi_post_resume() so we make sure that > > > the > > > device is alive before we try talking to it. > > > > > > This fixes RMI device suspend/resume over HID. > > > > > > Signed-off-by: Lyude <lyude@xxxxxxxxxx> > > > Cc: Andrew Duggan <aduggan@xxxxxxxxxxxxx> > > > Cc: stable@xxxxxxxxxxxxxxx > > > --- > > > drivers/hid/hid-rmi.c | 15 +++++++++++---- > > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c > > > index 5b40c2614599..e7d124f9a27f 100644 > > > --- a/drivers/hid/hid-rmi.c > > > +++ b/drivers/hid/hid-rmi.c > > > @@ -431,22 +431,29 @@ static int rmi_post_resume(struct hid_device > > > *hdev) > > > { > > > struct rmi_data *data = hid_get_drvdata(hdev); > > > struct rmi_device *rmi_dev = data->xport.rmi_dev; > > > - int ret; > > > + int ret = 0; > > > > > > if (!(data->device_flags & RMI_DEVICE)) > > > return 0; > > > > > > - ret = rmi_reset_attn_mode(hdev); > > > + /* Make sure the HID device is ready to receive events */ > > > + ret = hid_hw_open(hdev); > > > if (ret) > > > return ret; > > > > > > + ret = rmi_reset_attn_mode(hdev); > > > + if (ret) > > > + goto out; > > > + > > > ret = rmi_driver_resume(rmi_dev, false); > > > if (ret) { > > > hid_warn(hdev, "Failed to resume device: %d\n", > > > ret); > > > - return ret; > > > + goto out; > > > } > > > > > > - return 0; > > > +out: > > > + hid_hw_close(hdev); > > > + return ret; > > > } > > > #endif /* CONFIG_PM */ > > > > > > -- > > > 2.13.3 > > >