| Message ID | 20230913010313.418159-1-janb@os.amperecomputing.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: designware: Fix corrupted memory seen in the ISR | expand |
On Wed, Sep 13, 2023 at 11:04:00AM +0200, Yann Sionneau wrote: > On 13/09/2023 03:03, Jan Bottorff wrote: ... > > + /* > > + * 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(); > > Apart from the commit message it looks good to me. > > If I understand correctly without this wmb() it is possible that the writes > to dev->msg_write_idx , dev->msg_read_idx = 0 etc would not yet be visible > to another CPU running the ISR handler right after enabling those. If this is the case, shouldn't we rather use READ_ONCE()/WRITE_ONCE() where appropriate?
On 13/09/2023 13:32, Yann Sionneau wrote: > > On 13/09/2023 13:22, Andy Shevchenko wrote: >> On Wed, Sep 13, 2023 at 11:04:00AM +0200, Yann Sionneau wrote: >>> On 13/09/2023 03:03, Jan Bottorff wrote: >> ... >> >>>> + /* >>>> + * 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(); >>> Apart from the commit message it looks good to me. >>> >>> If I understand correctly without this wmb() it is possible that the >>> writes >>> to dev->msg_write_idx , dev->msg_read_idx = 0 etc would not yet be >>> visible >>> to another CPU running the ISR handler right after enabling those. >> If this is the case, shouldn't we rather use READ_ONCE()/WRITE_ONCE() >> where >> appropriate? >> > To my knowledge the READ_ONCE()/WRITE_ONCE() only imply the use of > volatile to access memory thus preventing the compiler to do weird > optimizations like merging store/loads, moving store/loads, removing > them etc > > They don't imply a memory barrier. > > Some systems need a memory barrier, to emit a "fence" like > instruction, so that the pipeline stalls waiting for the store to > "finish", for systems where the writes are posted. > > Regards, > Well, for the READ_ONCE() actually I'm wrong, it's overloaded for Alpha and arm64 https://elixir.bootlin.com/linux/latest/C/ident/__READ_ONCE
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(); + /* 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);
Errors were happening in the ISR that looked like corrupted memory. This was because writes from the core enabling interrupts where not yet visible to the core running the ISR. A write barrier assures writes to driver data structures are visible to all cores before interrupts are enabled. 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@os.amperecomputing.com> --- drivers/i2c/busses/i2c-designware-master.c | 8 ++++++++ 1 file changed, 8 insertions(+)