[PATCH] HID: rmi: Check that a device is a RMI device before calling RMI functions

[PATCH] HID: rmi: Check that a device is a RMI device before calling RMI functions

[Andrew Duggan]

The hid-rmi driver may handle non rmi devices on composite USB devices.
Callbacks need to make sure that the current device is a RMI device before
calling RMI specific functions. Most callbacks already have this check, but
this patch adds checks to the remaining callbacks.

Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
---
This is the patch which hopefully will address the X1 tablet dock freeze:
http://www.spinics.net/lists/linux-input/msg53582.html

I was not able to test on a composite USB device so I have not tested confirmed
this will fix the reported issues. But, based on the description I think it will.
I also added a check for rmi_raw_event() since it could be possible that another
hid device using one of the same report IDs as an RMI device could result in calling
into unitialized RMI functions. It was also the only callbacl left not checking the
RMI_DEVICE flag. I wonder if this explains the attach crash.

Anyway, I would appriciate it if Hendrik or someone else with the device could test this
patch to confirm it fixes the reported behavior.

Thanks,
Andrew

 drivers/hid/hid-rmi.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
index 5b40c26..d987e02 100644
--- a/drivers/hid/hid-rmi.c
+++ b/drivers/hid/hid-rmi.c
@@ -368,6 +368,11 @@ static int rmi_check_sanity(struct hid_device *hdev, u8 *data, int size)
 static int rmi_raw_event(struct hid_device *hdev,
 		struct hid_report *report, u8 *data, int size)
 {
+	struct rmi_data *hdata = hid_get_drvdata(hdev);
+
+	if (!(hdata->device_flags & RMI_DEVICE))
+		return 0;
+
 	size = rmi_check_sanity(hdev, data, size);
 	if (size < 2)
 		return 0;
@@ -706,9 +711,11 @@ static void rmi_remove(struct hid_device *hdev)
 {
 	struct rmi_data *hdata = hid_get_drvdata(hdev);
 
-	clear_bit(RMI_STARTED, &hdata->flags);
-	cancel_work_sync(&hdata->reset_work);
-	rmi_unregister_transport_device(&hdata->xport);
+	if (hdata->device_flags & RMI_DEVICE) {
+		clear_bit(RMI_STARTED, &hdata->flags);
+		cancel_work_sync(&hdata->reset_work);
+		rmi_unregister_transport_device(&hdata->xport);
+	}
 
 	hid_hw_stop(hdev);
 }
-- 
2.7.4

Re: [PATCH] HID: rmi: Check that a device is a RMI device before calling RMI functions

[Benjamin Tissoires]

On Oct 17 2017 or thereabouts, Andrew Duggan wrote:
> The hid-rmi driver may handle non rmi devices on composite USB devices.
> Callbacks need to make sure that the current device is a RMI device before
> calling RMI specific functions. Most callbacks already have this check, but
> this patch adds checks to the remaining callbacks.
> 
> Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
> ---
> This is the patch which hopefully will address the X1 tablet dock freeze:
> http://www.spinics.net/lists/linux-input/msg53582.html
> 
> I was not able to test on a composite USB device so I have not tested confirmed
> this will fix the reported issues. But, based on the description I think it will.
> I also added a check for rmi_raw_event() since it could be possible that another
> hid device using one of the same report IDs as an RMI device could result in calling
> into unitialized RMI functions. It was also the only callbacl left not checking the
> RMI_DEVICE flag. I wonder if this explains the attach crash.
> 
> Anyway, I would appriciate it if Hendrik or someone else with the device could test this
> patch to confirm it fixes the reported behavior.

Shouldn't rmi_hid_reset() also get the same check?

>From what I can see, even if the patch doesn't fix the immediate issue,
such a patch should definitively go in as those checks are clearly
missing.

Cheers,
Benjamin

> 
> Thanks,
> Andrew
> 
>  drivers/hid/hid-rmi.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index 5b40c26..d987e02 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -368,6 +368,11 @@ static int rmi_check_sanity(struct hid_device *hdev, u8 *data, int size)
>  static int rmi_raw_event(struct hid_device *hdev,
>  		struct hid_report *report, u8 *data, int size)
>  {
> +	struct rmi_data *hdata = hid_get_drvdata(hdev);
> +
> +	if (!(hdata->device_flags & RMI_DEVICE))
> +		return 0;
> +
>  	size = rmi_check_sanity(hdev, data, size);
>  	if (size < 2)
>  		return 0;
> @@ -706,9 +711,11 @@ static void rmi_remove(struct hid_device *hdev)
>  {
>  	struct rmi_data *hdata = hid_get_drvdata(hdev);
>  
> -	clear_bit(RMI_STARTED, &hdata->flags);
> -	cancel_work_sync(&hdata->reset_work);
> -	rmi_unregister_transport_device(&hdata->xport);
> +	if (hdata->device_flags & RMI_DEVICE) {
> +		clear_bit(RMI_STARTED, &hdata->flags);
> +		cancel_work_sync(&hdata->reset_work);
> +		rmi_unregister_transport_device(&hdata->xport);
> +	}
>  
>  	hid_hw_stop(hdev);
>  }
> -- 
> 2.7.4
> 

Re: [PATCH] HID: rmi: Check that a device is a RMI device before calling RMI functions

[Jiri Kosina]

On Wed, 18 Oct 2017, Benjamin Tissoires wrote:

> > The hid-rmi driver may handle non rmi devices on composite USB devices.
> > Callbacks need to make sure that the current device is a RMI device before
> > calling RMI specific functions. Most callbacks already have this check, but
> > this patch adds checks to the remaining callbacks.
> > 
> > Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
> > ---
> > This is the patch which hopefully will address the X1 tablet dock freeze:
> > http://www.spinics.net/lists/linux-input/msg53582.html
> > 
> > I was not able to test on a composite USB device so I have not tested confirmed
> > this will fix the reported issues. But, based on the description I think it will.
> > I also added a check for rmi_raw_event() since it could be possible that another
> > hid device using one of the same report IDs as an RMI device could result in calling
> > into unitialized RMI functions. It was also the only callbacl left not checking the
> > RMI_DEVICE flag. I wonder if this explains the attach crash.
> > 
> > Anyway, I would appriciate it if Hendrik or someone else with the device could test this
> > patch to confirm it fixes the reported behavior.
> 
> Shouldn't rmi_hid_reset() also get the same check?

I think so as well.

> 
> From what I can see, even if the patch doesn't fix the immediate issue, 
> such a patch should definitively go in as those checks are clearly 
> missing.

Agreed; however I'd like to get Hendrik's Tested-by: if possible in case 
this really fixes the issue, so I am not merging it right away.

Hendrik, are you by any chance able to test this patch in a reasonable 
timeframe please?

Thanks!

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: rmi: Check that a device is a RMI device before calling RMI functions

[Andrew Duggan]

On 10/18/2017 07:13 AM, Jiri Kosina wrote:
> On Wed, 18 Oct 2017, Benjamin Tissoires wrote:
>
>>> The hid-rmi driver may handle non rmi devices on composite USB devices.
>>> Callbacks need to make sure that the current device is a RMI device before
>>> calling RMI specific functions. Most callbacks already have this check, but
>>> this patch adds checks to the remaining callbacks.
>>>
>>> Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
>>> ---
>>> This is the patch which hopefully will address the X1 tablet dock freeze:
>>> http://www.spinics.net/lists/linux-input/msg53582.html
>>>
>>> I was not able to test on a composite USB device so I have not tested confirmed
>>> this will fix the reported issues. But, based on the description I think it will.
>>> I also added a check for rmi_raw_event() since it could be possible that another
>>> hid device using one of the same report IDs as an RMI device could result in calling
>>> into unitialized RMI functions. It was also the only callbacl left not checking the
>>> RMI_DEVICE flag. I wonder if this explains the attach crash.
>>>
>>> Anyway, I would appriciate it if Hendrik or someone else with the device could test this
>>> patch to confirm it fixes the reported behavior.
>> Shouldn't rmi_hid_reset() also get the same check?
> I think so as well.

Since rmi_hid_reset() is only called from with in the RMI4 driver I 
didn't think that it needed a check. All of the functions set in struct 
hid_driver should  now check the RMI_DEVICE flag before calling into the 
RMI4 driver. But, if I am missing something we can add additional checks.

Thanks,
Andrew

>>  From what I can see, even if the patch doesn't fix the immediate issue,
>> such a patch should definitively go in as those checks are clearly
>> missing.
> Agreed; however I'd like to get Hendrik's Tested-by: if possible in case
> this really fixes the issue, so I am not merging it right away.
>
> Hendrik, are you by any chance able to test this patch in a reasonable
> timeframe please?
>
> Thanks!
>

Re: [PATCH] HID: rmi: Check that a device is a RMI device before calling RMI functions

[Hendrik Langer]

On 18/10/17 16:13, Jiri Kosina wrote:
> On Wed, 18 Oct 2017, Benjamin Tissoires wrote:
> 
>>> The hid-rmi driver may handle non rmi devices on composite USB devices.
>>> Callbacks need to make sure that the current device is a RMI device before
>>> calling RMI specific functions. Most callbacks already have this check, but
>>> this patch adds checks to the remaining callbacks.
>>>
>>> Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
>>> ---
>>> This is the patch which hopefully will address the X1 tablet dock freeze:
>>> http://www.spinics.net/lists/linux-input/msg53582.html
>>>
>>> I was not able to test on a composite USB device so I have not tested confirmed
>>> this will fix the reported issues. But, based on the description I think it will.
>>> I also added a check for rmi_raw_event() since it could be possible that another
>>> hid device using one of the same report IDs as an RMI device could result in calling
>>> into unitialized RMI functions. It was also the only callbacl left not checking the
>>> RMI_DEVICE flag. I wonder if this explains the attach crash.
>>>
>>> Anyway, I would appriciate it if Hendrik or someone else with the device could test this
>>> patch to confirm it fixes the reported behavior.
>>
>> Shouldn't rmi_hid_reset() also get the same check?
> 
> I think so as well.
> 
>>
>> From what I can see, even if the patch doesn't fix the immediate issue, 
>> such a patch should definitively go in as those checks are clearly 
>> missing.
> 
> Agreed; however I'd like to get Hendrik's Tested-by: if possible in case 
> this really fixes the issue, so I am not merging it right away.
> 
> Hendrik, are you by any chance able to test this patch in a reasonable 
> timeframe please?
> 
> Thanks!
> 

Tested-by: Hendrik Langer <hendrik.langer@gmx.de>

Confirmed, the patch fixes the probem for me.
All freezes/Oops and rmi or input related error messages are gone. Even
keyboard and touchpad are working now!

Tested 4.13 and 4.14.0-rc3 (Debian) with the patch, repeated (un)docking
and booting with/without keyboard dock attached.

Thank you very much!
Hendrik

Re: [PATCH] HID: rmi: Check that a device is a RMI device before calling RMI functions

[Benjamin Tissoires]

On Oct 18 2017 or thereabouts, Andrew Duggan wrote:
> 
> 
> On 10/18/2017 07:13 AM, Jiri Kosina wrote:
> > On Wed, 18 Oct 2017, Benjamin Tissoires wrote:
> > 
> > > > The hid-rmi driver may handle non rmi devices on composite USB devices.
> > > > Callbacks need to make sure that the current device is a RMI device before
> > > > calling RMI specific functions. Most callbacks already have this check, but
> > > > this patch adds checks to the remaining callbacks.
> > > > 
> > > > Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
> > > > ---
> > > > This is the patch which hopefully will address the X1 tablet dock freeze:
> > > > http://www.spinics.net/lists/linux-input/msg53582.html
> > > > 
> > > > I was not able to test on a composite USB device so I have not tested confirmed
> > > > this will fix the reported issues. But, based on the description I think it will.
> > > > I also added a check for rmi_raw_event() since it could be possible that another
> > > > hid device using one of the same report IDs as an RMI device could result in calling
> > > > into unitialized RMI functions. It was also the only callbacl left not checking the
> > > > RMI_DEVICE flag. I wonder if this explains the attach crash.
> > > > 
> > > > Anyway, I would appriciate it if Hendrik or someone else with the device could test this
> > > > patch to confirm it fixes the reported behavior.
> > > Shouldn't rmi_hid_reset() also get the same check?
> > I think so as well.
> 
> Since rmi_hid_reset() is only called from with in the RMI4 driver I didn't
> think that it needed a check. All of the functions set in struct hid_driver
> should  now check the RMI_DEVICE flag before calling into the RMI4 driver.
> But, if I am missing something we can add additional checks.

Oh, right, you are correct. I didn't paid enough attention and
overlooked the fact that this function was set in struct
rmi_transport_ops only.

Given Hendrick's Tested-by came in as well (thanks for the quick
tests!), this is:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

> 
> Thanks,
> Andrew
> 
> > >  From what I can see, even if the patch doesn't fix the immediate issue,
> > > such a patch should definitively go in as those checks are clearly
> > > missing.
> > Agreed; however I'd like to get Hendrik's Tested-by: if possible in case
> > this really fixes the issue, so I am not merging it right away.
> > 
> > Hendrik, are you by any chance able to test this patch in a reasonable
> > timeframe please?
> > 
> > Thanks!
> > 
> 

Re: [PATCH] HID: rmi: Check that a device is a RMI device before calling RMI functions

[Jiri Kosina]

On Wed, 18 Oct 2017, Hendrik Langer wrote:

> Tested-by: Hendrik Langer <hendrik.langer@gmx.de>
> 
> Confirmed, the patch fixes the probem for me.
> All freezes/Oops and rmi or input related error messages are gone. Even
> keyboard and touchpad are working now!

Thanks a lot for confirmation.

Now queued in for-4.14/upstream-fixes.

-- 
Jiri Kosina
SUSE Labs