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 <[email protected]>
---
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
+
#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, Aug 30, 2022 at 1:39 PM Bastien Nocera <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
> 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 <[email protected]>
> > ---
> > 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
> >
>