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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Aug 01, 2025 at 09:34:42AM +0200, Jan Beulich wrote:
> On 31.07.2025 23:42, dmkhn@xxxxxxxxx wrote:
> > On Thu, Jul 31, 2025 at 08:54:10AM +0200, Jan Beulich wrote:
> >> On 30.07.2025 20:31, dmkhn@xxxxxxxxx wrote:
> >>> On Wed, Jul 30, 2025 at 10:12:54AM +0200, Jan Beulich wrote:
> >>>> On 30.07.2025 05:13, dmkhn@xxxxxxxxx wrote:
> >>>>> From: Denis Mukhin <dmukhin@xxxxxxxx>
> >>>>>
> >>>>> 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@xxxxxxxx>
> >>>>
> >>>> 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.