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, dm...@proton.me wrote: >>> On Thu, Jul 31, 2025 at 08:54:10AM +0200, Jan Beulich wrote: >>>> On 30.07.2025 20:31, dm...@proton.me wrote: >>>>> On Wed, Jul 30, 2025 at 10:12:54AM +0200, Jan Beulich wrote: >>>>>> On 30.07.2025 05:13, dm...@proton.me wrote: >>>>>>> From: Denis Mukhin <dmuk...@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 <dmuk...@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
