Re: [PATCH 02/10] mailbox: Enable BCM2835 mailbox support

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.