[PATCH] ns16550: ensure polling timer is disarmed

[PATCH] ns16550: ensure polling timer is disarmed

[dmkhn]

From: Denis Mukhin <dmukhin@ford.com> 

As it stands, polling timer is kept in the list of timers even after the
interrupts have been enabled / polling disabled on ns16550-compatible UART.

Ensure polling timer is removed from the timer list once UART interrupts are
enabled.

Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
 xen/drivers/char/ns16550.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index df7fff7f81df..299773d80065 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -191,6 +191,10 @@ static void cf_check ns16550_interrupt(int irq, void *dev_id)
     struct serial_port *port = dev_id;
     struct ns16550 *uart = port->uart;
 
+    /* Ensure polling timer is disarmed and removed from the timer list. */
+    if ( !uart->intr_works )
+        kill_timer(&uart->timer);
+
     uart->intr_works = 1;
 
     while ( !(ns_read_reg(uart, UART_IIR) & UART_IIR_NOINT) )
-- 
2.34.1

Re: [PATCH] ns16550: ensure polling timer is disarmed

[Jan Beulich]

On 30.07.2025 05:13, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com> 
> 
> As it stands, polling timer is kept in the list of timers even after the
> interrupts have been enabled / polling disabled on ns16550-compatible UART.
> 
> Ensure polling timer is removed from the timer list once UART interrupts are
> enabled.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>

Wasn't it Andrew(?) who suggested something along these lines? That would
want reflecting by a tag then.

Also, what's the real problem you want to solve here? The timer function
would be run one more time after ->intr_works is set, and then the timer
will be permanently inactive (up to a possible S3 resume). Is it being on
an inactive list an actual problem? (IOW I'd like to understand if the
change is merely cosmetic, or if there is some actual benefit.)

Jan

Re: [PATCH] ns16550: ensure polling timer is disarmed

[dmkhn]

On Wed, Jul 30, 2025 at 10:12:54AM +0200, Jan Beulich wrote:
> On 30.07.2025 05:13, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > As it stands, polling timer is kept in the list of timers even after the
> > interrupts have been enabled / polling disabled on ns16550-compatible UART.
> >
> > Ensure polling timer is removed from the timer list once UART interrupts are
> > enabled.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> 
> Wasn't it Andrew(?) who suggested something along these lines? That would
> want reflecting by a tag then.

Yes, indeed.

> 
> Also, what's the real problem you want to solve here? The timer function
> would be run one more time after ->intr_works is set, and then the timer
> will be permanently inactive (up to a possible S3 resume). Is it being on
> an inactive list an actual problem? (IOW I'd like to understand if the
> change is merely cosmetic, or if there is some actual benefit.)

My understanding is running polling timer one more time after the interrupts
are enabled is the issue: if there's a pending timer when it is known the
timer not needed, then the timer should be canceled.

> 
> Jan

Re: [PATCH] ns16550: ensure polling timer is disarmed

[Jan Beulich]

On 30.07.2025 20:31, dmkhn@proton.me wrote:
> On Wed, Jul 30, 2025 at 10:12:54AM +0200, Jan Beulich wrote:
>> On 30.07.2025 05:13, dmkhn@proton.me wrote:
>>> From: Denis Mukhin <dmukhin@ford.com>
>>>
>>> As it stands, polling timer is kept in the list of timers even after the
>>> interrupts have been enabled / polling disabled on ns16550-compatible UART.
>>>
>>> Ensure polling timer is removed from the timer list once UART interrupts are
>>> enabled.
>>>
>>> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
>>
>> Wasn't it Andrew(?) who suggested something along these lines? That would
>> want reflecting by a tag then.
> 
> Yes, indeed.
> 
>>
>> Also, what's the real problem you want to solve here? The timer function
>> would be run one more time after ->intr_works is set, and then the timer
>> will be permanently inactive (up to a possible S3 resume). Is it being on
>> an inactive list an actual problem? (IOW I'd like to understand if the
>> change is merely cosmetic, or if there is some actual benefit.)
> 
> My understanding is running polling timer one more time after the interrupts
> are enabled is the issue: if there's a pending timer when it is known the
> timer not needed, then the timer should be canceled.

And the effort of canceling outweighs the one extra running of the timer?

Jan

Re: [PATCH] ns16550: ensure polling timer is disarmed

[dmkhn]

