On Wed, Apr 28, 2010 at 2:16 PM, Hiroshi DOYU <hiroshi.d...@nokia.com> wrote:
> From: ext Ohad Ben-Cohen <o...@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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
