| 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
| ||
|
Date: Wed, 25 May 2022 11:44:09 -0400
From: Guillaume Champagne <champagne.guillaume.c@...il.com>
To: Michael Zaidman <michael.zaidman@...il.com>
Cc: jikos@...nel.org, benjamin.tissoires@...hat.com, wsa@...nel.org,
linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
linux-i2c@...r.kernel.org,
Mathieu Gallichand <mathieu.gallichand@...atest.com>
Subject: Re: [PATCH v1 3/5] HID: ft260: support i2c writes larger than HID
report size
Le mer. 25 mai 2022 à 03:48, Michael Zaidman
<michael.zaidman@...il.com> a écrit :
>
> To support longer than one HID report size write, the driver splits a single
> i2c message data payload into multiple i2c messages of HID report size.
> However, it does not replicate the offset bytes within the EEPROM chip in
> every consequent HID report because it is not and should not be aware of
> the EEPROM type. It breaks the i2c write message integrity and causes the
> EEPROM device not to acknowledge the second HID report keeping the i2c bus
> busy until the ft260 controller reports failure.
>
I tested this whole patchset and it resolves the issue I raised
https://patchwork.kernel.org/project/linux-input/patch/20220524192422.13967-1-champagne.guillaume.c@gmail.com/,
thanks.
> This patch preserves the i2c write message integrity by manipulating the
> i2c flag bits across multiple HID reports to be seen by the EEPROM device
> as a single i2c write transfer.
>
> Before:
>
> $ sudo ./i2cperf -f 2 -o 2 -s 64 -r 0-0xff 13 0x51 -S
> Error: Sending messages failed: Input/output error
>
> [ +3.667741] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 60 d[0] 0x0
> [ +0.007330] ft260_hid_output_report_check_status: wait 6400 usec, len 64
> [ +0.000203] ft260_xfer_status: bus_status 0x40, clock 100
> [ +0.000001] ft260_i2c_write: rep 0xd1 addr 0x51 off 60 len 6 d[0] 0x0
> [ +0.002337] ft260_hid_output_report_check_status: wait 1000 usec, len 10
> [ +0.000157] ft260_xfer_status: bus_status 0x2e, clock 100
> [ +0.000241] ft260_i2c_reset: done
> [ +0.000003] ft260 0003:0403:6030.000E: ft260_i2c_write: failed to start transfer, ret -5
>
> After:
>
> $ sudo ./i2cperf -f 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S
>
> Fill block with increment via i2ctransfer by chunks
> -------------------------------------------------------------------
> data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
> -------------------------------------------------------------------
> 58986 86 256 2 128
>
> Signed-off-by: Michael Zaidman <michael.zaidman@...il.com>
> ---
> drivers/hid/hid-ft260.c | 45 ++++++++++++++++++++++++-----------------
> 1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 44106cadd746..bfda5b191a3a 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -378,41 +378,50 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev,
> }
>
> static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
> - int data_len, u8 flag)
> + int len, u8 flag)
> {
> - int len, ret, idx = 0;
> + int ret, wr_len, idx = 0;
> + bool first = true;
> struct hid_device *hdev = dev->hdev;
> struct ft260_i2c_write_request_report *rep =
> (struct ft260_i2c_write_request_report *)dev->write_buf;
>
> do {
> - if (data_len <= FT260_WR_DATA_MAX)
> - len = data_len;
> - else
> - len = FT260_WR_DATA_MAX;
> + rep->flag = 0;
> + if (first) {
> + rep->flag = FT260_FLAG_START;
I feel like multi packet transactions must still honor flag sent to
ft20_i2c_write. This adds a START even if ft260_i2c_write is called
with FT260_FLAG_START_REPEATED or FT260_FLAG_NONE.
> + first = false;
> + }
> +
> + if (len <= FT260_WR_DATA_MAX) {
> + wr_len = len;
> + if (flag == FT260_FLAG_START_STOP)
> + rep->flag |= FT260_FLAG_STOP;
> + } else {
> + wr_len = FT260_WR_DATA_MAX;
> + }
>
> - rep->report = FT260_I2C_DATA_REPORT_ID(len);
> + rep->report = FT260_I2C_DATA_REPORT_ID(wr_len);
> rep->address = addr;
> - rep->length = len;
> - rep->flag = flag;
> + rep->length = wr_len;
>
> - memcpy(rep->data, &data[idx], len);
> + memcpy(rep->data, &data[idx], wr_len);
>
> - ft260_dbg("rep %#02x addr %#02x off %d len %d d[0] %#02x\n",
> - rep->report, addr, idx, len, data[0]);
> + ft260_dbg("rep %#02x addr %#02x off %d len %d wlen %d flag %#x d[0] %#02x\n",
> + rep->report, addr, idx, len, wr_len,
> + rep->flag, data[0]);
>
> ret = ft260_hid_output_report_check_status(dev, (u8 *)rep,
> - len + 4);
> + wr_len + 4);
> if (ret < 0) {
> - hid_err(hdev, "%s: failed to start transfer, ret %d\n",
> - __func__, ret);
> + hid_err(hdev, "%s: failed with %d\n", __func__, ret);
> return ret;
> }
>
> - data_len -= len;
> - idx += len;
> + len -= wr_len;
> + idx += wr_len;
>
> - } while (data_len > 0);
> + } while (len > 0);
>
> return 0;
> }
> --
> 2.25.1
>
Powered by blists - more mailing lists