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:46:44 -0600 [thread overview]
Message-ID: <550BA634.9060005@wwwdotorg.org> (raw)
In-Reply-To: <20150319075836.GU3318@x1>
On 03/19/2015 01:58 AM, Lee Jones wrote:
> On Wed, 18 Mar 2015, 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.
>
> That's a fantastic explanation. Thanks for taking the time to
> write this out so diligently.
>
> Doing this at a sub-arch level sounds a little wrong to me. I don't
> think Broadcom are the only vendor who do not ensure correct read
> order,
It seems like quite a surprising bug to me, and AFAIK there's nothing
already upstream that requires a similar WAR.
> and writing <vendor>_peripheral_read_workarounds() all over the
> place sounds less than graceful.
It sounds like Eric's latest plan is to put the one WAR into the
bcm2835-specific IRQ controller driver. Assuming we don't find any
unusual code elsewhere, we shouldn't need to put WARs all over the place.
> Granted, if there were a greater
> need and this can't be fixed another way we could knock off the
> <vendor>_ part and make the call generic, but is there no way we can
> deal with this at the architecture level?
>
> The whole point of doing readl_relaxed()s is that you can be assured
> that the architecture guarantee ordering. If that's not the case,
> then we need to be using readl()s in the IRQ handler instead.
Doing a single dsb at the start of the IRQ handler should be enough,
assuming that the code consumes each read result in a control path
before issuing another readl_relaxed, there will never be multiple reads
outstanding.
next prev parent reply other threads:[~2015-03-20 4:46 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
[not found] ` <20150319075836.GU3318@x1>
2015-03-20 4:46 ` Stephen Warren [this message]
[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=550BA634.9060005@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