Re: [PATCH v2] tty: serial: max3100: shut down timer before freeing port

[Greg KH <gregkh@linuxfoundation.org>]

On Sat, Aug 01, 2026 at 06:12:08AM +0000, Fan Wu wrote:
> max3100_shutdown() stops the polling timer but returns early during
> system suspend. If the SPI device is unbound before resume, the serial
> core does not call max3100_shutdown() again, so max3100_remove() frees
> the port while the timer remains armed. max3100_timeout() can then
> access the freed port and re-arm the timer.
> 
> Add final timer teardown to max3100_remove() and use
> timer_shutdown_sync() to prevent a racing callback from re-arming it.
> Also drain the IRQ and workqueue before freeing the port.
> 
> Keep timer_delete_sync() in max3100_shutdown() so that a subsequent
> open() can re-arm the timer.
> 
> Introduce an irq_registered flag to track whether the IRQ is registered,
> independently of port->irq, so a failed request_irq() can be retried on
> the next open().
> 
> Found by static analysis.
> 
> Fixes: 7831d56b0a35 ("tty: MAX3100")
> Cc: stable@vger.kernel.org # 6.2+
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---
> Changes since v1:
>   - Drop the shared drain helper; call timer_shutdown_sync() only in
>     max3100_remove(), keeping timer_delete_sync() in max3100_shutdown()
>     so a later open() can re-arm the timer.
>   - Track IRQ registration with a flag instead of clearing port->irq,
>     so a failed request_irq() can be retried on the next open().
> 
> v1: https://lore.kernel.org/all/20260721035631.3186613-1-fanwu01@zju.edu.cn/
> ---
>  drivers/tty/serial/max3100.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
> index 44b745fa26c6..7bc3c5cfe886 100644
> --- a/drivers/tty/serial/max3100.c
> +++ b/drivers/tty/serial/max3100.c
> @@ -107,6 +107,7 @@ struct max3100_port {
>  	int  force_end_work;
>  	/* need to know we are suspending to avoid deadlock on workqueue */
>  	int suspending;
> +	bool irq_registered;

LLMs _love_ to use boolean flags to attempt to figure things out that
they can't seem to determine.  Are you _SURE_ this really is needed?
How about unwinding things better so it's not required?  You are just
adding another "state" to the device, adding to the complexity overall,
which is generally not a good idea.

And do you have this hardware to test this with?

thanks,

greg k-h