drivers/tty/serial/max3100.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
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;
struct timer_list timer;
};
@@ -538,8 +539,10 @@ static void max3100_shutdown(struct uart_port *port)
destroy_workqueue(s->workqueue);
s->workqueue = NULL;
}
- if (port->irq)
+ if (s->irq_registered) {
free_irq(port->irq, s);
+ s->irq_registered = false;
+ }
/* set shutdown mode to save power */
max3100_sr(s, MAX3100_WC | MAX3100_SHDN, &rx);
@@ -575,12 +578,12 @@ static int max3100_startup(struct uart_port *port)
ret = request_irq(port->irq, max3100_irq, IRQF_TRIGGER_FALLING, "max3100", s);
if (ret < 0) {
dev_warn(&s->spi->dev, "cannot allocate irq %d\n", port->irq);
- port->irq = 0;
destroy_workqueue(s->workqueue);
s->workqueue = NULL;
return -EBUSY;
}
+ s->irq_registered = true;
s->conf_commit = 1;
max3100_dowork(s);
/* wait for clock to settle */
@@ -752,6 +755,17 @@ static void max3100_remove(struct spi_device *spi)
if (max3100s[i] == s) {
dev_dbg(&spi->dev, "%s: removing port %d\n", __func__, i);
uart_remove_one_port(&max3100_uart_driver, &max3100s[i]->port);
+
+ s->force_end_work = 1;
+ timer_shutdown_sync(&s->timer);
+ if (s->irq_registered) {
+ free_irq(s->port.irq, s);
+ s->irq_registered = false;
+ }
+ if (s->workqueue) {
+ destroy_workqueue(s->workqueue);
+ s->workqueue = NULL;
+ }
kfree(max3100s[i]);
max3100s[i] = NULL;
break;
--
2.34.1On 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
Hi Greg, Sorry, I don't have MAX3100 hardware, so I have not tested this on hardware. You're right about the extra state. I'll rework the cleanup to use the existing startup/unwind state instead of adding irq_registered, then send v3 after rebuilding it. Thanks, Fan > On Aug 3, 2026, at 20:56, Greg KH <gregkh@linuxfoundation.org> wrote: > 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
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() may 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 free the IRQ and destroy the workqueue there before freeing the
port. Keep timer_delete_sync() in max3100_shutdown() so that a
subsequent open() can re-arm the timer.
The workqueue is created before request_irq() and destroyed on both
request_irq() failure and normal shutdown. Its presence at remove thus
identifies the IRQ left registered when suspend bypasses shutdown.
This issue was found by an in-house static analysis tool.
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 v2:
- Drop irq_registered; use the workqueue lifetime to decide whether
remove must release an IRQ left by the suspend path.
v1: https://lore.kernel.org/all/20260721035631.3186613-1-fanwu01@zju.edu.cn/
v2: https://lore.kernel.org/all/20260801061208.356142-1-fanwu01@zju.edu.cn/
---
drivers/tty/serial/max3100.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index 44b745fa26c6..48c66b6e1c18 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -537,9 +537,8 @@ static void max3100_shutdown(struct uart_port *port)
if (s->workqueue) {
destroy_workqueue(s->workqueue);
s->workqueue = NULL;
- }
- if (port->irq)
free_irq(port->irq, s);
+ }
/* set shutdown mode to save power */
max3100_sr(s, MAX3100_WC | MAX3100_SHDN, &rx);
@@ -752,6 +751,14 @@ static void max3100_remove(struct spi_device *spi)
if (max3100s[i] == s) {
dev_dbg(&spi->dev, "%s: removing port %d\n", __func__, i);
uart_remove_one_port(&max3100_uart_driver, &max3100s[i]->port);
+
+ s->force_end_work = 1;
+ timer_shutdown_sync(&s->timer);
+ if (s->workqueue) {
+ destroy_workqueue(s->workqueue);
+ s->workqueue = NULL;
+ free_irq(s->port.irq, s);
+ }
kfree(max3100s[i]);
max3100s[i] = NULL;
break;
--
2.34.1On Wed, Aug 05, 2026 at 12:40:39AM +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() may 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 free the IRQ and destroy the workqueue there before freeing the > port. Keep timer_delete_sync() in max3100_shutdown() so that a > subsequent open() can re-arm the timer. > > The workqueue is created before request_irq() and destroyed on both > request_irq() failure and normal shutdown. Its presence at remove thus > identifies the IRQ left registered when suspend bypasses shutdown. > > This issue was found by an in-house static analysis tool. There are still issues: https://sashiko.dev/#/patchset/20260805004039.382698-1-fanwu01@zju.edu.cn
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() may 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 free the IRQ and destroy the workqueue there before freeing the
port. Keep timer_delete_sync() in max3100_shutdown() so that a
subsequent open() can re-arm the timer.
The workqueue is created before request_irq() and destroyed on both
request_irq() failure and normal shutdown. Its presence at remove thus
identifies the IRQ left registered when suspend bypasses shutdown.
Free the IRQ before destroying the workqueue, in both
max3100_shutdown() and max3100_remove(), and cancel the pending work
in between. free_irq() waits for a running interrupt handler to
finish and no new handler can start afterwards, so a racing handler
cannot queue work on a workqueue that is being destroyed; the
force_end_work check in max3100_dowork() only narrows that window, as
a handler delayed between the check and queue_work() could still run
after the teardown has cleared the workqueue pointer. Cancelling the
work also keeps destroy_workqueue() from waiting on a queued work item
in case remove() ever runs while the freezable workqueue is still
frozen, e.g. device removal during the resume window, before
workqueues are thawed.
This issue was found by an in-house static analysis tool.
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 v3:
- Free the IRQ before destroying the workqueue in both
max3100_shutdown() and max3100_remove(), and cancel the pending
work in between. This closes a race where an interrupt handler
that already passed the force_end_work check in max3100_dowork()
could queue work on a cleared or destroyed workqueue pointer. It
also keeps destroy_workqueue() from waiting on a queued work item
should remove() ever run while the freezable workqueue is still
frozen, e.g. device removal during the resume window, before
workqueues are thawed.
No MAX3100 hardware was available for this change: verified by
inspection and compilation only.
v1: https://lore.kernel.org/all/20260721035631.3186613-1-fanwu01@zju.edu.cn/
v2: https://lore.kernel.org/all/20260801061208.356142-1-fanwu01@zju.edu.cn/
v3: https://lore.kernel.org/all/20260805004039.382698-1-fanwu01@zju.edu.cn/
---
drivers/tty/serial/max3100.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index 44b745fa26c6..e8027c09c619 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -535,11 +535,11 @@ static void max3100_shutdown(struct uart_port *port)
timer_delete_sync(&s->timer);
if (s->workqueue) {
+ free_irq(port->irq, s);
+ cancel_work_sync(&s->work);
destroy_workqueue(s->workqueue);
s->workqueue = NULL;
}
- if (port->irq)
- free_irq(port->irq, s);
/* set shutdown mode to save power */
max3100_sr(s, MAX3100_WC | MAX3100_SHDN, &rx);
@@ -752,6 +752,15 @@ static void max3100_remove(struct spi_device *spi)
if (max3100s[i] == s) {
dev_dbg(&spi->dev, "%s: removing port %d\n", __func__, i);
uart_remove_one_port(&max3100_uart_driver, &max3100s[i]->port);
+
+ s->force_end_work = 1;
+ timer_shutdown_sync(&s->timer);
+ if (s->workqueue) {
+ free_irq(s->port.irq, s);
+ cancel_work_sync(&s->work);
+ destroy_workqueue(s->workqueue);
+ s->workqueue = NULL;
+ }
kfree(max3100s[i]);
max3100s[i] = NULL;
break;
--
2.34.1© 2016 - 2026 Red Hat, Inc.