Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751332AbaLRR51 (ORCPT ); Thu, 18 Dec 2014 12:57:27 -0500 Received: from mail-qg0-f44.google.com ([209.85.192.44]:44700 "EHLO mail-qg0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751033AbaLRR5Z (ORCPT ); Thu, 18 Dec 2014 12:57:25 -0500 MIME-Version: 1.0 In-Reply-To: <4013567.XXXMc2jNn9@al> References: <1418691016-30681-1-git-send-email-peter@lekensteyn.nl> <1418691016-30681-2-git-send-email-peter@lekensteyn.nl> <4013567.XXXMc2jNn9@al> Date: Thu, 18 Dec 2014 12:57:24 -0500 Message-ID: Subject: Re: [PATCH 1/3] HID: logitech-hidpp: detect HID++ 2.0 errors too From: Benjamin Tissoires To: Peter Wu Cc: Jiri Kosina , Nestor Lopez Casado , linux-input , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 18, 2014 at 12:26 PM, Peter Wu wrote: > On Tuesday 16 December 2014 09:33:44 Benjamin Tissoires wrote: >> Hi Peter, >> >> On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu wrote: >> > Devices speaking HID++ 2.0 report a different error code (0xff). Detect >> > these errors too to avoid 5 second delays when the device reports an >> > error. Caught by... well, a bug in the QEMU emulation of this receiver. >> > >> > Renamed fap to rap for HID++ 1.0 errors because it is more logical, >> > it has no functional difference. >> > >> > Signed-off-by: Peter Wu >> > --- >> >> I'd like to have Nestor's opinion on this. I did not manage to find on >> the documentation that HID++ 2.0 Long report error code is 0xff, so >> introducing this change without Logitech's blessing would be >> unfortunate. >> I understand this will fix your qemu problem, but I am not entirely >> sure if we do not have to check on 0xff and 0x8f in both short and >> long responses. >> >> Cheers, >> Benjamin > > Hi Benjamin, > > The Logitech Unifying extension for Chrome[1] is documented quite well > and contains details which were not public before (including names and > descriptions for all registers and subIDs!). > > In lib/devices/HidppFap.js you can find this logic for handling HID++ > 2.0 messages: > > if ((reqView.getUint8(1) == rspView.getUint8(1)) // device index > && (reqView.getUint8(2) == rspView.getUint8(2)) // feature index > && (reqView.getUint8(3) == rspView.getUint8(3))) // function/event ID + software ID > { > result.matchResult = devices.MATCH_RESULT.SUCCESS; > } else if ((reqView.getUint8(1) == rspView.getUint8(1)) // device index > && (0xFF == rspView.getUint8(2)) // Hid++ 2.0 error > && (reqView.getUint8(2) == rspView.getUint8(3)) // feature index > && (reqView.getUint8(3) == rspView.getUint8(4))) // function/event ID + software ID > { > result.errCode = rspView.getUint8(5); // FAP_ERROR > result.matchResult = devices.MATCH_RESULT.ERROR; > } > > Looks like a sufficient proof that 0xFF is the correct number to detect > HID++ 2.0 errors right? Cool :) Then the patch is: Reviewed-by: Benjamin Tissoires Cheers, Benjamin > > In HID++ 1.0 devices ("rap"), 0xFF is named as "SYNC" (with no further > comments), so this will probably not trigger false positives either. > > Kind regards, > Peter > > [1]: https://chrome.google.com/webstore/detail/logitech-unifying-for-chr/agpmgihmmmfkbhckmciedmhincdggomo > >> > drivers/hid/hid-logitech-hidpp.c | 17 ++++++++++++++--- >> > 1 file changed, 14 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c >> > index 2f420c0..ae23dec 100644 >> > --- a/drivers/hid/hid-logitech-hidpp.c >> > +++ b/drivers/hid/hid-logitech-hidpp.c >> > @@ -105,6 +105,7 @@ struct hidpp_device { >> > }; >> > >> > >> > +/* HID++ 1.0 error codes */ >> > #define HIDPP_ERROR 0x8f >> > #define HIDPP_ERROR_SUCCESS 0x00 >> > #define HIDPP_ERROR_INVALID_SUBID 0x01 >> > @@ -119,6 +120,8 @@ struct hidpp_device { >> > #define HIDPP_ERROR_REQUEST_UNAVAILABLE 0x0a >> > #define HIDPP_ERROR_INVALID_PARAM_VALUE 0x0b >> > #define HIDPP_ERROR_WRONG_PIN_CODE 0x0c >> > +/* HID++ 2.0 error codes */ >> > +#define HIDPP20_ERROR 0xff >> > >> > static void hidpp_connect_event(struct hidpp_device *hidpp_dev); >> > >> > @@ -192,9 +195,16 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp, >> > } >> > >> > if (response->report_id == REPORT_ID_HIDPP_SHORT && >> > - response->fap.feature_index == HIDPP_ERROR) { >> > + response->rap.sub_id == HIDPP_ERROR) { >> > + ret = response->rap.params[1]; >> > + dbg_hid("%s:got hidpp error %02X\n", __func__, ret); >> > + goto exit; >> > + } >> > + >> > + if (response->report_id == REPORT_ID_HIDPP_LONG && >> > + response->fap.feature_index == HIDPP20_ERROR) { >> > ret = response->fap.params[1]; >> > - dbg_hid("__hidpp_send_report got hidpp error %02X\n", ret); >> > + dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret); >> > goto exit; >> > } >> > >> > @@ -271,7 +281,8 @@ static inline bool hidpp_match_answer(struct hidpp_report *question, >> > static inline bool hidpp_match_error(struct hidpp_report *question, >> > struct hidpp_report *answer) >> > { >> > - return (answer->fap.feature_index == HIDPP_ERROR) && >> > + return ((answer->rap.sub_id == HIDPP_ERROR) || >> > + (answer->fap.feature_index == HIDPP20_ERROR)) && >> > (answer->fap.funcindex_clientid == question->fap.feature_index) && >> > (answer->fap.params[0] == question->fap.funcindex_clientid); >> > } >> > -- >> > 2.1.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/