On Thu, Jul 31, 2025 at 08:54:10AM +0200, Jan Beulich wrote:
> On 30.07.2025 20:31, dmkhn@proton.me wrote:
> > On Wed, Jul 30, 2025 at 10:12:54AM +0200, Jan Beulich wrote:
> >> On 30.07.2025 05:13, dmkhn@proton.me wrote:
> >>> From: Denis Mukhin <dmukhin@ford.com>
> >>>
> >>> As it stands, polling timer is kept in the list of timers even after the
> >>> interrupts have been enabled / polling disabled on ns16550-compatible UART.
> >>>
> >>> Ensure polling timer is removed from the timer list once UART interrupts are
> >>> enabled.
> >>>
> >>> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> >>
> >> Wasn't it Andrew(?) who suggested something along these lines? That would
> >> want reflecting by a tag then.
> >
> > Yes, indeed.
> >
> >>
> >> Also, what's the real problem you want to solve here? The timer function
> >> would be run one more time after ->intr_works is set, and then the timer
> >> will be permanently inactive (up to a possible S3 resume). Is it being on
> >> an inactive list an actual problem? (IOW I'd like to understand if the
> >> change is merely cosmetic, or if there is some actual benefit.)
> >
> > My understanding is running polling timer one more time after the interrupts
> > are enabled is the issue: if there's a pending timer when it is known the
> > timer not needed, then the timer should be canceled.
> 
> And the effort of canceling outweighs the one extra running of the timer?

I think so, because intr_works will not flip at run-time once set.
If so, no need to keep the timer ready to be rearmed.

> 
> Jan

Re: [PATCH] ns16550: ensure polling timer is disarmed

[Jan Beulich]

On 31.07.2025 23:42, dmkhn@proton.me wrote:
> On Thu, Jul 31, 2025 at 08:54:10AM +0200, Jan Beulich wrote:
>> On 30.07.2025 20:31, dmkhn@proton.me wrote:
>>> On Wed, Jul 30, 2025 at 10:12:54AM +0200, Jan Beulich wrote:
>>>> On 30.07.2025 05:13, dmkhn@proton.me wrote:
>>>>> From: Denis Mukhin <dmukhin@ford.com>
>>>>>
>>>>> As it stands, polling timer is kept in the list of timers even after the
>>>>> interrupts have been enabled / polling disabled on ns16550-compatible UART.
>>>>>
>>>>> Ensure polling timer is removed from the timer list once UART interrupts are
>>>>> enabled.
>>>>>
>>>>> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
>>>>
>>>> Wasn't it Andrew(?) who suggested something along these lines? That would
>>>> want reflecting by a tag then.
>>>
>>> Yes, indeed.
>>>
>>>>
>>>> Also, what's the real problem you want to solve here? The timer function
>>>> would be run one more time after ->intr_works is set, and then the timer
>>>> will be permanently inactive (up to a possible S3 resume). Is it being on
>>>> an inactive list an actual problem? (IOW I'd like to understand if the
>>>> change is merely cosmetic, or if there is some actual benefit.)
>>>
>>> My understanding is running polling timer one more time after the interrupts
>>> are enabled is the issue: if there's a pending timer when it is known the
>>> timer not needed, then the timer should be canceled.
>>
>> And the effort of canceling outweighs the one extra running of the timer?
> 
> I think so, because intr_works will not flip at run-time once set.
> If so, no need to keep the timer ready to be rearmed.

Well, to me it looks like a code size increase with extremely limited benefit.
Hence while likely I wouldn't outright NAK such a change, I also wouldn't ACK
it.

Jan

Re: [PATCH] ns16550: ensure polling timer is disarmed

[Roger Pau Monné]

On Fri, Aug 01, 2025 at 09:34:42AM +0200, Jan Beulich wrote:
> On 31.07.2025 23:42, dmkhn@proton.me wrote:
> > On Thu, Jul 31, 2025 at 08:54:10AM +0200, Jan Beulich wrote:
> >> On 30.07.2025 20:31, dmkhn@proton.me wrote:
> >>> On Wed, Jul 30, 2025 at 10:12:54AM +0200, Jan Beulich wrote:
> >>>> On 30.07.2025 05:13, dmkhn@proton.me wrote:
> >>>>> From: Denis Mukhin <dmukhin@ford.com>
> >>>>>
> >>>>> As it stands, polling timer is kept in the list of timers even after the
> >>>>> interrupts have been enabled / polling disabled on ns16550-compatible UART.
> >>>>>
> >>>>> Ensure polling timer is removed from the timer list once UART interrupts are
> >>>>> enabled.
> >>>>>
> >>>>> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> >>>>
> >>>> Wasn't it Andrew(?) who suggested something along these lines? That would
> >>>> want reflecting by a tag then.
> >>>
> >>> Yes, indeed.
> >>>
> >>>>
> >>>> Also, what's the real problem you want to solve here? The timer function
> >>>> would be run one more time after ->intr_works is set, and then the timer
> >>>> will be permanently inactive (up to a possible S3 resume). Is it being on
> >>>> an inactive list an actual problem? (IOW I'd like to understand if the
> >>>> change is merely cosmetic, or if there is some actual benefit.)
> >>>
> >>> My understanding is running polling timer one more time after the interrupts
> >>> are enabled is the issue: if there's a pending timer when it is known the
> >>> timer not needed, then the timer should be canceled.
> >>
> >> And the effort of canceling outweighs the one extra running of the timer?
> > 
> > I think so, because intr_works will not flip at run-time once set.
> > If so, no need to keep the timer ready to be rearmed.
> 
> Well, to me it looks like a code size increase with extremely limited benefit.
> Hence while likely I wouldn't outright NAK such a change, I also wouldn't ACK
> it.

