| Message ID | 20191016182935.5616-3-andrew.smirnov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | Logitech G920 fixes | expand |
Hi Andrey, On Wed, Oct 16, 2019 at 8:30 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > > G920 device only advertises REPORT_ID_HIDPP_LONG and > REPORT_ID_HIDPP_VERY_LONG in its HID report descriptor, so querying > for REPORT_ID_HIDPP_SHORT with optional=false will always fail and > prevent G920 to be recognized as a valid HID++ device. > > To fix this and improve some other aspects, modify > hidpp_validate_device() as follows: > > - Inline the code of hidpp_validate_report() to simplify > distingushing between non-present and invalid report descriptors > > - Drop the check for id >= HID_MAX_IDS || id < 0 since all of our > IDs are static and known to satisfy that at compile time > > - Change the algorithms to check all possible report > types (including very long report) and deem the device as a valid > HID++ device if it supports at least one > > - Treat invalid report length as a hard stop for the validation > algorithm, meaning that if any of the supported reports has > invalid length we assume the worst and treat the device as a > generic HID device. > > - Fold initialization of hidpp->very_long_report_length into > hidpp_validate_device() since it already fetches very long report > length and validates its value > > Fixes: fe3ee1ec007b ("HID: logitech-hidpp: allow non HID++ devices to be handled by this module") > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204191 > Reported-by: Sam Bazely <sambazley@fastmail.com> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > Cc: Jiri Kosina <jikos@kernel.org> > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Cc: Henrik Rydberg <rydberg@bitmath.org> > Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com> > Cc: Austin Palmer <austinp@valvesoftware.com> > Cc: linux-input@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: stable@vger.kernel.org # 5.2+ > --- > drivers/hid/hid-logitech-hidpp.c | 54 ++++++++++++++++++-------------- > 1 file changed, 30 insertions(+), 24 deletions(-) > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > index 85911586b3b6..8c4be991f387 100644 > --- a/drivers/hid/hid-logitech-hidpp.c > +++ b/drivers/hid/hid-logitech-hidpp.c > @@ -3498,34 +3498,45 @@ static int hidpp_get_report_length(struct hid_device *hdev, int id) > return report->field[0]->report_count + 1; > } > > -static bool hidpp_validate_report(struct hid_device *hdev, int id, > - int expected_length, bool optional) > +static bool hidpp_validate_device(struct hid_device *hdev) > { > - int report_length; > + struct hidpp_device *hidpp = hid_get_drvdata(hdev); > + int id, report_length, supported_reports = 0; > + > + id = REPORT_ID_HIDPP_SHORT; > + report_length = hidpp_get_report_length(hdev, id); > + if (report_length) { > + if (report_length < HIDPP_REPORT_SHORT_LENGTH) > + goto bad_device; > > - if (id >= HID_MAX_IDS || id < 0) { > - hid_err(hdev, "invalid HID report id %u\n", id); > - return false; > + supported_reports++; > } > > + id = REPORT_ID_HIDPP_LONG; > report_length = hidpp_get_report_length(hdev, id); > - if (!report_length) > - return optional; > + if (report_length) { > + if (report_length < HIDPP_REPORT_LONG_LENGTH) > + goto bad_device; > > - if (report_length < expected_length) { > - hid_warn(hdev, "not enough values in hidpp report %d\n", id); > - return false; > + supported_reports++; > } > > - return true; > -} > + id = REPORT_ID_HIDPP_VERY_LONG; > + report_length = hidpp_get_report_length(hdev, id); > + if (report_length) { > + if (report_length > HIDPP_REPORT_LONG_LENGTH && > + report_length < HIDPP_REPORT_VERY_LONG_MAX_LENGTH) Can you double check the conditions here? It's late, but I think you inverted the tests as we expect the report length to be between HIDPP_REPORT_LONG_LENGTH and HIDPP_REPORT_VERY_LONG_MAX_LENGTH inclusive, while here this creates a bad_device. Other than that, I really like the series. Cheers, Benjamin > + goto bad_device; > > -static bool hidpp_validate_device(struct hid_device *hdev) > -{ > - return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT, > - HIDPP_REPORT_SHORT_LENGTH, false) && > - hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG, > - HIDPP_REPORT_LONG_LENGTH, true); > + supported_reports++; > + hidpp->very_long_report_length = report_length; > + } > + > + return supported_reports; > + > +bad_device: > + hid_warn(hdev, "not enough values in hidpp report %d\n", id); > + return false; > } > > static bool hidpp_application_equals(struct hid_device *hdev, > @@ -3572,11 +3583,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) > return hid_hw_start(hdev, HID_CONNECT_DEFAULT); > } > > - hidpp->very_long_report_length = > - hidpp_get_report_length(hdev, REPORT_ID_HIDPP_VERY_LONG); > - if (hidpp->very_long_report_length > HIDPP_REPORT_VERY_LONG_MAX_LENGTH) > - hidpp->very_long_report_length = HIDPP_REPORT_VERY_LONG_MAX_LENGTH; > - > if (id->group == HID_GROUP_LOGITECH_DJ_DEVICE) > hidpp->quirks |= HIDPP_QUIRK_UNIFYING; > > -- > 2.21.0 >
On Wed, Oct 16, 2019 at 12:24 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi Andrey, > > On Wed, Oct 16, 2019 at 8:30 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > > > > G920 device only advertises REPORT_ID_HIDPP_LONG and > > REPORT_ID_HIDPP_VERY_LONG in its HID report descriptor, so querying > > for REPORT_ID_HIDPP_SHORT with optional=false will always fail and > > prevent G920 to be recognized as a valid HID++ device. > > > > To fix this and improve some other aspects, modify > > hidpp_validate_device() as follows: > > > > - Inline the code of hidpp_validate_report() to simplify > > distingushing between non-present and invalid report descriptors > > > > - Drop the check for id >= HID_MAX_IDS || id < 0 since all of our > > IDs are static and known to satisfy that at compile time > > > > - Change the algorithms to check all possible report > > types (including very long report) and deem the device as a valid > > HID++ device if it supports at least one > > > > - Treat invalid report length as a hard stop for the validation > > algorithm, meaning that if any of the supported reports has > > invalid length we assume the worst and treat the device as a > > generic HID device. > > > > - Fold initialization of hidpp->very_long_report_length into > > hidpp_validate_device() since it already fetches very long report > > length and validates its value > > > > Fixes: fe3ee1ec007b ("HID: logitech-hidpp: allow non HID++ devices to be handled by this module") > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204191 > > Reported-by: Sam Bazely <sambazley@fastmail.com> > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > > Cc: Jiri Kosina <jikos@kernel.org> > > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > Cc: Henrik Rydberg <rydberg@bitmath.org> > > Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com> > > Cc: Austin Palmer <austinp@valvesoftware.com> > > Cc: linux-input@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > Cc: stable@vger.kernel.org # 5.2+ > > --- > > drivers/hid/hid-logitech-hidpp.c | 54 ++++++++++++++++++-------------- > > 1 file changed, 30 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > > index 85911586b3b6..8c4be991f387 100644 > > --- a/drivers/hid/hid-logitech-hidpp.c > > +++ b/drivers/hid/hid-logitech-hidpp.c > > @@ -3498,34 +3498,45 @@ static int hidpp_get_report_length(struct hid_device *hdev, int id) > > return report->field[0]->report_count + 1; > > } > > > > -static bool hidpp_validate_report(struct hid_device *hdev, int id, > > - int expected_length, bool optional) > > +static bool hidpp_validate_device(struct hid_device *hdev) > > { > > - int report_length; > > + struct hidpp_device *hidpp = hid_get_drvdata(hdev); > > + int id, report_length, supported_reports = 0; > > + > > + id = REPORT_ID_HIDPP_SHORT; > > + report_length = hidpp_get_report_length(hdev, id); > > + if (report_length) { > > + if (report_length < HIDPP_REPORT_SHORT_LENGTH) > > + goto bad_device; > > > > - if (id >= HID_MAX_IDS || id < 0) { > > - hid_err(hdev, "invalid HID report id %u\n", id); > > - return false; > > + supported_reports++; > > } > > > > + id = REPORT_ID_HIDPP_LONG; > > report_length = hidpp_get_report_length(hdev, id); > > - if (!report_length) > > - return optional; > > + if (report_length) { > > + if (report_length < HIDPP_REPORT_LONG_LENGTH) > > + goto bad_device; > > > > - if (report_length < expected_length) { > > - hid_warn(hdev, "not enough values in hidpp report %d\n", id); > > - return false; > > + supported_reports++; > > } > > > > - return true; > > -} > > + id = REPORT_ID_HIDPP_VERY_LONG; > > + report_length = hidpp_get_report_length(hdev, id); > > + if (report_length) { > > + if (report_length > HIDPP_REPORT_LONG_LENGTH && > > + report_length < HIDPP_REPORT_VERY_LONG_MAX_LENGTH) > > Can you double check the conditions here? > It's late, but I think you inverted the tests as we expect the report > length to be between HIDPP_REPORT_LONG_LENGTH and > HIDPP_REPORT_VERY_LONG_MAX_LENGTH inclusive, while here this creates a > bad_device. Hmm, I think you are right. Not sure why I didn't catch it on G920 since it support very long reports AFAIR. Will re-spin and double check in v3. Sorry about that. Thanks, Andrey Smirnov
On Wed, Oct 16, 2019 at 9:38 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > > On Wed, Oct 16, 2019 at 12:24 PM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > Hi Andrey, > > > > On Wed, Oct 16, 2019 at 8:30 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > > > > > > G920 device only advertises REPORT_ID_HIDPP_LONG and > > > REPORT_ID_HIDPP_VERY_LONG in its HID report descriptor, so querying > > > for REPORT_ID_HIDPP_SHORT with optional=false will always fail and > > > prevent G920 to be recognized as a valid HID++ device. > > > > > > To fix this and improve some other aspects, modify > > > hidpp_validate_device() as follows: > > > > > > - Inline the code of hidpp_validate_report() to simplify > > > distingushing between non-present and invalid report descriptors > > > > > > - Drop the check for id >= HID_MAX_IDS || id < 0 since all of our > > > IDs are static and known to satisfy that at compile time > > > > > > - Change the algorithms to check all possible report > > > types (including very long report) and deem the device as a valid > > > HID++ device if it supports at least one > > > > > > - Treat invalid report length as a hard stop for the validation > > > algorithm, meaning that if any of the supported reports has > > > invalid length we assume the worst and treat the device as a > > > generic HID device. > > > > > > - Fold initialization of hidpp->very_long_report_length into > > > hidpp_validate_device() since it already fetches very long report > > > length and validates its value > > > > > > Fixes: fe3ee1ec007b ("HID: logitech-hidpp: allow non HID++ devices to be handled by this module") > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204191 > > > Reported-by: Sam Bazely <sambazley@fastmail.com> > > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > > > Cc: Jiri Kosina <jikos@kernel.org> > > > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > Cc: Henrik Rydberg <rydberg@bitmath.org> > > > Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com> > > > Cc: Austin Palmer <austinp@valvesoftware.com> > > > Cc: linux-input@vger.kernel.org > > > Cc: linux-kernel@vger.kernel.org > > > Cc: stable@vger.kernel.org # 5.2+ > > > --- > > > drivers/hid/hid-logitech-hidpp.c | 54 ++++++++++++++++++-------------- > > > 1 file changed, 30 insertions(+), 24 deletions(-) > > > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > > > index 85911586b3b6..8c4be991f387 100644 > > > --- a/drivers/hid/hid-logitech-hidpp.c > > > +++ b/drivers/hid/hid-logitech-hidpp.c > > > @@ -3498,34 +3498,45 @@ static int hidpp_get_report_length(struct hid_device *hdev, int id) > > > return report->field[0]->report_count + 1; > > > } > > > > > > -static bool hidpp_validate_report(struct hid_device *hdev, int id, > > > - int expected_length, bool optional) > > > +static bool hidpp_validate_device(struct hid_device *hdev) > > > { > > > - int report_length; > > > + struct hidpp_device *hidpp = hid_get_drvdata(hdev); > > > + int id, report_length, supported_reports = 0; > > > + > > > + id = REPORT_ID_HIDPP_SHORT; > > > + report_length = hidpp_get_report_length(hdev, id); > > > + if (report_length) { > > > + if (report_length < HIDPP_REPORT_SHORT_LENGTH) > > > + goto bad_device; > > > > > > - if (id >= HID_MAX_IDS || id < 0) { > > > - hid_err(hdev, "invalid HID report id %u\n", id); > > > - return false; > > > + supported_reports++; > > > } > > > > > > + id = REPORT_ID_HIDPP_LONG; > > > report_length = hidpp_get_report_length(hdev, id); > > > - if (!report_length) > > > - return optional; > > > + if (report_length) { > > > + if (report_length < HIDPP_REPORT_LONG_LENGTH) > > > + goto bad_device; > > > > > > - if (report_length < expected_length) { > > > - hid_warn(hdev, "not enough values in hidpp report %d\n", id); > > > - return false; > > > + supported_reports++; > > > } > > > > > > - return true; > > > -} > > > + id = REPORT_ID_HIDPP_VERY_LONG; > > > + report_length = hidpp_get_report_length(hdev, id); > > > + if (report_length) { > > > + if (report_length > HIDPP_REPORT_LONG_LENGTH && > > > + report_length < HIDPP_REPORT_VERY_LONG_MAX_LENGTH) > > > > Can you double check the conditions here? > > It's late, but I think you inverted the tests as we expect the report > > length to be between HIDPP_REPORT_LONG_LENGTH and > > HIDPP_REPORT_VERY_LONG_MAX_LENGTH inclusive, while here this creates a > > bad_device. > > Hmm, I think you are right. Not sure why I didn't catch it on G920 > since it support very long reports AFAIR. Will re-spin and double > check in v3. Sorry about that. > Oh, the issue is that the very long HID++ reports on the G920 are HIDPP_REPORT_VERY_LONG_MAX_LENGTH long, which means the test will fail for those. Cheers, Benjamin
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 85911586b3b6..8c4be991f387 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -3498,34 +3498,45 @@ static int hidpp_get_report_length(struct hid_device *hdev, int id) return report->field[0]->report_count + 1; } -static bool hidpp_validate_report(struct hid_device *hdev, int id, - int expected_length, bool optional) +static bool hidpp_validate_device(struct hid_device *hdev) { - int report_length; + struct hidpp_device *hidpp = hid_get_drvdata(hdev); + int id, report_length, supported_reports = 0; + + id = REPORT_ID_HIDPP_SHORT; + report_length = hidpp_get_report_length(hdev, id); + if (report_length) { + if (report_length < HIDPP_REPORT_SHORT_LENGTH) + goto bad_device; - if (id >= HID_MAX_IDS || id < 0) { - hid_err(hdev, "invalid HID report id %u\n", id); - return false; + supported_reports++; } + id = REPORT_ID_HIDPP_LONG; report_length = hidpp_get_report_length(hdev, id); - if (!report_length) - return optional; + if (report_length) { + if (report_length < HIDPP_REPORT_LONG_LENGTH) + goto bad_device; - if (report_length < expected_length) { - hid_warn(hdev, "not enough values in hidpp report %d\n", id); - return false; + supported_reports++; } - return true; -} + id = REPORT_ID_HIDPP_VERY_LONG; + report_length = hidpp_get_report_length(hdev, id); + if (report_length) { + if (report_length > HIDPP_REPORT_LONG_LENGTH && + report_length < HIDPP_REPORT_VERY_LONG_MAX_LENGTH) + goto bad_device; -static bool hidpp_validate_device(struct hid_device *hdev) -{ - return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT, - HIDPP_REPORT_SHORT_LENGTH, false) && - hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG, - HIDPP_REPORT_LONG_LENGTH, true); + supported_reports++; + hidpp->very_long_report_length = report_length; + } + + return supported_reports; + +bad_device: + hid_warn(hdev, "not enough values in hidpp report %d\n", id); + return false; } static bool hidpp_application_equals(struct hid_device *hdev, @@ -3572,11 +3583,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) return hid_hw_start(hdev, HID_CONNECT_DEFAULT); } - hidpp->very_long_report_length = - hidpp_get_report_length(hdev, REPORT_ID_HIDPP_VERY_LONG); - if (hidpp->very_long_report_length > HIDPP_REPORT_VERY_LONG_MAX_LENGTH) - hidpp->very_long_report_length = HIDPP_REPORT_VERY_LONG_MAX_LENGTH; - if (id->group == HID_GROUP_LOGITECH_DJ_DEVICE) hidpp->quirks |= HIDPP_QUIRK_UNIFYING;
G920 device only advertises REPORT_ID_HIDPP_LONG and REPORT_ID_HIDPP_VERY_LONG in its HID report descriptor, so querying for REPORT_ID_HIDPP_SHORT with optional=false will always fail and prevent G920 to be recognized as a valid HID++ device. To fix this and improve some other aspects, modify hidpp_validate_device() as follows: - Inline the code of hidpp_validate_report() to simplify distingushing between non-present and invalid report descriptors - Drop the check for id >= HID_MAX_IDS || id < 0 since all of our IDs are static and known to satisfy that at compile time - Change the algorithms to check all possible report types (including very long report) and deem the device as a valid HID++ device if it supports at least one - Treat invalid report length as a hard stop for the validation algorithm, meaning that if any of the supported reports has invalid length we assume the worst and treat the device as a generic HID device. - Fold initialization of hidpp->very_long_report_length into hidpp_validate_device() since it already fetches very long report length and validates its value Fixes: fe3ee1ec007b ("HID: logitech-hidpp: allow non HID++ devices to be handled by this module") Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204191 Reported-by: Sam Bazely <sambazley@fastmail.com> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> Cc: Jiri Kosina <jikos@kernel.org> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> Cc: Henrik Rydberg <rydberg@bitmath.org> Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com> Cc: Austin Palmer <austinp@valvesoftware.com> Cc: linux-input@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org # 5.2+ --- drivers/hid/hid-logitech-hidpp.c | 54 ++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 24 deletions(-)