From: ext Ohad Ben-Cohen <[email protected]>
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?
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
