Quoting Andrew Lunn <and...@lunn.ch>:
>>+ /* Many i2c hw have limited rx buffers, split-up request when needed.
*/
>>+ while ((q->max_read_len) && (len > q->max_read_len)) {
>>+ ret = sfp->read(sfp, a2, addr, buf, q->max_read_len);
>
>Hi René
>
>I think you want to pass MIN(len, q->max_read_len) to read().
Hi Andrew,
max_read_len is 0 when there is no quirk.
I can write it a bit differently depending on the outcome of my other email.
Hi René
No, you misunderstood me.
>>+ ret = sfp->read(sfp, a2, addr, buf, q->max_read_len);
Say max_read_len = 64
The SFP code asks to read 68 bytes. The first call to read() is going
to ask for 64 bytes. The second call is going to also ask for 64
bytes, writing 60 bytes passed the end of buf. Bad things then happen.
Hi Andrew,
This can't happen, while loop is only executed when len > max_read_len.
len is reduced after a successful read in the while loop.
So sizes <= max_read_len is handled by sfp->read below the while loop.
>>+ if (ret < 0)
>>+ return ret;
>>+ rx_bytes += ret;
>>+ addr += q->max_read_len;
>>+ buf += q->max_read_len;
>>+ len -= q->max_read_len;
>
>I would prefer you add ret, not q->max_read_len. There is a danger it
>returned less bytes than you asked for.
Getting less bytes then asked is already an error I think.
I could check the return size and directly return the number of bytes that I
have. The callers are checking for size and they can retry if
wanted. So that
should not be an issue.
If that is true, why is rx_bytes += ret, where as all the others are
+= q->max_read_len. Please be consistent. The general pattern of a
read function in POSIX systems is that it returns how many bytes were
actually returned. So i would always use += ret.
By reading the SSF spec we can write to a user writable EERPOM area of 120
bytes.
But the current code has only has 1 sfp_write for a byte value.
So for now I should say no.
So how about adding a WARN_ON. If the request is bigger than what the
quirk allows, make it very obvious we have an issue.
Andrew
i2c-core already reports an error in i2c_check_for_quirks [0].
Is that sufficient?
This is how I know that something was wrong.
But the driver should not retry it for infinitely like what is
happening with read.
And keeps spamming the error logs.
[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/i2c-core-base.c?h=v5.0-rc3#n1787
Greats,
René
