From: Sean Young <sean@mess.org> To: Mauro Carvalho Chehab <mchehab@s-opensource.com> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>, Mauro Carvalho Chehab <mchehab@infradead.org>, Patrice Chotard <patrice.chotard@st.com>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever Date: Thu, 12 Apr 2018 23:21:32 +0100 [thread overview] Message-ID: <20180412222132.z7g5enhin2uodbk7@gofer.mess.org> (raw) In-Reply-To: <16b1993cde965edc096f0833091002dd05d4da7f.1523546545.git.mchehab@s-opensource.com> On Thu, Apr 12, 2018 at 11:24:07AM -0400, Mauro Carvalho Chehab wrote: > As warned by smatch: > drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding > > If something goes wrong at readl(), the logic will stay there > inside an IRQ code forever. This is not the nicest thing to > do :-) > > So, add a timeout there, preventing staying inside the IRQ > for more than 10ms. If we knew how large the fifo was, then we could limit the loop to that many iterations (maybe a few extra in case IR arrives while we a reading, but IR is much slower than a cpu executing this loop of course). Patrice is something you could help with? Thanks Sean > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > --- > drivers/media/rc/st_rc.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c > index d2efd7b2c3bc..c855b177103c 100644 > --- a/drivers/media/rc/st_rc.c > +++ b/drivers/media/rc/st_rc.c > @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev) > > static irqreturn_t st_rc_rx_interrupt(int irq, void *data) > { > + unsigned long timeout; > unsigned int symbol, mark = 0; > struct st_rc_device *dev = data; > int last_symbol = 0; > - u32 status; > + u32 status, int_status; > DEFINE_IR_RAW_EVENT(ev); > > if (dev->irq_wake) > pm_wakeup_event(dev->dev, 0); > > - status = readl(dev->rx_base + IRB_RX_STATUS); > + /* FIXME: is 10ms good enough ? */ > + timeout = jiffies + msecs_to_jiffies(10); > + do { > + status = readl(dev->rx_base + IRB_RX_STATUS); > + if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW))) > + break; > > - while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) { > - u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS); > + int_status = readl(dev->rx_base + IRB_RX_INT_STATUS); > if (unlikely(int_status & IRB_RX_OVERRUN_INT)) { > /* discard the entire collection in case of errors! */ > ir_raw_event_reset(dev->rdev); > @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data) > > } > last_symbol = 0; > - status = readl(dev->rx_base + IRB_RX_STATUS); > - } > + } while (time_is_after_jiffies(timeout)); > > writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR); > > -- > 2.14.3
WARNING: multiple messages have this Message-ID (diff)
From: sean@mess.org (Sean Young) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever Date: Thu, 12 Apr 2018 23:21:32 +0100 [thread overview] Message-ID: <20180412222132.z7g5enhin2uodbk7@gofer.mess.org> (raw) In-Reply-To: <16b1993cde965edc096f0833091002dd05d4da7f.1523546545.git.mchehab@s-opensource.com> On Thu, Apr 12, 2018 at 11:24:07AM -0400, Mauro Carvalho Chehab wrote: > As warned by smatch: > drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding > > If something goes wrong at readl(), the logic will stay there > inside an IRQ code forever. This is not the nicest thing to > do :-) > > So, add a timeout there, preventing staying inside the IRQ > for more than 10ms. If we knew how large the fifo was, then we could limit the loop to that many iterations (maybe a few extra in case IR arrives while we a reading, but IR is much slower than a cpu executing this loop of course). Patrice is something you could help with? Thanks Sean > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > --- > drivers/media/rc/st_rc.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c > index d2efd7b2c3bc..c855b177103c 100644 > --- a/drivers/media/rc/st_rc.c > +++ b/drivers/media/rc/st_rc.c > @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev) > > static irqreturn_t st_rc_rx_interrupt(int irq, void *data) > { > + unsigned long timeout; > unsigned int symbol, mark = 0; > struct st_rc_device *dev = data; > int last_symbol = 0; > - u32 status; > + u32 status, int_status; > DEFINE_IR_RAW_EVENT(ev); > > if (dev->irq_wake) > pm_wakeup_event(dev->dev, 0); > > - status = readl(dev->rx_base + IRB_RX_STATUS); > + /* FIXME: is 10ms good enough ? */ > + timeout = jiffies + msecs_to_jiffies(10); > + do { > + status = readl(dev->rx_base + IRB_RX_STATUS); > + if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW))) > + break; > > - while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) { > - u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS); > + int_status = readl(dev->rx_base + IRB_RX_INT_STATUS); > if (unlikely(int_status & IRB_RX_OVERRUN_INT)) { > /* discard the entire collection in case of errors! */ > ir_raw_event_reset(dev->rdev); > @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data) > > } > last_symbol = 0; > - status = readl(dev->rx_base + IRB_RX_STATUS); > - } > + } while (time_is_after_jiffies(timeout)); > > writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR); > > -- > 2.14.3
next prev parent reply other threads:[~2018-04-12 22:21 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-04-12 15:23 [PATCH 01/17] media: staging: atomisp: fix number conversion Mauro Carvalho Chehab 2018-04-12 15:23 ` [PATCH 02/17] media: staging: atomisp: don't declare the same vars as both private and public Mauro Carvalho Chehab 2018-04-13 21:27 ` kbuild test robot 2018-04-12 15:23 ` [PATCH 03/17] media: atomisp: fix __user annotations Mauro Carvalho Chehab 2018-04-13 22:35 ` kbuild test robot 2018-04-16 10:22 ` [PATCHv2 02/17] media: staging: atomisp: don't declare the same vars as both private and public Mauro Carvalho Chehab 2018-04-12 15:23 ` [PATCH 04/17] media: staging: atomisp: fix string comparation logic Mauro Carvalho Chehab 2018-04-12 15:23 ` [PATCH 05/17] dvb_frontend: fix locking issues at dvb_frontend_get_event() Mauro Carvalho Chehab 2018-04-12 15:23 ` Mauro Carvalho Chehab 2018-04-12 15:23 ` [PATCH 06/17] media: v4l2-fwnode: simplify v4l2_fwnode_reference_parse_int_props() Mauro Carvalho Chehab 2018-04-12 15:23 ` [PATCH 07/17] media: cec: fix smatch error Mauro Carvalho Chehab 2018-04-12 15:24 ` [PATCH 08/17] atomisp: remove an impossible condition Mauro Carvalho Chehab 2018-04-12 15:24 ` [PATCH 09/17] media: platform: fix some 64-bits warnings Mauro Carvalho Chehab 2018-04-12 15:24 ` [PATCH 10/17] media: v4l2-compat-ioctl32: prevent go past max size Mauro Carvalho Chehab 2018-04-12 15:24 ` Mauro Carvalho Chehab 2018-04-12 15:24 ` [PATCH 11/17] media: atomisp: compat32: use get_user() before referencing user data Mauro Carvalho Chehab 2018-04-12 15:24 ` [PATCH 12/17] media: staging: atomisp: add missing include Mauro Carvalho Chehab 2018-04-12 15:24 ` [PATCH 13/17] media: atomisp: compat32: fix __user annotations Mauro Carvalho Chehab 2018-04-12 15:24 ` [PATCH 14/17] media: atomisp: get rid of a warning Mauro Carvalho Chehab 2018-04-12 15:24 ` [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever Mauro Carvalho Chehab 2018-04-12 15:24 ` Mauro Carvalho Chehab 2018-04-12 22:21 ` Sean Young [this message] 2018-04-12 22:21 ` Sean Young 2018-04-13 7:32 ` Patrice CHOTARD 2018-04-13 7:32 ` Patrice CHOTARD 2018-04-13 9:06 ` Mauro Carvalho Chehab 2018-04-13 9:06 ` Mauro Carvalho Chehab 2018-04-13 9:40 ` Sean Young 2018-04-13 9:40 ` Sean Young 2018-04-13 10:00 ` Mauro Carvalho Chehab 2018-04-13 10:00 ` Mauro Carvalho Chehab 2018-04-13 13:20 ` Sean Young 2018-04-13 13:20 ` Sean Young 2018-04-13 14:08 ` Mauro Carvalho Chehab 2018-04-13 14:08 ` Mauro Carvalho Chehab 2018-04-13 8:03 ` Patrice CHOTARD 2018-04-13 8:03 ` Patrice CHOTARD 2018-04-13 9:15 ` Mason 2018-04-13 9:15 ` Mason 2018-04-13 9:25 ` Mauro Carvalho Chehab 2018-04-13 9:25 ` Mauro Carvalho Chehab 2018-04-13 9:36 ` Mason 2018-04-13 9:36 ` Mason 2018-04-13 9:52 ` Mauro Carvalho Chehab 2018-04-13 9:52 ` Mauro Carvalho Chehab 2018-04-12 15:24 ` [PATCH 16/17] media: mantis: prevent staying forever in a loop at IRQ Mauro Carvalho Chehab 2018-04-12 15:24 ` [PATCH 17/17] media: v4l2-compat-ioctl32: fix several __user annotations Mauro Carvalho Chehab 2018-04-13 18:07 ` [PATCHv2 " Mauro Carvalho Chehab 2018-04-16 12:03 ` Hans Verkuil 2018-04-16 14:50 ` Mauro Carvalho Chehab 2018-04-16 15:32 ` Hans Verkuil 2018-04-16 17:26 ` Mauro Carvalho Chehab
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180412222132.z7g5enhin2uodbk7@gofer.mess.org \ --to=sean@mess.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-media@vger.kernel.org \ --cc=mchehab@infradead.org \ --cc=mchehab@s-opensource.com \ --cc=patrice.chotard@st.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.