On 03/18/2015 04:39 PM, Eric Anholt wrote: > Lee Jones <lee@xxxxxxxxxx> writes: > >> On Thu, 12 Mar 2015, Eric Anholt wrote: >> >>> From: Lubomir Rintel <lkundrak@xxxxx> >>> >>> Implement BCM2835 mailbox support as a device registered with >>> the general purpose mailbox framework. Implementation based on >>> commits by Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on >>> which to base the implementation. >>> >>> [1] >>> http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html >>> >>> [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html >>> >>> Signed-off-by: Lubomir Rintel <lkundrak@xxxxx> Signed-off-by: >>> Craig McGeachie <slapdau@xxxxxxxxxxxx> Signed-off-by: Suman >>> Anna <s-anna@xxxxxx> Signed-off-by: Jassi Brar >>> <jassisinghbrar@xxxxxxxxx> Signed-off-by: Eric Anholt >>> <eric@xxxxxxxxxx> Cc: Jassi Brar <jassisinghbrar@xxxxxxxxx> >>> Acked-by: Lee Jones <lee.jones@xxxxxxxxxx> --- >>> >>> >>> v2: Squashed Craig's work for review, carried over to new >>> version of Mailbox framework (changes by Lubomir) >>> >>> v3: Fix multi-line comment style. Refer to the documentation >>> by filename. Only declare one MODULE_AUTHOR. Alphabetize >>> includes. Drop some excessive dev_dbg()s (changes by anholt). >>> >>> v4: Use the new bcm2835_peripheral_read_workaround(), drop the >> >> Can you explain to me why this is required (and don't just point >> me in the direction of the other patch ;) ). You appear to be >> using the non-relaxed variants of readl and writel, which already >> do memory barriers, so I'm a little perplexed as to how the >> problem can arise. > > Hmm. > > A shorter restatement of the architecture requirement would be, I > think, "Don't let there be two outstanding reads of different > peripherals on the AXI bus, or the CPU might mis-assign the read > results. Use rmb() to wait for the previous bus reads when you > need to prevent this" > > arch/arm/include/asm/io.h's readl() does __iormb() after each > __raw_readl(). Imagine taking an interrupt for a new peripheral > between the driver's __raw_readl() and the following __iormb(): Now > you've got two __raw_readl()s in between iormb()s and you can > theoretically get unordered reads. > > We could hope that the architecture IRQ handler would happen to do > an incidental rmb(), resolving the need to protect from interrupt > handling inside of device drivers. The interrupt controller's > presence at 0x7e00b200 sounds like it's an AXI peripheral, so it > would need to be ensuring ordering of reads. However, it's doing > readl_relaxed()s. So my rmb() at the start of my irq handler is > silly -- if somebody got interrupted between readl and rmb, we've > already had a chance to get the wrong result inside of the IRQ > chip's status read. > > My new idea for handling this would be to: > > 1) Assume drivers don't exit with reads outstanding. This means > they don't do a readl_relaxed() from an AXI peripheral at the end > of a path without doing something with the result. > > 2) Make bcm2835_handle_irq() do this rmb() at the top, with the > big explanation, to avoid a race against the interrupted code > device being inside a readl() before the __iormb(). We don't worry > about the 1-2 readl_relaxed()s inside of bcm2835_handle_irq(), > because their return values get waited on before continuing on to > calling the device driver, so the device driver knows its IRQ > handler is being entered with no AXI reads outstanding. I /think/ that sounds reasonable. I suppose if we do find any code that initiates a read but doesn't use the result before returning or calling into some other code, we can always patch that up with an extra barrier at that time. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
