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

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

[Ellen Wang]

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-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org>
---
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);
+		if (ret < 0)
+			goto power_normal;
+		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

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

[Antonio Borneo]

On Thu, Jun 18, 2015 at 8:34 AM, Ellen Wang <ellen-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org> 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-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org>

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.

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

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

[Antonio Borneo]

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

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

[Ellen Wang]

Sorry this fell off my todo list for a while.

On 06/20/2015 11:30 PM, Antonio Borneo wrote:
> On Sat, Jun 20, 2015 at 11:10 PM, Antonio Borneo
> <borneo.antonio-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Thu, Jun 18, 2015 at 8:34 AM, Ellen Wang <ellen-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org> 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-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org>
>>
>> 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.

Sigh.  I just reread the documentation 
(https://www.silabs.com/Support%20Documents/TechnicalDocs/AN495.pdf). 
It doesn't say one way or the other but it does seem to imply one 
CP2112_DATA_READ_RESPONSE is returned for each 
CP2112_DATA_READ_FORCE_SEND.  Sorry about the bug.

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

Absolutely.  The loop should always make progress.  I will send an 
updated patch.  I should have handled that case in the first place.

Also, I took a look at cp2112_raw_event() and cp2112_read().  There's 
really no fundamental reason the code can't get all the data with a 
single CP2112_DATA_READ_FORCE_SEND.  It just has to arrange for 
cp2112_raw_event() to fill the i2c_msg buffer directly.

For that matter, it can probably set auto_send_read and do away with 
CP2112_DATA_READ_FORCE_SEND all together.

There are some complications (like the wait timeout value may have to 
change) and it will take a lot of testing so I should do the safe fix first.

>> Thanks,
>> Antonio

Thanks!