Stephen Warren <swarren@xxxxxxxxxxxxx> writes:
> On 03/04/2015 11:20 AM, Eric Anholt wrote:
>> Stephen Warren <swarren@xxxxxxxxxxxxx> writes:
>>
>>> On 03/02/2015 01:54 PM, Eric Anholt wrote:
>>>> From: Lubomir Rintel <lkundrak@xxxxx>
>>>>
>>>> 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.
>>>
>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>> b/drivers/mailbox/bcm2835-mailbox.c
>
>>>> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) +{
>>>> + struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; +
>>>> struct device *dev = mbox->dev; + + while (!(readl(mbox->regs +
>>>> MAIL0_STA) & ARM_MS_EMPTY)) { + u32 msg = readl(mbox->regs +
>>>> MAIL0_RD); + unsigned int chan = MBOX_CHAN(msg); + + if
>>>> (!mbox->channel[chan].started) { + dev_err(dev, "Reply on
>>>> stopped channel %d\n", chan); + continue; + } +
>>>> dev_dbg(dev, "Reply 0x%08X\n", msg); +
>>>> mbox_chan_received_data(mbox->channel[chan].link, + (void *)
>>>> MBOX_DATA28(msg)); + } + rmb(); /* Finished last mailbox read.
>>>> */
>>>
>>> I know the PDF mentioned in the comment earlier in the patch says
>>> to put in barriers between accesses to different peripherals,
>>> which this seems compliant with, but I don't see quite what this
>>> barrier achieves. I think the PDF is talking generalities, not
>>> imposing a rule that must be blindly followed. Besides, if
>>> there's a context-switch you can't actually implement the rules
>>> the PDF suggests. What read operation is this barrier attempting
>>> to ensure happens after reading all mailbox messages and any
>>> associated DRAM buffer?
>>
>> Looking at this bit of code in particular:
>>
>> "As interrupts can appear anywhere in the code so you should
>> safeguard those. If an interrupt routine reads from a peripheral
>> the routine should start with a memory read barrier. If an
>> interrupt routine writes to a peripheral the routine should end
>> with a memory write barrier."
>
> I can see that doing that would be safe, but I don't think following
> those rules is actually necessary in the general case. Following those
> rules would cause lots of unnecessary barriers in code.
>
> Barriers shouldn't be used arbitrarily at the start/end of ISRs, but
> rather at specific chosen locations in the code that actually need to
> enforce some kind of memory ordering.
According to the architecture docs, if you don't RMB at the start of
your ISR, then the following timeline could get the wrong values:
some other device driver our isr
------------------------ -------
rmb()
read from device
read from device
examine value read
exit isr
examine value raed.
The ISR could get the device driver's value. This is made explicit in
footnote 2 on page 7.
>> So it seems like the IRQ handler at least wants:
>>
>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>> b/drivers/mailbox/bcm2835-mailbox.c index 604beb7..7e528f6 100644
>> --- a/drivers/mailbox/bcm2835-mailbox.c +++
>> b/drivers/mailbox/bcm2835-mailbox.c @@ -88,6 +88,13 @@ static
>> irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) struct
>> bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; struct device
>> *dev = mbox->dev;
>>
>> + /* + * BCM2835-ARM-Peripherals.pdf says "If an interrupt
>> routine + * reads from a peripheral the routine should start with
>> a + * memory read barrier." + */ + rmb(); + while
>> (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) { u32 msg =
>> readl(mbox->regs + MAIL0_RD); unsigned int chan = MBOX_CHAN(msg);
>> @@ -100,7 +107,6 @@ static irqreturn_t bcm2835_mbox_irq(int irq,
>> void *dev_id) mbox_chan_received_data(mbox->channel[chan].link,
>> (void *) MBOX_DATA28(msg)); } - rmb(); /* Finished last mailbox
>> read. */ return IRQ_HANDLED; }
>
> In this case, I don't think neither the original barrier is needed,
> nor the extra one you added.
Your formatting got completely destroyed here.
Attachment:
signature.asc
Description: PGP signature
