Re: [PATCH] serial: max310x: improve interrupt handling

[Tapio Reijonen <tapio.reijonen@vaisala.com>]

Hi,

On 9/4/25 19:36, Hugo Villeneuve wrote:
> Hi,
> 
> On Thu, 4 Sep 2025 14:50:16 +0300
> Tapio Reijonen <tapio.reijonen@vaisala.com> wrote:
> 
>> Hi,
>>
>> On 9/4/25 10:53, Jiri Slaby wrote:
>>> On 03. 09. 25, 11:23, Tapio Reijonen wrote:
>>>> When there is a heavy load of receiving characters to all
>>>> four UART's, the warning 'Hardware RX FIFO overrun' is
>>>> sometimes detected.
>>>> The current implementation always service first UART3 until
>>>> no more interrupt and then service another UARTs.
>>>>
>>>> This commit improve interrupt service routine to handle all
>>>> interrupt sources, e.g. UARTs when a global IRQ is detected.
>>>>
>>>> Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
>>>> ---
>>>>    drivers/tty/serial/max310x.c | 21 ++++++++++++++++-----
>>>>    1 file changed, 16 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
>>>> index
>>>> ce260e9949c3c268e706b2615d6fc01adc21e49b..3234ed7c688ff423d25a007ed8b938b249ae0b82 100644
>>>> --- a/drivers/tty/serial/max310x.c
>>>> +++ b/drivers/tty/serial/max310x.c
>>>> @@ -824,15 +824,26 @@ static irqreturn_t max310x_ist(int irq, void
>>>> *dev_id)
>>>>        if (s->devtype->nr > 1) {
>>>>            do {
>>>> -            unsigned int val = ~0;
>>>> +            unsigned int val;
>>>> +            unsigned int global_irq = ~0;
>>>> +            int port;
>>>>                WARN_ON_ONCE(regmap_read(s->regmap,
>>>> -                         MAX310X_GLOBALIRQ_REG, &val));
>>>> -            val = ((1 << s->devtype->nr) - 1) & ~val;
>>>> +                MAX310X_GLOBALIRQ_REG, &global_irq));
>>>> +
>>>> +            val = ((1 << s->devtype->nr) - 1) & ~global_irq;
>>>
>>> This is horrid. Use GENMASK() (or BIT() below) instead. Likely, you want
>>> a local var storing the mask (the first part before the &).
>>>
>> val = GENMASK(s->devtype->nr - 1, 0) & ~global_irq;>>               if
>> (!val)
>>>>                    break;
>>>> -            if (max310x_port_irq(s, fls(val) - 1) == IRQ_HANDLED)
>>>> -                handled = true;
>>>> +
>>>> +            do {
>>>> +                port = fls(val) - 1;
>>>> +                if (max310x_port_irq(s, port) == IRQ_HANDLED)
>>>> +                    handled = true;
>>>> +
>>>> +                global_irq |= 1 << port;
>>>> +                val = ((1 << s->devtype->nr) - 1) & ~global_irq;
>>>> +            } while (val);
>>>
>>> Actually, does it have to be from the end? I am thinking of
>>> for_each_and_bit()...
>>>
>>>>            } while (1);
>>>>        } else {
>>>>            if (max310x_port_irq(s, 0) == IRQ_HANDLED)
> 
> Combining what I suggested earlier, with Jiri's comments but using
> for_each_clear_bit() allow to get rid of the original horrid mask:
> 

Thanks about suggested clear example. I'll test this in HW.

> ---
>          if (s->devtype->nr > 1) {
>                  do {
>                          unsigned int val = ~0;
> +                       unsigned long channel;
> +                       unsigned long irq;
> +                       bool read_reg_done = true;
>   
>                          WARN_ON_ONCE(regmap_read(s->regmap,
>                                                   MAX310X_GLOBALIRQ_REG, &val));
> -                       val = ((1 << s->devtype->nr) - 1) & ~val;
> -                       if (!val)
> +                       irq = val;
> +
> +                       for_each_clear_bit(channel, &irq, s->devtype->nr) {
> +                               read_reg_done = false;
> +
> +                               if (max310x_port_irq(s, channel) == IRQ_HANDLED)
> +                                       handled = true;
> +                       }
> +
> +                       if (read_reg_done)
>                                  break;
> -                       if (max310x_port_irq(s, fls(val) - 1) == IRQ_HANDLED)
> -                               handled = true;
>                  } while (1);
> ---
> 
> 
-- 
Many thanks,
Tapio Reijonen