On 2/12/20 8:18 PM, Samuel Holland wrote:

Jassi,

On 2/12/20 8:02 PM, Jassi Brar wrote:

quoted

On Sun, Jan 12, 2020 at 11:18 PM Samuel Holland [off-list ref] wrote:

quoted

+static int sun6i_msgbox_send_data(struct mbox_chan *chan, void *data)
+{
+       struct sun6i_msgbox *mbox = to_sun6i_msgbox(chan);
+       int n = channel_number(chan);
+       uint32_t msg = *(uint32_t *)data;
+
+       /* Using a channel backwards gets the hardware into a bad state. */
+       if (WARN_ON_ONCE(!(readl(mbox->regs + CTRL_REG(n)) & CTRL_TX(n))))
+               return 0;
+
+       /* We cannot post a new message if the FIFO is full. */
+       if (readl(mbox->regs + FIFO_STAT_REG(n)) & FIFO_STAT_MASK) {
+               mbox_dbg(mbox, "Channel %d busy sending 0x%08x\n", n, msg);
+               return -EBUSY;
+       }
+

This check should go into sun6i_msgbox_last_tx_done().
send_data() assumes all is clear to send next packet.

sun6i_msgbox_last_tx_done() already checks that the FIFO is completely empty (as
the big comment explains). So this error could only be hit in the knows_txdone
== true case, if the client pipelines multiple messages by calling
mbox_client_txdone() before the message is actually removed from the FIFO.

From the comments in mailbox_controller.h, this kind of usage looks to be
unsupported. In that case, I could remove the check entirely. Does that sound right?

After more thought, I would prefer to keep the check. It is fast/simple, and it
keeps the hardware from getting into an inconsistent state. Silently dropping
messages sounds like a poor quality of implementation.

send_data() is documented in mailbox_controller.h as returning EBUSY, and I see
multiple other mailbox controllers implementing the same or a similar check. If
that is not the way you intend for the API to work, then please update the
comments in mailbox_controller.h.

Thanks,
Samuel

quoted

.....

quoted

+
+       mbox->controller.dev           = dev;
+       mbox->controller.ops           = &sun6i_msgbox_chan_ops;
+       mbox->controller.chans         = chans;
+       mbox->controller.num_chans     = NUM_CHANS;
+       mbox->controller.txdone_irq    = false;
+       mbox->controller.txdone_poll   = true;
+       mbox->controller.txpoll_period = 5;
+

nit:  just a single space should do too.

Sorry, for some reason I thought I had replied to this patch, but
apparently not. My mistake. Do you want to revise this submission or
send another patch on top?

For just this change, it would be simpler to send a follow-up patch.

quoted

thanks

Thank you,
Samuel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel