From: swarren@wwwdotorg•org (Stephen Warren)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support
Date: Thu, 19 Mar 2015 22:44:04 -0600 [thread overview]
Message-ID: <550BA594.9060305@wwwdotorg.org> (raw)
In-Reply-To: <87bnjqorpe.fsf@eliezer.anholt.net>
On 03/18/2015 04:39 PM, Eric Anholt wrote:
> Lee Jones <lee@kernel•org> writes:
>
>> On Thu, 12 Mar 2015, Eric Anholt wrote:
>>
>>> From: Lubomir Rintel <lkundrak@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@v3•sk> Signed-off-by:
>>> Craig McGeachie <slapdau@yahoo•com.au> Signed-off-by: Suman
>>> Anna <s-anna@ti•com> Signed-off-by: Jassi Brar
>>> <jassisinghbrar@gmail•com> Signed-off-by: Eric Anholt
>>> <eric@anholt•net> Cc: Jassi Brar <jassisinghbrar@gmail•com>
>>> Acked-by: Lee Jones <lee.jones@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.
next prev parent reply other threads:[~2015-03-20 4:44 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1426213936-4139-1-git-send-email-eric@anholt.net>
2015-03-17 3:24 ` [PATCH 1/4] ARM: BCM2835: Add a function for doing an rmb() between device reads Stephen Warren
2015-03-17 19:06 ` Eric Anholt
[not found] ` <1426213936-4139-3-git-send-email-eric@anholt.net>
2015-03-17 3:33 ` [PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support Stephen Warren
2015-03-17 18:05 ` Lee Jones
2015-03-17 19:04 ` Eric Anholt
2015-03-18 23:28 ` Eric Anholt
2015-03-20 4:48 ` Stephen Warren
2015-03-20 5:12 ` Jassi Brar
2015-03-20 17:38 ` Eric Anholt
2015-03-20 17:24 ` Eric Anholt
2015-03-20 19:29 ` Stephen Warren
[not found] ` <20150318084255.GJ3318@x1>
[not found] ` <87bnjqorpe.fsf@eliezer.anholt.net>
2015-03-20 4:44 ` Stephen Warren [this message]
[not found] ` <20150319075836.GU3318@x1>
2015-03-20 4:46 ` Stephen Warren
[not found] ` <1426213936-4139-4-git-send-email-eric@anholt.net>
2015-03-17 3:34 ` [PATCH 4/4] ARM: bcm2835: Add the mailbox to the device tree Stephen Warren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=550BA594.9060305@wwwdotorg.org \
--to=swarren@wwwdotorg$(echo .)org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox