Hi Jan On Wed, Sep 13, 2023 at 04:29:38PM -0700, Jan Bottorff wrote: > Errors were happening in the ISR that looked like corrupted > memory. This was because memory writes from the core enabling > interrupts were not yet visible to the core running the ISR. The > kernel log would get the message "i2c_designware APMC0D0F:00: > controller timed out" during in-band IPMI SSIF stress tests. > > Add a write barrier before enabling interrupts to assure data written > by the current core is visible to all cores before the interrupt fires. > > The ARM Barrier Litmus Tests and Cookbook has an example under > Sending Interrupts and Barriers that matches the usage in this > driver. That document says a DSB barrier is required. > > Signed-off-by: Jan Bottorff <janb@xxxxxxxxxxxxxxxxxxxxxx> > Reviewed-by: Yann Sionneau <ysionneau@xxxxxxxxxxxxx> > Tested-by: Yann Sionneau <ysionneau@xxxxxxxxxxxxx> > --- > drivers/i2c/busses/i2c-designware-master.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c > index ca1035e010c7..1694ac6bb592 100644 > --- a/drivers/i2c/busses/i2c-designware-master.c > +++ b/drivers/i2c/busses/i2c-designware-master.c > @@ -248,6 +248,14 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev) > /* Dummy read to avoid the register getting stuck on Bay Trail */ > regmap_read(dev->map, DW_IC_ENABLE_STATUS, &dummy); > > + /* > + * To guarantee data written by the current core is visible to > + * all cores, a write barrier is required. This needs to be > + * before an interrupt causes execution on another core. > + * For ARM processors, this needs to be a DSB barrier. > + */ > + wmb(); Based on the patch log and the comment, smp_wmb() seems to be more suitable here since the problem looks like SMP-specific. Most importantly the smp_wmb() will get to be just the compiler barrier on the UP system, so no cache and pipeline flushes in that case. Meanwhile I am not ARM expert, but based on the problem and the DMB/DSB barriers descriptions using DMB should be enough in your case since you only need memory syncs. -Serge(y) > + > /* Clear and enable interrupts */ > regmap_read(dev->map, DW_IC_CLR_INTR, &dummy); > regmap_write(dev->map, DW_IC_INTR_MASK, DW_IC_INTR_MASTER_MASK); > -- > 2.41.0 >
