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@xxxxxxxxx> wrote:
On Thu, Jun 18, 2015 at 8:34 AM, Ellen Wang <ellen@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
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!
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
