[PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

Stephen Warren swarren at wwwdotorg.org
Thu Mar 19 21:44:04 PDT 2015

On 03/18/2015 04:39 PM, Eric Anholt wrote:
> Lee Jones <lee at kernel.org> writes:
>> On Thu, 12 Mar 2015, Eric Anholt wrote:
>>> From: Lubomir Rintel <lkundrak at v3.sk>
>>> 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
>>> Signed-off-by: Lubomir Rintel <lkundrak at v3.sk> Signed-off-by:
>>> Craig McGeachie <slapdau at yahoo.com.au> Signed-off-by: Suman
>>> Anna <s-anna at ti.com> Signed-off-by: Jassi Brar
>>> <jassisinghbrar at gmail.com> Signed-off-by: Eric Anholt
>>> <eric at anholt.net> Cc: Jassi Brar <jassisinghbrar at gmail.com> 
>>> Acked-by: Lee Jones <lee.jones at linaro.org> ---
>>> 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.

More information about the linux-rpi-kernel mailing list