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: <20220726080504.4185715-1-tero.kristo@linux.intel.com>
In-Reply-To: <20220726080504.4185715-1-tero.kristo@linux.intel.com>
From:   Angela Czubak <acz@semihalf.com>
Date:   Thu, 18 Aug 2022 12:27:33 +0200
Message-ID: <CAB4aORWWL7w1rVCrFw-Q2aof6WeQZR5iYRFg2UrvQQwHZzy7hA@mail.gmail.com>
Subject: Re: [PATCH] HID: i2c-hid: fix the report-id passed in via set_or_send_report
To:     Tero Kristo <tero.kristo@linux.intel.com>
Cc:     linux-input@vger.kernel.org, benjamin.tissoires@redhat.com,
        jikos@kernel.org, linux-kernel@vger.kernel.org,
        dmitry.torokhov@gmail.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Tero,

I believe this is not the right solution. To my mind the spec does not
mention anything that the ID in the payload of SET_REPORT command
should be altered; you have a full byte to us, so why not?
Other than that this will result into problems with sending reports
via output register: for any report with report ID >=0xF we will get
the same report ID in the payload, so how could the device know which
one we have in mind?
Dmitry's rework was intended to actually solve problems with large
report IDs being incorrectly overwritten with 0xF.

Regards,
Angela

On Tue, Jul 26, 2022 at 10:05 AM Tero Kristo
<tero.kristo@linux.intel.com> wrote:
>
> The formatting of the data passed to the i2c HID data register was
> changed with the re-work of the i2c-hid-core. Previously the report ID
> passed in was encoded as 0xF if the report-id was greater than 0xF
> (similar to what is done with the command portion.) Now with the rework,
> a full report-id is passed in always, and this causes the messages to be
> rejected by the i2c controller. Fix this by encoding the report-id
> field in the same manner as previously was done.
>
> Fixes: dbe0dd5fd2e0 ("HID: i2c-hid: explicitly code setting and sending
> reports")
> Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index c078f09a2318..156b12f840c4 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -296,6 +296,9 @@ static size_t i2c_hid_format_report(u8 *buf, int report_id,
>  {
>         size_t length = sizeof(__le16); /* reserve space to store size */
>
> +       if (report_id > 0xF)
> +               report_id = 0xF;
> +
>         if (report_id)
>                 buf[length++] = report_id;
>
> --
> 2.25.1
>