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

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

On Wed, Apr 28, 2010 at 2:52 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:25:41 +0200
>
>> 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.
>
> I agree that WARN_ON would be enough for this case. I just thought of
> the rareness of this triggering.

Yeah, I was thinking exactly the same, and wanted to put BUG_ON too initially,
but the combination of its calling panic and its header comment
convinced me otherwise:

/*
 * Don't use BUG() or BUG_ON() unless there's really no way out; one
 * example might be detecting data structure corruption in the middle
 * of an operation that can't be backed out of.  If the (sub)system
 * can somehow continue operating, perhaps with reduced functionality,
 * it's probably not BUG-worthy.
 *
 * If you're tempted to BUG(), think again:  is completely giving up
 * really the *only* solution?  There are usually better options, where
 * users don't need to reboot ASAP and can mostly shut down cleanly.
 */




> --
> 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