Re: [PATCH 0/4] hw/i2c: smbus: Reset fixes

[Corey Minyard <minyard@acm.org>]

On Wed, Jan 10, 2024 at 09:26:37PM +0000, Joe Komlodi wrote:
> Hi all,
> 
> This series adds some resets for SMBus and for the I2C core. Along with
> it, we make SMBus slave error printing a little more helpful.
> 
> These reset issues were very infrequent, they would maybe occur in 1 out
> of hundreds of resets in our testing, but the way they happen is pretty
> straightforward.
> Basically as long as a reset happens in the middle of a transaction, the
> state of the old transaction would still partially be there after the
> reset. Once a new transaction comes in, the partial stale state can
> cause the new transaction to incorrectly fail.

This seems wrong to me.  In a real system, the reset would be done on
the smbus master and not necessarily on the mux (though I looked at a
few of the PCA954x devices and they appear to have reset lines, but
different systems may drive that reset differently).

It seems to me that the bug is the smbus master device isn't getting
reset in a system reset.  Just adding the reset logic there would be
easier and more consistent with the real hardware.

-corey

> 
> Thanks,
> Joe
> 
> Joe Komlodi (4):
>   hw/i2c: core: Add reset
>   hw/i2c/smbus_slave: Add object path on error prints
>   hw/i2c: smbus_slave: Reset state on reset
>   hw/i2c: smbus: mux: Reset SMBusDevice state on reset
> 
>  hw/i2c/core.c                | 30 +++++++++++++++++++++++++-----
>  hw/i2c/i2c_mux_pca954x.c     |  5 +++++
>  hw/i2c/smbus_slave.c         | 20 ++++++++++++++++++--
>  include/hw/i2c/i2c.h         |  6 +++++-
>  include/hw/i2c/smbus_slave.h |  1 +
>  5 files changed, 54 insertions(+), 8 deletions(-)
> 
> -- 
> 2.43.0.472.g3155946c3a-goog
> 
>