drivers/i2c: ite: Add handling for read operation with 0-byte length #90202
Conversation
github-actions
bot
requested review from
brockus-zephyr,
Dino-Li,
jackrosenthal,
keith-zephyr,
RuibinChang,
sjg20 and
teburd
drivers/i2c/i2c_ite_enhance.c
Outdated
| @@ -408,6 +411,7 @@ static int enhanced_i2c_error(const struct device *dev) | |||
| } else if ((i2c_str & E_HOSTA_BDS_AND_ACK) == E_HOSTA_BDS) { | |||
| if (IT8XXX2_I2C_CTR(base) & E_ACK) { | |||
| data->err = E_HOSTA_ACK; | |||
| data->nack = 1; | |||
There was a problem hiding this comment.
nack is a bool type.
| data->nack = 1; | |
| data->nack = true; |
drivers/i2c/i2c_ite_enhance.c
Outdated
| * following i2c_reset. | ||
| */ | ||
| if (data->nack) { | ||
| data->nack = 0; |
There was a problem hiding this comment.
| data->nack = 0; | |
| data->nack = false; |
drivers/i2c/i2c_ite_enhance.c
Outdated
| @@ -659,6 +682,8 @@ static int i2c_enhance_pio_transfer(const struct device *dev, | |||
| if (data->err || (data->active_msg->flags & I2C_MSG_STOP)) { | |||
| data->i2ccs = I2C_CH_NORMAL; | |||
| } | |||
| /* Clear the flag */ | |||
| data->nack = 0; | |||
There was a problem hiding this comment.
| data->nack = 0; | |
| data->nack = false; |
GTLin08
force-pushed
the
it8xxx2_i2c_eh_handle_read_0
branch
from
2f4d205 to
ffefb47
Compare
There was a problem hiding this comment.
The commit message could use with a bit more elaboration here as to what the problem is, why the change is needed, how this change solves the problem.
The changes themselves, from what I can tell, look ok.
Would highly recommend reducing the nesting level of the if/else dealing with what appears to be a hardware state machine flow into something more easily read by naming/assigning perhaps intermediate states in a more meaningful way.
The current I2C driver assumes that at least one byte will be read in CQ (command queue) mode. However, when a 0-byte read is issued (e.g., by cmd_i2c_scan), The read handler uses (len - 1) to set the command queue length. When len is 0, this underflows to 0xFF, leading to an incorrect transfer length and possible crash. To fix this, add a check in cq_mode_allowed() for reads with length 0: -Fallback to PIO mode in such cases. -Properly handle 0-byte reads by issuing STOP (E_FINISH) when the slave address is acknowledged. -Add appropriate handling for NACK conditions when the slave address is not acknowledged. Signed-off-by: Tim Lin <tim2.lin@ite.corp-partner.google.com>
GTLin08
force-pushed
the
it8xxx2_i2c_eh_handle_read_0
branch
from
ffefb47 to
137521a
Compare
Thank you for the feedback! |
|
The current I2C driver assumes that at least one byte will be read in CQ (command queue) mode. However, when a 0-byte read is issued(e.g.,
i2c read <bus> <addr> <reg> 0), the driver fails to handle it correctly.The read handler uses (len - 1) to set the command queue length. When len is 0, this underflows to 0xFF, leading to an incorrect transfer length and possible crash.
To fix this, add a check in cq_mode_allowed() for reads with length 0:
-Fallback to PIO mode in such cases.
-Properly handle 0-byte reads by issuing STOP (E_FINISH) when the slave address is acknowledged.
-Add appropriate handling for NACK conditions when the slave address is not acknowledged.