| Message ID | 16b1993cde965edc096f0833091002dd05d4da7f.1523546545.git.mchehab@s-opensource.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
Hi Mauro, Sean On 04/13/2018 12:21 AM, Sean Young wrote: > 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? Unfortunately, i will not be able to give you an answer regarding the Rx fifo size. For information, currently, none of upstreamed ST boards are using this driver. It was used in the past internally and by customers. Regards > > 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
Hi Mauro On 04/12/2018 05:24 PM, 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); > > Acked-by: Patrice Chotard <patrice.chotard@st.com> Thanks Patrice
Hi Sean, Em Thu, 12 Apr 2018 23:21:32 +0100 Sean Young <sean@mess.org> escreveu: > 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). IR is slower, but this code is called at IRQ time, e. g. when the controller already received the IR data. Also, it reads directly via a memory mapped register, with should be fast. I suspect that 10ms is a lot more time than what would be required to go though all the FIFO data. > > 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 Thanks, Mauro
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 Regards.
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. Thanks, Mauro
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() But it might have to be open-coded because of the check for overruns. Regards.
On Fri, Apr 13, 2018 at 06:06:46AM -0300, Mauro Carvalho Chehab wrote: > Hi Sean, > > Em Thu, 12 Apr 2018 23:21:32 +0100 > Sean Young <sean@mess.org> escreveu: > > > 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). > > IR is slower, but this code is called at IRQ time, e. g. when the > controller already received the IR data. Also, it reads directly > via a memory mapped register, with should be fast. There is a chance that a new IR edge occurs whilst reading the fifo. All of this is academic since we don't know the size of the fifo. > I suspect that > 10ms is a lot more time than what would be required to go though > all the FIFO data. 10ms seems far too much. The serial_ir prints a warning if it loops more than 255 times (and breaks out of the loop). Since we don't know the size of the fifo, that will be more than enough. We could also limit it to 512 times, since the raw IR kfifo is 512 and any more than that would not fit in the kfifo anyway. This would exit much quicker than 10ms. At least winbond-cir has a similar loop, there might be other drivers. Thanks Sean
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
Em Fri, 13 Apr 2018 10:40:05 +0100 Sean Young <sean@mess.org> escreveu: > On Fri, Apr 13, 2018 at 06:06:46AM -0300, Mauro Carvalho Chehab wrote: > > Hi Sean, > > > > Em Thu, 12 Apr 2018 23:21:32 +0100 > > Sean Young <sean@mess.org> escreveu: > > > > > 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). > > > > IR is slower, but this code is called at IRQ time, e. g. when the > > controller already received the IR data. Also, it reads directly > > via a memory mapped register, with should be fast. > > There is a chance that a new IR edge occurs whilst reading the fifo. All of > this is academic since we don't know the size of the fifo. > > > I suspect that > > 10ms is a lot more time than what would be required to go though > > all the FIFO data. > > 10ms seems far too much. The serial_ir prints a warning if it loops more > than 255 times (and breaks out of the loop). Since we don't know the size > of the fifo, that will be more than enough. We could also limit it to > 512 times, since the raw IR kfifo is 512 and any more than that would > not fit in the kfifo anyway. This would exit much quicker than 10ms. > > At least winbond-cir has a similar loop, there might be other drivers. Yeah, we could limit it to run only 512 times (or some other reasonable quantity), but in order to do that, we need to be sure that, on each read(), the FIFO will shift - e. g. no risk of needing to do more than one read to get the next element. That would work if the FIFO is implemented via flip-flops. But if it is implemented via some slow memory, or if the shift logic is implemented via some software on a micro-controller, it may need a few interactions to get the next value. Without knowing about the hardware implementation, I'd say that setting a max time for the whole FIFO interaction is safer. Thanks, Mauro
On Fri, Apr 13, 2018 at 07:00:50AM -0300, Mauro Carvalho Chehab wrote: > Yeah, we could limit it to run only 512 times (or some other reasonable > quantity), but in order to do that, we need to be sure that, on each read(), > the FIFO will shift - e. g. no risk of needing to do more than one read > to get the next element. That would work if the FIFO is implemented via > flip-flops. But if it is implemented via some slow memory, or if the > shift logic is implemented via some software on a micro-controller, it > may need a few interactions to get the next value. > > Without knowing about the hardware implementation, I'd say that setting > a max time for the whole FIFO interaction is safer. Ok. If the 10ms timeout is reached, there really is a problem; should we report an error in this case? Thanks Sean
Em Fri, 13 Apr 2018 14:20:52 +0100 Sean Young <sean@mess.org> escreveu: > On Fri, Apr 13, 2018 at 07:00:50AM -0300, Mauro Carvalho Chehab wrote: > > Yeah, we could limit it to run only 512 times (or some other reasonable > > quantity), but in order to do that, we need to be sure that, on each read(), > > the FIFO will shift - e. g. no risk of needing to do more than one read > > to get the next element. That would work if the FIFO is implemented via > > flip-flops. But if it is implemented via some slow memory, or if the > > shift logic is implemented via some software on a micro-controller, it > > may need a few interactions to get the next value. > > > > Without knowing about the hardware implementation, I'd say that setting > > a max time for the whole FIFO interaction is safer. > > Ok. If the 10ms timeout is reached, there really is a problem; should we > report an error in this case? Maybe, but then it should likely warn only once. Thanks, Mauro
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);
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(-)