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

Reply via email to