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.1On 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.
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
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
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
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
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
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.
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
© 2016 - 2025 Red Hat, Inc.