Re: [PATCH] HID: rmi: Make sure the HID device is opened on resume

[Benjamin Tissoires]

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