| Message ID | 20240110212641.1916202-1-komlodi@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hw/i2c: smbus: Reset fixes | expand |
+cminyard Accidentally typed Corey's email address wrong in the initial send, oops. On Wed, Jan 10, 2024 at 1:26 PM Joe Komlodi <komlodi@google.com> 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. > > 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 >
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 > >
On Thu, Jan 11, 2024 at 6:03 AM Corey Minyard <minyard@acm.org> wrote: > > 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 > Oops, sorry, missed this in my inbox. That sounds good to me, I'll send up a v2 that resets the SMBus master instead of the mux. Thanks, Joe > > > > 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 > > > >