Re: [PATCH v1] HID: cp2112: support large i2c transfers in hid-cp2112

[Antonio Borneo <borneo.antonio@gmail.com>]

On Sat, Jun 20, 2015 at 11:10 PM, Antonio Borneo
<borneo.antonio@gmail.com> wrote:
> On Thu, Jun 18, 2015 at 8:34 AM, Ellen Wang <ellen@cumulusnetworks.com> wrote:
>> cp2112_i2c_xfer() only reads up to 61 bytes, returning EIO
>> on longers reads.  The fix is to wrap a loop around
>> cp2112_read() to pick up all the returned data.
>>
>> Signed-off-by: Ellen Wang <ellen@cumulusnetworks.com>
>
> Hi Ellen,
>
> with this patch the driver occasionally enters in an infinite loop.
> I spent some time to understand the reason.
>
> The sequence for a data read in cp2112_i2c_xfer() is:
> 1) send report CP2112_DATA_READ_REQUEST (no reply is expected)
> 2) send report CP2112_TRANSFER_STATUS_REQUEST
> 3) wait for reply report CP2112_TRANSFER_STATUS_RESPONSE to indicate
> i2c read completed
> 4) send report CP2112_DATA_READ_FORCE_SEND
> 5) wait for reply report CP2112_DATA_READ_RESPONSE containing the data just read
>
> Your patch repeats step 4) and 5) until all data are received.
>
> Every report CP2112_DATA_READ_RESPONSE can carry max 61 bytes of data.
> It is not reported in Silab documentation (or maybe I failed to find
> it), but if you send a request CP2112_DATA_READ_FORCE_SEND for _more_
> than 61 bytes then cp2112 replies with a sequence of reports
> CP2112_DATA_READ_RESPONSE, each report carrying 61 bytes max.
>
> To get only one report as reply, the request in 4) should not exceed 61 bytes!
>
> The current code in cp2112_raw_event() is very simple and can only
> handle receiving one data report at a time; it's not designed to
> handle a sequence of reports.
> If a new incoming report arrives while we are still consuming a
> previous report, the new data will overwrite the older one.
>
> If the loop over 4) and 5) is not fast enough (e.g. CPU overloaded,
> interrupts) then you get reports overwritten.
> Once one report is overwritten, we fail to get the whole data, the
> loop will not reach the upper limit and will never exit!
>
> I got this case just adding a hid_info() inside the loop.
> If you want, you can check by adding a msleep(100) inside the loop.
> Enough to get infinite loop at almost every execution.
>
> Hints in the code below:
>
>> ---
>> This is like the previous patch but with the controversial
>> part left out.
>> ---
>>  drivers/hid/hid-cp2112.c |   26 +++++++++++++++++++-------
>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
>> index 3318de6..5a72819 100644
>> --- a/drivers/hid/hid-cp2112.c
>> +++ b/drivers/hid/hid-cp2112.c
>> @@ -509,13 +509,25 @@ static int cp2112_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>>         if (!(msgs->flags & I2C_M_RD))
>>                 goto finish;
>>
>> -       ret = cp2112_read(dev, msgs->buf, msgs->len);
>> -       if (ret < 0)
>> -               goto power_normal;
>> -       if (ret != msgs->len) {
>> -               hid_warn(hdev, "short read: %d < %d\n", ret, msgs->len);
>> -               ret = -EIO;
>> -               goto power_normal;
>> +       for (count = 0; count < msgs->len;) {
>> +               ret = cp2112_read(dev, msgs->buf + count, msgs->len - count);
>
> Limit the read to 61 bytes with a check like
>         if (size > 61)
>                 size = 61;
>         ret = cp2112_read(..., size);
> This guarantees we get back only one report at a time.
> Instead of the magic number 61, you can use sizeof(dev->read_data).
>
> Or, better, put the check inside cp2112_read(). We are not supposed to
> use this function for more than 61 bytes due to current simple
> cp2112_raw_event(). Please also comment the change in cp2112_read().
> The code in cp2112_read() expects only one report of data. Seams the
> proper place to limit the amount of data requested.

Hi Ellen,

at last I changed mind!
The multi-report issus is a bug of current code and must be fixed separately.
I just sent out a patch for it, tagging it for linux-stable too.

Regarding you patch.
No need to handle the case of size > 61, supposed already fixed. Just
keep your code as is.
But please rise an error in case of ret == 0 to avoid infinite loop.

Best Regards,
Antonio

>
>> +               if (ret < 0)
>> +                       goto power_normal;
>
> If ret == 0 it means we have lost one report and the operation should
> be aborted.
> I cannot imagine what could cause it (maybe weak USB contacts or line
> noise), but for sure this return value is unexpected.
> Please generate error for ret == 0 so we never get infinite loop.
>
> Thanks,
> Antonio
>
>> +               count += ret;
>> +               if (count > msgs->len) {
>> +                       /*
>> +                        * The hardware returned too much data.
>> +                        * This is mostly harmless because cp2112_read()
>> +                        * has a limit check so didn't overrun our
>> +                        * buffer.  Nevertheless, we return an error
>> +                        * because something is seriously wrong and
>> +                        * it shouldn't go unnoticed.
>> +                        */
>> +                       hid_err(hdev, "long read: %d > %zd\n",
>> +                               ret, msgs->len - count + ret);
>> +                       ret = -EIO;
>> +                       goto power_normal;
>> +               }
>>         }
>>
>>  finish:
>> --
>> 1.7.10.4
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in