i2c / smbus emulation: bad handling of truncated xfers |
||||
Issue descriptionRelated to bug 848112 , except this is a kernel bug. This is also one of the causes of battery misreadings on Scarlet, which may lead to unexpected shutdowns as noted here and following: https://issuetracker.google.com/74120207#comment22 --- The kernel I2C core doesn't actually check for partial command failures -- typical I2C/SMBUS checks consist of a write followed by a read; the write command succeeds but the read command fails (due to the above lack of support). See code like this: https://elixir.bootlin.com/linux/v4.16.13/source/drivers/i2c/i2c-core-smbus.c#L462 status = i2c_transfer(adapter, msg, num); if (status < 0) return status; Note how 'status' might be positive but not equal to 'num' -- this means we didn't succeed at transferring all messages, and so we can't rely on all of the returned data being valid. But...we ignore that, and instead continue with potential garbage. The kernel treats this as "success", which is really really wrong, but apparently hasn't caused problems so far. This *is* problematic if I want to start catching I2C protocol errors correctly. The obvious solution would be to propagate an error if 'status != num', but this would trip up on stuff like bug 848112 . It's possible we could convince the sbs-battery driver to handle this more gracefully...
,
Jun 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ce5cd79eef8eb340852d06e66513ba4e0072a350 commit ce5cd79eef8eb340852d06e66513ba4e0072a350 Author: Wenwen Wang <wang6495@umn.edu> Date: Fri Jun 22 18:37:56 2018 BACKPORT: i2c: core: smbus: fix a potential missing-check bug In i2c_smbus_xfer_emulated(), the function i2c_transfer() is invoked to transfer i2c messages. The number of actual transferred messages is returned and saved to 'status'. If 'status' is negative, that means an error occurred during the transfer process. In that case, the value of 'status' is an error code to indicate the reason of the transfer failure. In most cases, i2c_transfer() can transfer 'num' messages with no error. And so 'status' == 'num'. However, due to unexpected errors, it is probable that only partial messages are transferred by i2c_transfer(). As a result, 'status' != 'num'. This special case is not checked after the invocation of i2c_transfer() and can potentially lead to unexpected issues in the following execution since it is expected that 'status' == 'num'. This patch checks the return value of i2c_transfer() and returns an error code -EIO if the number of actual transferred messages 'status' is not equal to 'num'. Signed-off-by: Wenwen Wang <wang6495@umn.edu> Signed-off-by: Wolfram Sang <wsa@the-dreams.de> (cherry picked from commit 8e03477cb709b73a2c1e1f4349ee3b7b33c50416) Conflicts: drivers/i2c/i2c-core-smbus.c [smbus was split from -core upstream] BUG= chromium:848119 TEST=sbs-battery on kevin, scarlet Change-Id: I5469cd47d17eb6dc06c0cf4733181da58281e422 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1111272 Commit-Ready: Douglas Anderson <dianders@chromium.org> Reviewed-by: Stephen Boyd <swboyd@chromium.org> [modify] https://crrev.com/ce5cd79eef8eb340852d06e66513ba4e0072a350/drivers/i2c/i2c-core.c
,
Jun 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/d5096fba69a435b3c5af3f9142247f2d691ba725 commit d5096fba69a435b3c5af3f9142247f2d691ba725 Author: Wenwen Wang <wang6495@umn.edu> Date: Fri Jun 22 18:37:50 2018 UPSTREAM: i2c: core: smbus: fix a potential missing-check bug In i2c_smbus_xfer_emulated(), the function i2c_transfer() is invoked to transfer i2c messages. The number of actual transferred messages is returned and saved to 'status'. If 'status' is negative, that means an error occurred during the transfer process. In that case, the value of 'status' is an error code to indicate the reason of the transfer failure. In most cases, i2c_transfer() can transfer 'num' messages with no error. And so 'status' == 'num'. However, due to unexpected errors, it is probable that only partial messages are transferred by i2c_transfer(). As a result, 'status' != 'num'. This special case is not checked after the invocation of i2c_transfer() and can potentially lead to unexpected issues in the following execution since it is expected that 'status' == 'num'. This patch checks the return value of i2c_transfer() and returns an error code -EIO if the number of actual transferred messages 'status' is not equal to 'num'. Signed-off-by: Wenwen Wang <wang6495@umn.edu> Signed-off-by: Wolfram Sang <wsa@the-dreams.de> (cherry picked from commit 8e03477cb709b73a2c1e1f4349ee3b7b33c50416) BUG= chromium:848119 TEST=sbs-battery on kevin, scarlet Change-Id: I5469cd47d17eb6dc06c0cf4733181da58281e422 Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1111207 Commit-Ready: Douglas Anderson <dianders@chromium.org> Reviewed-by: Stephen Boyd <swboyd@chromium.org> [modify] https://crrev.com/d5096fba69a435b3c5af3f9142247f2d691ba725/drivers/i2c/i2c-core-smbus.c
,
Jun 22 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by briannorris@chromium.org
, Jun 21 2018Status: Started (was: Untriaged)