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

[Stephen Warren]

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
>>>
>>> 
[2]
http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.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.