| Message ID | 1508290656-18871-1-git-send-email-aduggan@synaptics.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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!
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! > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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! > > > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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); }
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(-)