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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@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.

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