Re: [PATCH] HID: ft260: fix SMBus block read protocol handling

[Michael Zaidman]

Hi Raman,

Thanks for the patch. Your analysis matches what I see in the driver:
on a block read the count comes from the slave, data->block[0] is not
initialized by the caller, so sizing the read from data->block[0] + 1
is wrong. Reading and validating the count first is the correct fix
for the FT260.

I'm validating on FT260 hardware and will follow up on this thread with
Tested-by/Acked-by when that is complete.

Reviewed-by: Michael Zaidman <michaelz@xxxxxxxxxxxxxx>

On Wed, Jun 10, 2026 at 4:42 PM Raman Varabets
<kernel-linux-20260610-80b7ab08@xxxxxxxxxxx> wrote:
>
> For I2C_SMBUS_BLOCK_DATA reads, ft260_smbus_xfer() passed
> data->block[0] + 1 as the read length. But on a block read the byte
> count is supplied by the slave as the first byte of the response;
> data->block[0] is not initialized by the caller, so the transfer
> length was taken from stale buffer contents, and the count byte the
> slave did return was stored without any validation.
>
> Implement the SMBus 2.0 block read protocol properly: read the count
> byte first with a repeated START and no STOP, validate it against
> I2C_SMBUS_BLOCK_MAX (resetting the bus and returning -EPROTO on a
> bogus count), then read exactly that many data bytes and finish the
> transaction with STOP. This keeps the whole sequence within a single
> I2C transaction:
>
>   S Addr+Wr A Reg A Sr Addr+Rd A Count A Data... P
>
> To support issuing the two reads as one transaction, teach
> ft260_i2c_read() to honor the caller's flags instead of always
> forcing a START and unconditionally appending STOP to the last
> chunk: START is only emitted if requested, and STOP is appended to
> the final chunk only when the caller asked for it.
>
> Signed-off-by: Raman Varabets <kernel-linux-20260610-80b7ab08@xxxxxxxxxxx>
> ---
>  drivers/hid/hid-ft260.c | 45 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 70e2eedb4..946ed0c6f 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -502,15 +502,23 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
>         struct ft260_i2c_read_request_report rep;
>         struct hid_device *hdev = dev->hdev;
>         u8 bus_busy = 0;
> +       /*
> +        * STOP terminates the last chunk; clear means hold the bus so a
> +        * follow-up call continues the same I2C transaction.
> +        */
> +       bool want_stop = !!(flag & FT260_FLAG_STOP);
>
>         if ((flag & FT260_FLAG_START_REPEATED) == FT260_FLAG_START_REPEATED)
>                 flag = FT260_FLAG_START_REPEATED;
> -       else
> +       else if (flag & FT260_FLAG_START)
>                 flag = FT260_FLAG_START;
> +       else
> +               flag = 0;       /* no fresh START - continue current transaction */
>         do {
>                 if (len <= rd_data_max) {
>                         rd_len = len;
> -                       flag |= FT260_FLAG_STOP;
> +                       if (want_stop)
> +                               flag |= FT260_FLAG_STOP;
>                 } else {
>                         rd_len = rd_data_max;
>                 }
> @@ -708,14 +716,41 @@ static int ft260_smbus_xfer(struct i2c_adapter *adapter, u16 addr, u16 flags,
>                 break;
>         case I2C_SMBUS_BLOCK_DATA:
>                 if (read_write == I2C_SMBUS_READ) {
> +                       u8 count = 0;
> +
> +                       /*
> +                        * SMBus 2.0 section 6.5.7 block read in one I2C
> +                        * transaction:
> +                        *
> +                        *   S Addr+Wr A Reg A Sr Addr+Rd A Count A Data... P
> +                        *
> +                        * The count is read separately and validated
> +                        * before sizing the data read so a misbehaving
> +                        * slave cannot drive a write past data->block[].
> +                        */
>                         ret = ft260_smbus_write(dev, addr, cmd, NULL, 0,
>                                                 FT260_FLAG_START);
>                         if (ret)
>                                 goto smbus_exit;
>
> -                       ret = ft260_i2c_read(dev, addr, data->block,
> -                                            data->block[0] + 1,
> -                                            FT260_FLAG_START_STOP_REPEATED);
> +                       ret = ft260_i2c_read(dev, addr, &count, 1,
> +                                            FT260_FLAG_START_REPEATED);
> +                       if (ret)
> +                               goto smbus_exit;
> +
> +                       if (count == 0 || count > I2C_SMBUS_BLOCK_MAX) {
> +                               hid_warn(hdev,
> +                                        "smbus block read: invalid count %u from slave 0x%02x\n",
> +                                        count, addr);
> +                               ft260_i2c_reset(hdev);
> +                               ret = -EPROTO;
> +                               goto smbus_exit;
> +                       }
> +
> +                       data->block[0] = count;
> +
> +                       ret = ft260_i2c_read(dev, addr, data->block + 1,
> +                                            count, FT260_FLAG_STOP);
>                 } else {
>                         ret = ft260_smbus_write(dev, addr, cmd, data->block,
>                                                 data->block[0] + 1,
>
> base-commit: ab5fce87a778cb780a05984a2ca448f2b41aafbf
> --
> 2.54.0
>