Em Fri, 13 Apr 2018 11:36:56 +0200
Mason <slash.tmp@xxxxxxx> escreveu:
> On 13/04/2018 11:25, Mauro Carvalho Chehab wrote:
> > Em Fri, 13 Apr 2018 11:15:16 +0200
> > Mason <slash.tmp@xxxxxxx> 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@xxxxxxxxxxxxxxxx>
> >>> ---
> >>> 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
