On Fri, May 17, 2019 at 8:08 PM Alex Elder <el...@linaro.org> wrote: > > On 5/15/19 2:34 AM, Arnd Bergmann wrote: > >> +static void gsi_trans_tre_fill(struct gsi_tre *dest_tre, dma_addr_t addr, > >> + u32 len, bool last_tre, bool bei, > >> + enum ipa_cmd_opcode opcode) > >> +{ > >> + struct gsi_tre tre; > >> + > >> + tre.addr = cpu_to_le64(addr); > >> + tre.len_opcode = gsi_tre_len_opcode(opcode, len); > >> + tre.reserved = 0; > >> + tre.flags = gsi_tre_flags(last_tre, bei, opcode); > >> + > >> + *dest_tre = tre; /* Write TRE as a single (16-byte) unit */ > >> +} > > Have you checked that the atomic write is actually what happens here, > > but looking at the compiler output? You might need to add a 'volatile' > > qualifier to the dest_tre argument so the temporary structure doesn't > > get optimized away here. > > Currently, the assignment *does* become a "stp" instruction. > But I don't know that we can *force* the compiler to write it > as a pair of registers, so I'll soften the comment with > "Attempt to write" or something similar. > > To my knowledge, adding a volatile qualifier only prevents the > compiler from performing funny optimizations, but that has no > effect on whether the 128-bit assignment is made as a single > unit. Do you know otherwise?
I don't think it you can force the 128-bit assignment to be atomic, but marking 'dest_tre' should serve to prevent a specific optimization that replaces the function with dest_tre->addr = ... dest_tre->len_opcode = ... dest_tre->reserved = ... dest_tre->flags = ... which it might find more efficient than the stp and is equivalent when the pointer is not marked volatile. We also have the WRITE_ONCE() macro that can help prevent this, but it does not work reliably beyond 64 bit assignments. Arnd