| Message ID | 20220830113907.4886-4-hadess@hadess.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [v3,1/5] HID: core: Export hid_match_id() | expand |
On Tue, Aug 30, 2022 at 1:39 PM Bastien Nocera <hadess@hadess.net> wrote: > > Always set a non-zero "Sw. Id." in the lower nibble of the Function/ASE > and Software Identifier byte in HID++ 2.0 commands. > > As per the "Protocol HID++2.0 essential features" section in > https://lekensteyn.nl/files/logitech/logitech_hidpp_2.0_specification_draft_2012-06-04.pdf > " > Software identifier (4 bits, unsigned) > > A number uniquely defining the software that sends a request. The > firmware must copy the software identifier in the response but does > not use it in any other ways. > > 0 Do not use (allows to distinguish a notification from a response). > " > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215699 > Signed-off-by: Bastien Nocera <hadess@hadess.net> > --- > drivers/hid/hid-logitech-hidpp.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > index 98ebedb73d98..9c8088d8879e 100644 > --- a/drivers/hid/hid-logitech-hidpp.c > +++ b/drivers/hid/hid-logitech-hidpp.c > @@ -41,6 +41,9 @@ module_param(disable_tap_to_click, bool, 0644); > MODULE_PARM_DESC(disable_tap_to_click, > "Disable Tap-To-Click mode reporting for touchpads (only on the K400 currently)."); > > +/* Define a non-zero software ID to identify our own requests */ > +#define LINUX_KERNEL_SW_ID 0x06 For consistency, and as Peter already asked, please use 0x01 instead of 0x06. The simple reason is that it was well known that the kernel used 0x01 from day one, and so we might have userspace application that uses 0x06, and in this case you are walking on their toes. Cheers, Benjamin > + > #define REPORT_ID_HIDPP_SHORT 0x10 > #define REPORT_ID_HIDPP_LONG 0x11 > #define REPORT_ID_HIDPP_VERY_LONG 0x12 > @@ -343,7 +346,7 @@ static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp, > else > message->report_id = REPORT_ID_HIDPP_LONG; > message->fap.feature_index = feat_index; > - message->fap.funcindex_clientid = funcindex_clientid; > + message->fap.funcindex_clientid = funcindex_clientid | LINUX_KERNEL_SW_ID; > memcpy(&message->fap.params, params, param_count); > > ret = hidpp_send_message_sync(hidpp, message, response); > -- > 2.37.2 >
On Tue, 2022-08-30 at 15:19 +0200, Benjamin Tissoires wrote: > On Tue, Aug 30, 2022 at 1:39 PM Bastien Nocera <hadess@hadess.net> > wrote: > > > > Always set a non-zero "Sw. Id." in the lower nibble of the > > Function/ASE > > and Software Identifier byte in HID++ 2.0 commands. > > > > As per the "Protocol HID++2.0 essential features" section in > > https://lekensteyn.nl/files/logitech/logitech_hidpp_2.0_specification_draft_2012-06-04.pdf > > " > > Software identifier (4 bits, unsigned) > > > > A number uniquely defining the software that sends a request. The > > firmware must copy the software identifier in the response but does > > not use it in any other ways. > > > > 0 Do not use (allows to distinguish a notification from a > > response). > > " > > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215699 > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > > --- > > drivers/hid/hid-logitech-hidpp.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid- > > logitech-hidpp.c > > index 98ebedb73d98..9c8088d8879e 100644 > > --- a/drivers/hid/hid-logitech-hidpp.c > > +++ b/drivers/hid/hid-logitech-hidpp.c > > @@ -41,6 +41,9 @@ module_param(disable_tap_to_click, bool, 0644); > > MODULE_PARM_DESC(disable_tap_to_click, > > "Disable Tap-To-Click mode reporting for touchpads (only on > > the K400 currently)."); > > > > +/* Define a non-zero software ID to identify our own requests */ > > +#define LINUX_KERNEL_SW_ID 0x06 > > For consistency, and as Peter already asked, please use 0x01 instead > of 0x06. > > The simple reason is that it was well known that the kernel used 0x01 > from day one, and so we might have userspace application that uses > 0x06, and in this case you are walking on their toes. Done in v4, thanks. > > Cheers, > Benjamin > > > + > > #define REPORT_ID_HIDPP_SHORT 0x10 > > #define REPORT_ID_HIDPP_LONG 0x11 > > #define REPORT_ID_HIDPP_VERY_LONG 0x12 > > @@ -343,7 +346,7 @@ static int hidpp_send_fap_command_sync(struct > > hidpp_device *hidpp, > > else > > message->report_id = REPORT_ID_HIDPP_LONG; > > message->fap.feature_index = feat_index; > > - message->fap.funcindex_clientid = funcindex_clientid; > > + message->fap.funcindex_clientid = funcindex_clientid | > > LINUX_KERNEL_SW_ID; > > memcpy(&message->fap.params, params, param_count); > > > > ret = hidpp_send_message_sync(hidpp, message, response); > > -- > > 2.37.2 > > >
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 98ebedb73d98..9c8088d8879e 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -41,6 +41,9 @@ module_param(disable_tap_to_click, bool, 0644); MODULE_PARM_DESC(disable_tap_to_click, "Disable Tap-To-Click mode reporting for touchpads (only on the K400 currently)."); +/* Define a non-zero software ID to identify our own requests */ +#define LINUX_KERNEL_SW_ID 0x06 + #define REPORT_ID_HIDPP_SHORT 0x10 #define REPORT_ID_HIDPP_LONG 0x11 #define REPORT_ID_HIDPP_VERY_LONG 0x12 @@ -343,7 +346,7 @@ static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp, else message->report_id = REPORT_ID_HIDPP_LONG; message->fap.feature_index = feat_index; - message->fap.funcindex_clientid = funcindex_clientid; + message->fap.funcindex_clientid = funcindex_clientid | LINUX_KERNEL_SW_ID; memcpy(&message->fap.params, params, param_count); ret = hidpp_send_message_sync(hidpp, message, response);
Always set a non-zero "Sw. Id." in the lower nibble of the Function/ASE and Software Identifier byte in HID++ 2.0 commands. As per the "Protocol HID++2.0 essential features" section in https://lekensteyn.nl/files/logitech/logitech_hidpp_2.0_specification_draft_2012-06-04.pdf " Software identifier (4 bits, unsigned) A number uniquely defining the software that sends a request. The firmware must copy the software identifier in the response but does not use it in any other ways. 0 Do not use (allows to distinguish a notification from a response). " Link: https://bugzilla.kernel.org/show_bug.cgi?id=215699 Signed-off-by: Bastien Nocera <hadess@hadess.net> --- drivers/hid/hid-logitech-hidpp.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)