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