Re: [PATCH 4/4] omap: mailbox: convert block api to kfifo

[Ohad Ben-Cohen <ohad@wizery.com>]

On Wed, Apr 28, 2010 at 2:16 PM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> From: ext Ohad Ben-Cohen <ohad@wizery.com>
> 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.

Thanks,
Ohad.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html