On Wed, Apr 28, 2010 at 2:52 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: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.
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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
