Re: [v3 4/5] HID: logitech-hidpp: Fix "Sw. Id." for HID++ 2.0 commands

[Benjamin Tissoires]

On Tue, Aug 30, 2022 at 1:39 PM Bastien Nocera <hadess@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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
>