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. Thanks, Ohad. > -- > 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