From: Mauro Carvalho Chehab <mchehab@s-opensource.com> To: Mason <slash.tmp@free.fr> Cc: Mauro Carvalho Chehab <mchehab@infradead.org>, Patrice Chotard <patrice.chotard@st.com>, Linux ARM <linux-arm-kernel@lists.infradead.org>, linux-media <linux-media@vger.kernel.org> Subject: Re: [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever Date: Fri, 13 Apr 2018 06:52:19 -0300 [thread overview] Message-ID: <20180413065219.36e18418@vento.lan> (raw) In-Reply-To: <a83f5846-b93c-b3c5-31fa-a694df54b6b0@free.fr> Em Fri, 13 Apr 2018 11:36:56 +0200 Mason <slash.tmp@free.fr> escreveu: > On 13/04/2018 11:25, Mauro Carvalho Chehab wrote: > > Em Fri, 13 Apr 2018 11:15:16 +0200 > > Mason <slash.tmp@free.fr> escreveu: > > > >> On 12/04/2018 17:24, 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. > >>> > >>> 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); > >>> > >> > >> Isn't this a place where the iopoll.h helpers might be useful? > >> > >> e.g. readl_poll_timeout() > >> > >> https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L114 > > > > That won't work. Internally[1], readx_poll_timeout() calls > > usleep_range(). > > > > [1] https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L43 > > > > It can't be called here, as this loop happens at the irq > > handler. > > Sorry, I meant readl_poll_timeout_atomic() Ah, ok! > But it might have to be open-coded because of the check for overruns. Yeah, readl_poll_timeout_atomic() works fine if we wanted to read just one value, but in this case, we need a loop to read from a FIFO, and we want to ensure that the total time spent there won't be bigger than a reasonable limit. So, we would need to open-code it, with is what the patch I proposed actually did (except that it uses jiffies instead of the high-res clock). If we take Sean's suggestion of limiting the loop by the FIFO size, then readl_poll_timeout_atomic() could indeed be an interesting alternative. Thanks, Mauro
WARNING: multiple messages have this Message-ID (diff)
From: mchehab@s-opensource.com (Mauro Carvalho Chehab) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever Date: Fri, 13 Apr 2018 06:52:19 -0300 [thread overview] Message-ID: <20180413065219.36e18418@vento.lan> (raw) In-Reply-To: <a83f5846-b93c-b3c5-31fa-a694df54b6b0@free.fr> Em Fri, 13 Apr 2018 11:36:56 +0200 Mason <slash.tmp@free.fr> escreveu: > On 13/04/2018 11:25, Mauro Carvalho Chehab wrote: > > Em Fri, 13 Apr 2018 11:15:16 +0200 > > Mason <slash.tmp@free.fr> escreveu: > > > >> On 12/04/2018 17:24, 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. > >>> > >>> 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); > >>> > >> > >> Isn't this a place where the iopoll.h helpers might be useful? > >> > >> e.g. readl_poll_timeout() > >> > >> https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L114 > > > > That won't work. Internally[1], readx_poll_timeout() calls > > usleep_range(). > > > > [1] https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L43 > > > > It can't be called here, as this loop happens at the irq > > handler. > > Sorry, I meant readl_poll_timeout_atomic() Ah, ok! > But it might have to be open-coded because of the check for overruns. Yeah, readl_poll_timeout_atomic() works fine if we wanted to read just one value, but in this case, we need a loop to read from a FIFO, and we want to ensure that the total time spent there won't be bigger than a reasonable limit. So, we would need to open-code it, with is what the patch I proposed actually did (except that it uses jiffies instead of the high-res clock). If we take Sean's suggestion of limiting the loop by the FIFO size, then readl_poll_timeout_atomic() could indeed be an interesting alternative. Thanks, Mauro
next prev parent reply other threads:[~2018-04-13 9:52 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 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 [this message] 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=20180413065219.36e18418@vento.lan \ --to=mchehab@s-opensource.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-media@vger.kernel.org \ --cc=mchehab@infradead.org \ --cc=patrice.chotard@st.com \ --cc=slash.tmp@free.fr \ /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.