Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220830113907.4886-1-hadess@hadess.net> <20220830113907.4886-4-hadess@hadess.net>
In-Reply-To: <20220830113907.4886-4-hadess@hadess.net>
From:   Benjamin Tissoires <benjamin.tissoires@redhat.com>
Date:   Tue, 30 Aug 2022 15:19:15 +0200
Message-ID: <CAO-hwJJWDFS7zCbGAvbDCx0e+xNCtr_A4gBx0T5M8WeDVhnPgA@mail.gmail.com>
Subject: Re: [v3 4/5] HID: logitech-hidpp: Fix "Sw. Id." for HID++ 2.0 commands
To:     Bastien Nocera <hadess@hadess.net>
Cc:     "open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Jiri Kosina <jikos@kernel.org>,
        "Peter F . Patel-Schneider" <pfpschneider@gmail.com>,
        =?UTF-8?Q?Filipe_La=C3=ADns?= <lains@riseup.net>,
        Nestor Lopez Casado <nlopezcasad@logitech.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>