linux-kernel - Re: [PATCH v1 3/5] HID: ft260: support i2c writes larger than HID report size

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220525194242.GA13454@michael-VirtualBox>
Date:   Wed, 25 May 2022 22:42:42 +0300
From:   Michael Zaidman <michael.zaidman@...il.com>
To:     Guillaume Champagne <champagne.guillaume.c@...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

On Wed, May 25, 2022 at 11:44:09AM -0400, Guillaume Champagne wrote:
> 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.

Much appreciated!
I will add your tested-by in the second version of the patchset.

> 
> > 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.

We use the FT260_FLAG_START_REPEATED to precede the Read message following
the Write message in the i2c combined transaction. Am I missing any i2c
protocol case using the Repeated Start in the Write path?

The FT260_FLAG_NONE should not be passed into the ft20_i2c_write as well.

So, we can keep it simple.