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 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 <ly...@redhat.com>
> > Cc: Andrew Duggan <adug...@synaptics.com>
> > Cc: sta...@vger.kernel.org
> > ---
> > 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
> >
