Stephen Warren writes: > On 03/02/2015 01:54 PM, 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. > >> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c > >> +/* Mailboxes */ >> +#define ARM_0_MAIL0 0x00 >> +#define ARM_0_MAIL1 0x20 >> + >> +/* >> + * Mailbox registers. We basically only support mailbox 0 & 1. We >> + * deliver to the VC in mailbox 1, it delivers to us in mailbox 0. See >> + * BCM2835-ARM-Peripherals.pdf section 1.3 for an explanation about >> + * the placement of memory barriers. >> + */ >> +#define MAIL0_RD (ARM_0_MAIL0 + 0x00) >> +#define MAIL0_POL (ARM_0_MAIL0 + 0x10) >> +#define MAIL0_STA (ARM_0_MAIL0 + 0x18) >> +#define MAIL0_CNF (ARM_0_MAIL0 + 0x1C) >> +#define MAIL1_WRT (ARM_0_MAIL1 + 0x00) > > That implies there are more mailboxes. I wonder if we should > parameterize which to use via some DT properties? I guess we can defer > that though; we can default to the current values and add properties > later if we want to use something else. Yeah, until there's something to talk to in another mailbox, this seems fine. >> +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." 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; } -- > If any barrier is needed, shouldn't it be between the mailbox read and > the processing of that message (which could at least in some cases read > an SDRAM buffer). So, the producer would do roughly: > > p1) Fill in DRAM buffer > p2) Write memory barrier so the MBOX write happens after the above > p3) Send mbox message to tell the consumer to process the buffer > > ... and the consumer: > > c1) Read MBOX register to know which DRAM buffer to handle > c2) rmb() to make sure we read from the DRAM buffer after the MBOX read > c3) Read the DRAM buffer > > Even then, since (c3) is data-dependent on (c1), I don't think the rmb() > in (c2) there actually does anything useful. I'm not sure if this is already covered by "The GPU has special logic to cope with data arriving out-of-order", but I think I agree we should probably do it for safety. >> +static int bcm2835_send_data(struct mbox_chan *link, void *data) >> +{ >> + struct bcm2835_channel *chan = to_channel(link); >> + struct bcm2835_mbox *mbox = chan->mbox; >> + int ret = 0; >> + >> + if (!chan->started) >> + return -ENODEV; >> + spin_lock(&mbox->lock); > > Is it safe to read chan->started without the channel lock held? The channel's lock is held by our caller (msg_submit() of mailbox.c). >> + if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) { >> + rmb(); /* Finished last mailbox read. */ > > This also doesn't seem useful? This (and the next one) seems to fall under: "You should place: • A memory write barrier before the first write to a peripheral. • A memory read barrier after the last read of a peripheral. It is not required to put a memory barrier instruction after each read or write access. Only at those places in the code where it is possible that a peripheral read or write may be followed by a read or write of a different peripheral. This is normally at the entry and exit points of the peripheral service code." >> +static int bcm2835_startup(struct mbox_chan *link) >> +{ >> + struct bcm2835_channel *chan = to_channel(link); >> + >> + chan->started = true; >> + return 0; >> +} >> + >> +static void bcm2835_shutdown(struct mbox_chan *link) >> +{ >> + struct bcm2835_channel *chan = to_channel(link); >> + >> + chan->started = false; >> +} > > Don't we need to hold chan->lock when adjusting chan->started? Or is > start/stop intended to be asynchronous to any operations currently in > progress on the channel? Only one client can be on a channel at a time, which is controlled by con_mutex at channel request time, and these hooks are when the client appears/disappears. The started check in the irq handler is necessary so that we drop any stray mbox messages instead of NULL dereffing in mbox_chan_received_data(). I think the check in bcm2846_send_data() could just be dropped (we know we have a client if a client is trying to send a message). I haven't followed quite what bcm2835_last_tx_done() is doing. >> +static bool bcm2835_last_tx_done(struct mbox_chan *link) >> +{ >> + struct bcm2835_channel *chan = to_channel(link); >> + struct bcm2835_mbox *mbox = chan->mbox; >> + bool ret; >> + >> + if (!chan->started) >> + return false; >> + spin_lock(&mbox->lock); >> + ret = !(readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL); >> + rmb(); /* Finished last mailbox read. */ > > That barrier doesn't seem useful? Same barrier comment. > What are the semantics of "tx done"? This seems to be testing that the > TX mailbox isn't completely full, which is more about whether the > consumer side is backed up rather than whether our producer-side TX > operations are done. Take a look at poll_txdone() -- it does look like we're doing the right thing here, just that the method would be better named as "ready_to_send" or something. >> +static int request_mailbox_irq(struct bcm2835_mbox *mbox) > >> + if (irq <= 0) { >> + dev_err(dev, "Can't get IRQ number for mailbox\n"); >> + return -ENODEV; >> + } > > I expect devm_request_irq() checkes that condition. Chasing things down, it looks like you'd get a silent error, but then we've already got a whine if devm_request_irq fails anyway. >> + ret = devm_request_irq(dev, irq, bcm2835_mbox_irq, 0, dev_name(dev), >> + mbox); >> + if (ret) { >> + dev_err(dev, "Failed to register a mailbox IRQ handler\n"); > > Printing ret might be useful to know why. Yeah. > Are you sure devm_request_irq() is appropriate? The IRQ handler will be > unregistered *after* bcm2835_mbox_remove() is called, and I think > without any guarantee re: the order that other devm_ allocations are > cleaned up. If bcm2835_mbox_remove() can't guarantee that no IRQ will > fire after it exits, then bcm2835_mbox_irq() might just get called after > some allocations are torn down, thus causing the IRQ handler to touch > free'd memory. It looks like we should writel(0, mbox->regs + MAIL0_CNF); in the unload. > Both request_mailbox_iomem and request_mailbox_irq are small enough > they're typically written inline into the main probe() function. Sounds good. >> +static int bcm2835_mbox_probe(struct platform_device *pdev) > >> + mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL); >> + if (mbox == NULL) { >> + dev_err(dev, "Failed to allocate mailbox memory\n"); > > devm_kzalloc() already prints an error, so no need to add another here, > even if it does nicely document the fact that you remembered error > messages:-) Wait, it does? I couldn't find where it would, when I was looking into the checkpatch warnings. >> + mbox->controller.txdone_poll = true; >> + mbox->controller.txpoll_period = 5; >> + mbox->controller.ops = &bcm2835_mbox_chan_ops; >> + mbox->controller.dev = dev; >> + mbox->controller.num_chans = MBOX_CHAN_COUNT; >> + mbox->controller.chans = devm_kzalloc(dev, >> + sizeof(struct mbox_chan) * MBOX_CHAN_COUNT, >> + GFP_KERNEL); > > It'd be common to say "sizeof(*mbox->controller.chans) so the type can't > mismatch what's being assigned to. Agreed.