Re: [PATCH v2] i2c: designware: Fix corrupted memory seen in the ISR

[Catalin Marinas <catalin.marinas@arm.com>]

On Mon, Sep 18, 2023 at 08:45:42PM -0700, Jan Bottorff wrote:
> > > > > > > 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();
[...]
> I did find the below text in the Arm Architectural Reference Manual (DDI
> 0487I.a) section K13.4 "Using a mailbox to send an interrupt". It was nearly
> the same wording as the ARM barrier document I previously referenced at
> https://developer.arm.com/documentation/genc007826/latest/ This too says a
> DSB barrier is required for memory updates to be observable in the ISR.
> 
> "
> K13.4 Using a mailbox to send an interrupt
>   In some message passing systems, it is common for one observer to update
> memory and then notify a second observer of the update by sending an
> interrupt, using a mailbox. Although a memory access might be made to
> initiate the sending of the mailbox interrupt, a DSB instruction is
> required to ensure the completion of previous memory accesses.
> 
> Therefore, the following sequence is required to ensure that P2 observes the
> updated value:
> 
> AArch32
> P1
>   STR R5, [R1] ; message stored to shared memory location
>   DSB ST
>   STR R0, [R4] ; R4 contains the address of a mailbox
> P2
>   ; interrupt service routine
>   LDR R5, [R1]
> 
> AArch64
> P1
>   STR W5, [X1] ; message stored to shared memory location
>   DSB ST
>   STR W0, [X4] ; R4 contains the address of a mailbox
> P2
>   ; interrupt service routine
>   LDR W5, [X1]
> "

Will convinced me in the past that a DMB is sufficient here unless the
peripheral is CPU-local. The Arm ARM is not entirely clear here.

> I hear your concern about how this barrier in platform portable code may
> impact platforms other than the one I'm trying to fix. It almost seems like
> there is some missing type of barrier macro that on ARM64 does what is
> required for cases like this and on other platforms does whatever is
> appropriate for that platform, often nothing.

I also agree that a wmb() in the i2c driver is not the more elegant fix.
For similar reasons, we hid barriers in the write*() macros, drivers
need to stay architecture-agnostic as much as possible.

Where does the regmap_write() end up? I think the barrier should be
somewhere down this path.

-- 
Catalin