From: ext Ohad Ben-Cohen <ohad@xxxxxxxxxx>
Subject: Re: [PATCH 4/4] omap: mailbox: convert block api to kfifo
Date: Wed, 28 Apr 2010 13:25:41 +0200
> On Wed, Apr 28, 2010 at 2:16 PM, Hiroshi DOYU <Hiroshi.DOYU@xxxxxxxxx> wrote:
>> From: ext Ohad Ben-Cohen <ohad@xxxxxxxxxx>
>> Subject: Re: [PATCH 4/4] omap: mailbox: convert block api to kfifo
>> Date: Wed, 28 Apr 2010 13:02:06 +0200
>>
>>>>> diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
>>>>> index 72b17ad..b1324f3 100644
>>>>> --- a/arch/arm/plat-omap/mailbox.c
>>>>> +++ b/arch/arm/plat-omap/mailbox.c
>>>>> @@ -26,6 +26,7 @@
>>>>> #include <linux/device.h>
>>>>> #include <linux/delay.h>
>>>>> #include <linux/slab.h>
>>>>> +#include <linux/kfifo.h>
>>>>>
>>>>> #include <plat/mailbox.h>
>>>>>
>>>>> @@ -67,7 +68,7 @@ static inline int is_mbox_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq)
>>>>> /*
>>>>> * message sender
>>>>> */
>>>>> -static int __mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
>>>>> +static int __mbox_poll_for_space(struct omap_mbox *mbox)
>>>>> {
>>>>> int ret = 0, i = 1000;
>>>>>
>>>>> @@ -78,49 +79,54 @@ static int __mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
>>>>> return -1;
>>>>> udelay(1);
>>>>> }
>>>>> - mbox_fifo_write(mbox, msg);
>>>>> return ret;
>>>>> }
>>>>>
>>>>> -
>>>>> int omap_mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
>>>>> {
>>>>> + struct omap_mbox_queue *mq = mbox->txq;
>>>>> + int ret = 0, len;
>>>>>
>>>>> - struct request *rq;
>>>>> - struct request_queue *q = mbox->txq->queue;
>>>>> + spin_lock(&mq->lock);
>>>>>
>>>>> - rq = blk_get_request(q, WRITE, GFP_ATOMIC);
>>>>> - if (unlikely(!rq))
>>>>> - return -ENOMEM;
>>>>> + if (kfifo_avail(&mq->fifo) < sizeof(msg)) {
>>>>> + ret = -ENOMEM;
>>>>> + goto out;
>>>>> + }
>>>>> +
>>>>> + len = kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
>>>>> + if (unlikely(len != sizeof(msg))) {
>>>>> + pr_err("%s: kfifo_in anomaly\n", __func__);
>>>>> + ret = -ENOMEM;
>>>>> + }
>>>>>
>>>>> - blk_insert_request(q, rq, 0, (void *) msg);
>>>>> tasklet_schedule(&mbox->txq->tasklet);
>>>>>
>>>>> - return 0;
>>>>> +out:
>>>>> + spin_unlock(&mq->lock);
>>>>> + return ret;
>>>>> }
>>>>> EXPORT_SYMBOL(omap_mbox_msg_send);
>>>>>
>>>>> static void mbox_tx_tasklet(unsigned long tx_data)
>>>>> {
>>>>> - int ret;
>>>>> - struct request *rq;
>>>>> struct omap_mbox *mbox = (struct omap_mbox *)tx_data;
>>>>> - struct request_queue *q = mbox->txq->queue;
>>>>> -
>>>>> - while (1) {
>>>>> -
>>>>> - rq = blk_fetch_request(q);
>>>>> -
>>>>> - if (!rq)
>>>>> - break;
>>>>> + struct omap_mbox_queue *mq = mbox->txq;
>>>>> + mbox_msg_t msg;
>>>>> + int ret;
>>>>>
>>>>> - ret = __mbox_msg_send(mbox, (mbox_msg_t)rq->special);
>>>>> - if (ret) {
>>>>> + while (kfifo_len(&mq->fifo)) {
>>>>> + if (__mbox_poll_for_space(mbox)) {
>>>>> omap_mbox_enable_irq(mbox, IRQ_TX);
>>>>> - blk_requeue_request(q, rq);
>>>>> - return;
>>>>> + break;
>>>>> }
>>>>> - blk_end_request_all(rq, 0);
>>>>> +
>>>>> + ret = kfifo_out(&mq->fifo, (unsigned char *)&msg,
>>>>> + sizeof(msg));
>>>>> + if (unlikely(ret != sizeof(msg)))
>>>>> + pr_err("%s: kfifo_out anomaly\n", __func__);
>>>>
>>>> No error recovery? same for other anomalies.
>>>
>>> I thought about it too, but eventually I think it doesn't really make
>>> a lot of sense:
>>> The only reason that kfifo_out/kfifo_in can fail here is if there's
>>> not enough data/space (respectively).
>>> Since we are checking for the availability of data/space before calling
>>> kfifo_out/kfifo_in, it cannot really fail.
>>> If it does fail, that's a critical bug that we cannot really recover from.
>>> Only reasonable explanation can be a bug in the code (either ours or kfifo's),
>>> and with such a bug it really feels futile to put some recovery.
>>> So maybe we should really make this a WARN_ON.
>>> What do you think ?
>>
>> Makes sense. What about BUG_ON if it shouldn't happen theoretically?
>
> I'm not sure this bug is critical enough to panic and enforce the user
> to reboot immediately - he can probably still do a clean shutdown here.
I agree that WARN_ON would be enough for this case. I just thought of
the rareness of this triggering.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
