linux-kernel - Re: [PATCH] HID: ft260: fix multi packet i2c transactions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 25 May 2022 11:44:21 +0300
From:   Michael Zaidman <michael.zaidman@...il.com>
To:     Guillaume Champagne <champagne.guillaume.c@...il.com>
Cc:     jikos@...nel.org, benjamin.tissoires@...hat.com,
        linux-i2c@...r.kernel.org, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Mathieu Gallichand <mathieu.gallichand@...atest.com>
Subject: Re: [PATCH] HID: ft260: fix multi packet i2c transactions

On Tue, May 24, 2022 at 03:24:22PM -0400, Guillaume Champagne wrote:
> Only trigger START and STOP conditions for the first and last HID
> packets when i2c writes are split in multiple packets. Otherwise, slave
> i2c devices receive each packet as standalone i2c transactions. Since
> i2c slave devices clear their internal state on STOP, this breaks auto
> increment of the register address written to.
> 
> Concretely, SCL is now held low between processing of HID packets so i2c
> slave devices know to keep increment the same register address when the
> next bytes arrive.
> 
> Co-developed-by: Mathieu Gallichand <mathieu.gallichand@...atest.com>
> Signed-off-by: Mathieu Gallichand <mathieu.gallichand@...atest.com>
> Signed-off-by: Guillaume Champagne <champagne.guillaume.c@...il.com>
> ---
>  drivers/hid/hid-ft260.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 79505c64dbfe..9c5912a21ccb 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -390,6 +390,8 @@ static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
>  	struct hid_device *hdev = dev->hdev;
>  	struct ft260_i2c_write_request_report *rep =
>  		(struct ft260_i2c_write_request_report *)dev->write_buf;
> +	bool multi_packet = data_len > FT260_WR_DATA_MAX;
> +	u8 packet_flag = multi_packet ? flag & FT260_FLAG_START_REPEATED : flag;

Please take a look at the https://patchwork.kernel.org/project/linux-input/patch/20220525074757.7519-4-michael.zaidman@gmail.com/
that addresses the same issue but does it more efficiently by adding one
conditional statement per ft260_i2c_write call in the main path vs. three
in this commit. It comes in the patch set with other performance improvements
published on my GitHub https://github.com/MichaelZaidman/hid-ft260 several
months ago. I would greatly appreciate it if you could test and feedback
on the patch set content.

Thanks,
Michael