From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Subject: Re: [PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support Date: Thu, 19 Mar 2015 22:44:04 -0600 Message-ID: <550BA594.9060305@wwwdotorg.org> References: <1426213936-4139-1-git-send-email-eric@anholt.net> <1426213936-4139-3-git-send-email-eric@anholt.net> <20150318084255.GJ3318@x1> <87bnjqorpe.fsf@eliezer.anholt.net> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <87bnjqorpe.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Eric Anholt Cc: Lee Jones , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jassi Brar , Craig McGeachie , Lubomir Rintel , Suman Anna List-Id: devicetree@vger.kernel.org On 03/18/2015 04:39 PM, Eric Anholt wrote: > Lee Jones writes: > >> On Thu, 12 Mar 2015, Eric Anholt wrote: >> >>> From: Lubomir Rintel >>> >>> 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 Signed-off-by: >>> Craig McGeachie Signed-off-by: Suman >>> Anna Signed-off-by: Jassi Brar >>> Signed-off-by: Eric Anholt >>> Cc: Jassi Brar >>> Acked-by: Lee Jones --- >>> >>> >>> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html