Hm, indeed the net win of this is dubious, as the extra polling
interrupt would only happen once.  Using stop_timer() would be less
heavyweight than kill_timer().

Overall I think it needs justification in the commit message, as the
timer cannot be removed from the list of timers, otherwise it's usage
on resume from suspension will trigger an ASSERT, so part of the
commit message is stale.

Thanks, Roger.

Re: [PATCH] ns16550: ensure polling timer is disarmed

[Jan Beulich]

On 06.08.2025 12:53, Roger Pau Monné wrote:
> On Fri, Aug 01, 2025 at 09:34:42AM +0200, Jan Beulich wrote:
>> On 31.07.2025 23:42, dmkhn@proton.me wrote:
>>> On Thu, Jul 31, 2025 at 08:54:10AM +0200, Jan Beulich wrote:
>>>> On 30.07.2025 20:31, dmkhn@proton.me wrote:
>>>>> On Wed, Jul 30, 2025 at 10:12:54AM +0200, Jan Beulich wrote:
>>>>>> On 30.07.2025 05:13, dmkhn@proton.me wrote:
>>>>>>> From: Denis Mukhin <dmukhin@ford.com>
>>>>>>>
>>>>>>> As it stands, polling timer is kept in the list of timers even after the
>>>>>>> interrupts have been enabled / polling disabled on ns16550-compatible UART.
>>>>>>>
>>>>>>> Ensure polling timer is removed from the timer list once UART interrupts are
>>>>>>> enabled.
>>>>>>>
>>>>>>> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
>>>>>>
>>>>>> Wasn't it Andrew(?) who suggested something along these lines? That would
>>>>>> want reflecting by a tag then.
>>>>>
>>>>> Yes, indeed.
>>>>>
>>>>>>
>>>>>> Also, what's the real problem you want to solve here? The timer function
>>>>>> would be run one more time after ->intr_works is set, and then the timer
>>>>>> will be permanently inactive (up to a possible S3 resume). Is it being on
>>>>>> an inactive list an actual problem? (IOW I'd like to understand if the
>>>>>> change is merely cosmetic, or if there is some actual benefit.)
>>>>>
>>>>> My understanding is running polling timer one more time after the interrupts
>>>>> are enabled is the issue: if there's a pending timer when it is known the
>>>>> timer not needed, then the timer should be canceled.
>>>>
>>>> And the effort of canceling outweighs the one extra running of the timer?
>>>
>>> I think so, because intr_works will not flip at run-time once set.
>>> If so, no need to keep the timer ready to be rearmed.
>>
>> Well, to me it looks like a code size increase with extremely limited benefit.
>> Hence while likely I wouldn't outright NAK such a change, I also wouldn't ACK
>> it.
> 
> Hm, indeed the net win of this is dubious, as the extra polling
> interrupt would only happen once.  Using stop_timer() would be less
> heavyweight than kill_timer().
> 
> Overall I think it needs justification in the commit message, as the
> timer cannot be removed from the list of timers, otherwise it's usage
> on resume from suspension will trigger an ASSERT, so part of the
> commit message is stale.

That could be compensated by another init_timer(), though. (In fact when
originally looking at the patch, I [wrongly] thought that path was taken
on resume, so didn't comment on that aspect.)

Jan

Re: [PATCH] ns16550: ensure polling timer is disarmed

[Roger Pau Monné]

On Wed, Jul 30, 2025 at 03:13:31AM +0000, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com> 
> 
> As it stands, polling timer is kept in the list of timers even after the
> interrupts have been enabled / polling disabled on ns16550-compatible UART.
> 
> Ensure polling timer is removed from the timer list once UART interrupts are
> enabled.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
>  xen/drivers/char/ns16550.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index df7fff7f81df..299773d80065 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -191,6 +191,10 @@ static void cf_check ns16550_interrupt(int irq, void *dev_id)
>      struct serial_port *port = dev_id;
>      struct ns16550 *uart = port->uart;
>  
> +    /* Ensure polling timer is disarmed and removed from the timer list. */
> +    if ( !uart->intr_works )
> +        kill_timer(&uart->timer);

I think if anything you should issue a stop_timer() call, but not a
kill_timer() one, otherwise the uart code will hit an ASSERT when
calling set_timer() from ns16550_setup_postirq()? (that's from
_ns16550_resume()).

Thanks, Roger.