Driver i2c_imx: Fix system fault when there is no data to send #30169
Conversation
anthonytex
force-pushed
the
nxp-i2c-fix
branch
2 times, most recently
from
b7184fe to
721b1f7
Compare
drivers/i2c/i2c_imx.c
Outdated
There was a problem hiding this comment.
Apparently I don't have this SoC, but #27896 fixed a similar problem where the NAK from the address did not get delivered promptly: it would normally arrive during the transfer of the data. There the solution was to wait for some period to see whether it would show up. Is that the problem seen on this platform? If so, I don't see how this can work, as it simply bypasses the write and returns immediately with no error.
What platform are you using?
There was a problem hiding this comment.
Well it doesn't seem to be the same problem: without checking message length in this case i2c_imx_write() is called directly but that function doesn't check size of message before doing this:
https://github.com/zephyrproject-rtos/zephyr/blob/721b1f7c0c5cb901f1d49caafbe2e161ae2f62c2/drivers/i2c/i2c_imx.c#L69-L71
and so when a IRQ is received transfer->txSize is 0xff and the buffer is somewhere in the address space
There was a problem hiding this comment.
sorry my platform is a custom board based on imx6sx and i've also a imx6sx sabre and a a pico pi imx7d
There was a problem hiding this comment.
I'm not questioning the need to avoid the write when the length is zero. What I'm concerned about is what happens if an ack was (not) received for a zero-byte transfer.
If this I2C peripheral guarantees to indicate the NAK immediately this fix is fine. If, like the LPI2C, the NAK can be delayed, then it might falsely indicate presence of a device when one isn't actually there.
Absent evidence that a delay is possible I suspect this is fine, but want @MaureenHelm to weigh in.
There was a problem hiding this comment.
Ok now i understand 😄 , it's seem that the NAK is received immediately but to be honest i'll wait a confirmation from @MaureenHelm thanks
drivers/i2c/i2c_imx.c
Outdated
There was a problem hiding this comment.
If you moved this check up to enclose the entire /* Transfer data */ section that would also prevent breakage if somebody issues a read of length zero.
There was a problem hiding this comment.
Thanks for your review @pabigot !
True also read is broken , i'll update my pr this weekend.
|
@pabigot shouldn't also be an |
That would seem to make sense, yes.
I'd noticed that too after I replied. The driver probably could benefit from a closer look overall. Fix what you think is important (in separate commits where appropriate, e.g. changing the size type is unrelated to avoiding zero-length writes), but there's no obligation to clean up a module in ways unrelated to your main goal. |
When there is no data to send (e.g. i2c message with NULL buffer and len=0), i2c_imx driver locks itself in isr handling forever. So if there is no data to send, only device's address must be written on bus. This fixes i2c_shell's scan command on both imx7 and imx6sx. Signed-off-by: Antonio Tessarolo <[email protected]>
anthonytex
force-pushed
the
nxp-i2c-fix
branch
2 times, most recently
from
0fcf64e to
277949b
Compare
imx_msg transmission size type is uint32_t: changed imx_read and imx_write signature accordingly. Removed also two unused variables. Signed-off-by: Antonio Tessarolo <[email protected]>
anthonytex
force-pushed
the
nxp-i2c-fix
branch
from
277949b to
1369790
Compare
|
@pabigot Hello, i've updated the PR. if i'll find something else during the development of my project i'll open new PRs. |
4 participants
When there is no data to send (e.g. i2c message with NULL buffer and len=0), i2c_imx driver locks itself in isr handling forever.
So if there is no data to send, only device's address must be written on bus. This fixes i2c_shell's scan command on both imx7 and imx6sx.