drivers/tty/serial/max310x.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
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;
+
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);
} while (1);
} else {
if (max310x_port_irq(s, 0) == IRQ_HANDLED)
---
base-commit: c8bc81a52d5a2ac2e4b257ae123677cf94112755
change-id: 20250903-master-max310x-improve-interrupt-handling-aa22b7ba1c1d
Best regards,
--
Tapio Reijonen <tapio.reijonen@vaisala.com>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 &).
> 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)
thanks,
--
js
suse labs
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)
>
> thanks,
--
Many thanks,
Tapio Reijonen
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:
---
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);
---
--
Hugo Villeneuve
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
Hi,
On Wed, 03 Sep 2025 09:23:04 +0000
Tapio Reijonen <tapio.reijonen@vaisala.com> 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.
To improve clarity and reduce confusion, maybe change to
something like:
... 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 improve interrupt service routine to handle all
> interrupt sources, e.g. UARTs when a global IRQ is detected.
The current code already handle all interrupt sources. What you
maybe could be saying is that you handle all individual interrupt
sources before reading the global IRQ register again?
You could also add in your commit message that your modification has the
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>
> ---
> 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));
You changed the indentation here...
> +
> + val = ((1 << s->devtype->nr) - 1) & ~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);
> } while (1);
> } else {
> if (max310x_port_irq(s, 0) == IRQ_HANDLED)
Maybe you could simplify (and improve readability) with this instead:
---
val = ((1 << s->devtype->nr) - 1) & ~val;
if (!val)
break;
- if (max310x_port_irq(s, fls(val) - 1) == IRQ_HANDLED)
- handled = true;
+
+ do {
+ unsigned int channel;
+
+ channel = fls(val) - 1;
+
+ if (max310x_port_irq(s, channel) == IRQ_HANDLED)
+ handled = true;
+
+ val &= ~(1 << channel);
+ } while (val);
---
>
> ---
> base-commit: c8bc81a52d5a2ac2e4b257ae123677cf94112755
> change-id: 20250903-master-max310x-improve-interrupt-handling-aa22b7ba1c1d
>
> Best regards,
> --
> Tapio Reijonen <tapio.reijonen@vaisala.com>
>
>
>
--
Hugo Villeneuve
Hi,
On 9/3/25 18:37, Hugo Villeneuve wrote:
> Hi,
>
> On Wed, 03 Sep 2025 09:23:04 +0000
> Tapio Reijonen <tapio.reijonen@vaisala.com> 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.
>
> To improve clarity and reduce confusion, maybe change to
> something like:
>
> ... 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 improve interrupt service routine to handle all
>> interrupt sources, e.g. UARTs when a global IRQ is detected.
>
> The current code already handle all interrupt sources. What you
> maybe could be saying is that you handle all individual interrupt
> sources before reading the global IRQ register again?
>
> You could also add in your commit message that your modification has the
> nice side-effect of improving the efficiency of the driver by reducing
> the number of reads of the global IRQ register.
>
>
Try to improve commit message in next patch version.
>>
>> 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));
>
> You changed the indentation here...
>
I'll fix the indentation, my fault.
>> +
>> + val = ((1 << s->devtype->nr) - 1) & ~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);
>> } while (1);
>> } else {
>> if (max310x_port_irq(s, 0) == IRQ_HANDLED)
>
> Maybe you could simplify (and improve readability) with this instead:
>
I am reviewing the proposal.
> ---
> val = ((1 << s->devtype->nr) - 1) & ~val;
> if (!val)
> break;
>
> - if (max310x_port_irq(s, fls(val) - 1) == IRQ_HANDLED)
> - handled = true;
> +
> + do {
> + unsigned int channel;
> +
> + channel = fls(val) - 1;
> +
> + if (max310x_port_irq(s, channel) == IRQ_HANDLED)
> + handled = true;
> +
> + val &= ~(1 << channel);
> + } while (val);
> ---
>
>>
>> ---
>> base-commit: c8bc81a52d5a2ac2e4b257ae123677cf94112755
>> change-id: 20250903-master-max310x-improve-interrupt-handling-aa22b7ba1c1d
>>
>> Best regards,
>> --
>> Tapio Reijonen <tapio.reijonen@vaisala.com>
>>
>>
>>
>
> --
> Hugo Villeneuve
--
Many thanks,
Tapio Reijonen
© 2016 - 2025 Red Hat, Inc.