From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ohad Ben-Cohen Subject: Re: [PATCH 4/4] omap: mailbox: convert block api to kfifo Date: Wed, 28 Apr 2010 14:25:41 +0300 Message-ID: References: <1272390982-14882-5-git-send-email-ohad@wizery.com> <20100428.085254.112592556.Hiroshi.DOYU@nokia.com> <20100428.141620.59684829.Hiroshi.DOYU@nokia.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-yw0-f194.google.com ([209.85.211.194]:47777 "EHLO mail-yw0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750840Ab0D1L0E convert rfc822-to-8bit (ORCPT ); Wed, 28 Apr 2010 07:26:04 -0400 Received: by ywh32 with SMTP id 32so8356645ywh.33 for ; Wed, 28 Apr 2010 04:26:02 -0700 (PDT) In-Reply-To: <20100428.141620.59684829.Hiroshi.DOYU@nokia.com> Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: Hiroshi DOYU Cc: linux-omap@vger.kernel.org, h-kanigeri2@ti.com On Wed, Apr 28, 2010 at 2:16 PM, Hiroshi DOYU = wrote: > From: ext Ohad Ben-Cohen > 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/mai= lbox.c >>>> index 72b17ad..b1324f3 100644 >>>> --- a/arch/arm/plat-omap/mailbox.c >>>> +++ b/arch/arm/plat-omap/mailbox.c >>>> @@ -26,6 +26,7 @@ >>>> =A0#include >>>> =A0#include >>>> =A0#include >>>> +#include >>>> >>>> =A0#include >>>> >>>> @@ -67,7 +68,7 @@ static inline int is_mbox_irq(struct omap_mbox *= mbox, omap_mbox_irq_t irq) >>>> =A0/* >>>> =A0 * message sender >>>> =A0 */ >>>> -static int __mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg= ) >>>> +static int __mbox_poll_for_space(struct omap_mbox *mbox) >>>> =A0{ >>>> =A0 =A0 =A0 int ret =3D 0, i =3D 1000; >>>> >>>> @@ -78,49 +79,54 @@ static int __mbox_msg_send(struct omap_mbox *m= box, mbox_msg_t msg) >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -1; >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(1); >>>> =A0 =A0 =A0 } >>>> - =A0 =A0 mbox_fifo_write(mbox, msg); >>>> =A0 =A0 =A0 return ret; >>>> =A0} >>>> >>>> - >>>> =A0int omap_mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg) >>>> =A0{ >>>> + =A0 =A0 struct omap_mbox_queue *mq =3D mbox->txq; >>>> + =A0 =A0 int ret =3D 0, len; >>>> >>>> - =A0 =A0 struct request *rq; >>>> - =A0 =A0 struct request_queue *q =3D mbox->txq->queue; >>>> + =A0 =A0 spin_lock(&mq->lock); >>>> >>>> - =A0 =A0 rq =3D blk_get_request(q, WRITE, GFP_ATOMIC); >>>> - =A0 =A0 if (unlikely(!rq)) >>>> - =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM; >>>> + =A0 =A0 if (kfifo_avail(&mq->fifo) < sizeof(msg)) { >>>> + =A0 =A0 =A0 =A0 =A0 =A0 ret =3D -ENOMEM; >>>> + =A0 =A0 =A0 =A0 =A0 =A0 goto out; >>>> + =A0 =A0 } >>>> + >>>> + =A0 =A0 len =3D kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeo= f(msg)); >>>> + =A0 =A0 if (unlikely(len !=3D sizeof(msg))) { >>>> + =A0 =A0 =A0 =A0 =A0 =A0 pr_err("%s: kfifo_in anomaly\n", __func_= _); >>>> + =A0 =A0 =A0 =A0 =A0 =A0 ret =3D -ENOMEM; >>>> + =A0 =A0 } >>>> >>>> - =A0 =A0 blk_insert_request(q, rq, 0, (void *) msg); >>>> =A0 =A0 =A0 tasklet_schedule(&mbox->txq->tasklet); >>>> >>>> - =A0 =A0 return 0; >>>> +out: >>>> + =A0 =A0 spin_unlock(&mq->lock); >>>> + =A0 =A0 return ret; >>>> =A0} >>>> =A0EXPORT_SYMBOL(omap_mbox_msg_send); >>>> >>>> =A0static void mbox_tx_tasklet(unsigned long tx_data) >>>> =A0{ >>>> - =A0 =A0 int ret; >>>> - =A0 =A0 struct request *rq; >>>> =A0 =A0 =A0 struct omap_mbox *mbox =3D (struct omap_mbox *)tx_data= ; >>>> - =A0 =A0 struct request_queue *q =3D mbox->txq->queue; >>>> - >>>> - =A0 =A0 while (1) { >>>> - >>>> - =A0 =A0 =A0 =A0 =A0 =A0 rq =3D blk_fetch_request(q); >>>> - >>>> - =A0 =A0 =A0 =A0 =A0 =A0 if (!rq) >>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 break; >>>> + =A0 =A0 struct omap_mbox_queue *mq =3D mbox->txq; >>>> + =A0 =A0 mbox_msg_t msg; >>>> + =A0 =A0 int ret; >>>> >>>> - =A0 =A0 =A0 =A0 =A0 =A0 ret =3D __mbox_msg_send(mbox, (mbox_msg_= t)rq->special); >>>> - =A0 =A0 =A0 =A0 =A0 =A0 if (ret) { >>>> + =A0 =A0 while (kfifo_len(&mq->fifo)) { >>>> + =A0 =A0 =A0 =A0 =A0 =A0 if (__mbox_poll_for_space(mbox)) { >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 omap_mbox_enable_irq(m= box, IRQ_TX); >>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 blk_requeue_request(q, r= q); >>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return; >>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 break; >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 } >>>> - =A0 =A0 =A0 =A0 =A0 =A0 blk_end_request_all(rq, 0); >>>> + >>>> + =A0 =A0 =A0 =A0 =A0 =A0 ret =3D kfifo_out(&mq->fifo, (unsigned c= har *)&msg, >>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 sizeof(msg)); >>>> + =A0 =A0 =A0 =A0 =A0 =A0 if (unlikely(ret !=3D sizeof(msg))) >>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pr_err("%s: kfifo_out an= omaly\n", __func__); >>> >>> No error recovery? same for other anomalies. >> >> I thought about it too, but eventually I think it doesn't really mak= e >> 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 call= ing >> 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@vger.kernel.org > More majordomo info at =A0http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" i= n the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html