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

[Jiri Slaby <jirislaby@kernel.org>]

On 05. 09. 25, 10:54, 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 the highest UART
> until no more interrupt and then service another UART
> (ex: UART3 will be serviced for as long as there are interrupts
> for it, then UART2, etc).
> 
> This commit handle all individual interrupt sources before
> reading the global IRQ register again.
> 
> This commit has also a nice side-effect of improving the efficiency
> of the driver by reducing the number of reads of the global
> IRQ register.
> 
> Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
> ---
> Changes in v2:
> - Improve content of the commit message
> - Fix a line indention in prevoius patch
> - According review comments, changed to use for_each_clear_bit
>    to simplify serve all IRQs in a loop.
>    NOTE: When a bit in IRQ[n] is set 0 the associated UART’s
>    internal IRQ is generated.
> - Link to v1: https://lore.kernel.org/r/20250903-master-max310x-improve-interrupt-handling-v1-1-bfb44829e760@vaisala.com
> ---
>   drivers/tty/serial/max310x.c | 18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index ce260e9949c3c268e706b2615d6fc01adc21e49b..464f2828bd304198d79046e79b2a8a0893e5cd77 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
> @@ -825,14 +825,24 @@ static irqreturn_t max310x_ist(int irq, void *dev_id)
>   	if (s->devtype->nr > 1) {
>   		do {
>   			unsigned int val = ~0;
> +			unsigned long irq;
> +			unsigned long port;
> +			bool 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(port, &irq, s->devtype->nr) {

port can be uint, no need for ulong.

> +				done = false;
> +
> +				if (max310x_port_irq(s, port) == IRQ_HANDLED)
> +					handled = true;
> +			}

Hey, this looks to be the order of magnitude saner!

> +
> +			if (done)
>   				break;
> -			if (max310x_port_irq(s, fls(val) - 1) == IRQ_HANDLED)
> -				handled = true;
>   		} while (1);

So now, this should perhaps be "while (!done)"?

thanks,
-- 
js
suse labs