[PATCH net] nfc: trf7970a: initialize lock and timeout_work before requesting the IRQ

[PATCH net] nfc: trf7970a: initialize lock and timeout_work before requesting the IRQ

[Fan Wu]

trf7970a_probe() calls devm_request_threaded_irq() -- which registers the
threaded handler trf7970a_irq() -- before it initializes trf->lock with
mutex_init() and trf->timeout_work with INIT_DELAYED_WORK(). The handler
takes trf->lock at entry and, through the helpers it dispatches to
(fill_fifo/drain_fifo/issue_eof/transmit), can schedule_delayed_work() on
trf->timeout_work. In the window between the IRQ request succeeding and
those two initializations, an IRQ would therefore touch an uninitialized
mutex and/or schedule an uninitialized delayed_work.

Initialize trf->lock and trf->timeout_work before requesting the IRQ. On
IRQ-request failure, destroy the mutex inline and return directly: the
request failed, so no handler is registered and nothing else will touch
the mutex.

This issue was found by an in-house static analysis tool.

Fixes: 165063f1dac4 ("NFC: trf7970a: Add driver with ISO/IEC 14443 Type 2 Tag Support")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/nfc/trf7970a.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
index f22e091..bc4d974 100644
--- a/drivers/nfc/trf7970a.c
+++ b/drivers/nfc/trf7970a.c
@@ -2124,18 +2124,19 @@ static int trf7970a_probe(struct spi_device *spi)
 		}
 	}
 
+	mutex_init(&trf->lock);
+	INIT_DELAYED_WORK(&trf->timeout_work, trf7970a_timeout_work_handler);
+
 	ret = devm_request_threaded_irq(trf->dev, spi->irq, NULL,
 					trf7970a_irq,
 					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
 					"trf7970a", trf);
 	if (ret) {
 		dev_err(trf->dev, "Can't request IRQ#%d: %d\n", spi->irq, ret);
+		mutex_destroy(&trf->lock);
 		return ret;
 	}
 
-	mutex_init(&trf->lock);
-	INIT_DELAYED_WORK(&trf->timeout_work, trf7970a_timeout_work_handler);
-
 	trf->vin_regulator = devm_regulator_get(&spi->dev, "vin");
 	if (IS_ERR(trf->vin_regulator)) {
 		ret = PTR_ERR(trf->vin_regulator);
-- 
2.34.1

Re: [PATCH net] nfc: trf7970a: initialize lock and timeout_work before requesting the IRQ

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260728031021.230831-1-fanwu01@zju.edu.cn?part=1
---
--- Patch [1]: [PATCH net] nfc: trf7970a: initialize lock and timeout_work before requesting the IRQ ---
commit 4be20d05d9fa2b3917899acbab0b48bcd2d1125f
Author: Fan Wu <fanwu01@zju.edu.cn>

nfc: trf7970a: initialize lock and timeout_work before requesting the IRQ

This moves mutex_init(&trf->lock) and INIT_DELAYED_WORK(&trf->timeout_work,
...) above devm_request_threaded_irq() in trf7970a_probe(), so an interrupt
arriving right after the handler is registered cannot touch an
uninitialized mutex or delayed_work. The IRQ-request failure path destroys
the mutex inline and returns. Tagged for stable.

> diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
> index 60883001fa5d..3e57f8cf229e 100644
> --- a/drivers/nfc/trf7970a.c
> +++ b/drivers/nfc/trf7970a.c
> @@ -2124,18 +2124,19 @@ static int trf7970a_probe(struct spi_device *spi)
>  		}
>  	}
>  
> +	mutex_init(&trf->lock);
> +	INIT_DELAYED_WORK(&trf->timeout_work, trf7970a_timeout_work_handler);
> +
>  	ret = devm_request_threaded_irq(trf->dev, spi->irq, NULL,
>  					trf7970a_irq,
>  					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>  					"trf7970a", trf);
>  	if (ret) {
>  		dev_err(trf->dev, "Can't request IRQ#%d: %d\n", spi->irq, ret);
> +		mutex_destroy(&trf->lock);
>  		return ret;
>  	}
>  
> -	mutex_init(&trf->lock);
> -	INIT_DELAYED_WORK(&trf->timeout_work, trf7970a_timeout_work_handler);
> -

[Severity: High]
This isn't a bug introduced by this patch, but since the patch fixes the
setup side of exactly this ordering problem and is tagged for stable, is
the mirror-image teardown window worth addressing at the same time?

Nothing in this driver ever synchronously cancels trf->timeout_work, and
the devm-registered IRQ is only released by devres_release_all(), i.e.
after trf7970a_remove() returns and after probe's error unwinding at the
err_destroy_lock label.

First case, work still queued at unbind. trf7970a_tg_listen() arms the work
in mode-detect state:

	trf->state = mode_detect ? TRF7970A_ST_LISTENING_MD :
				   TRF7970A_ST_LISTENING;

	schedule_delayed_work(&trf->timeout_work, msecs_to_jiffies(timeout));

but trf7970a_shutdown() has no arm for TRF7970A_ST_LISTENING_MD:

	switch (trf->state) {
	case TRF7970A_ST_WAIT_FOR_TX_FIFO:
	...
	case TRF7970A_ST_LISTENING:
		trf7970a_send_err_upstream(trf, -ECANCELED);
		fallthrough;
	...
	default:
		break;
	}

so the queued work survives, and trf7970a_remove() goes on to

	nfc_digital_unregister_device(trf->ddev);
	nfc_digital_free_device(trf->ddev);
	...
	mutex_destroy(&trf->lock);

and returns, after which devres frees the devm_kzalloc'ed trf. Can the
still-armed delayed_work timer, which lives inside that freed allocation,
then fire and have trf7970a_timeout_work_handler() dereference trf->dev,
trf->state and trf->lock after free?

Second case, work already running. The only stop used anywhere in the file
is the non-synchronous variant, in trf7970a_send_err_upstream():

	cancel_delayed_work(&trf->timeout_work);

If trf7970a_timeout_work_handler() is already blocked on
mutex_lock(&trf->lock) while trf7970a_remove() holds it, that cancel
returns 0 and remove() continues. Once remove() unlocks, does the handler
acquire the lock and run concurrently with the rest of remove(), reaching
trf7970a_send_err_upstream() -> trf7970a_send_upstream() -> trf->cb(trf->ddev,
...) after nfc_digital_free_device(trf->ddev), and finally
mutex_unlock(&trf->lock) on a mutex already destroyed inside memory devres
frees as soon as remove() returns? nfc_digital_unregister_device() only
sync-cancels its own poll_work/cmd_work/cmd_complete_work, it does not
touch trf->timeout_work.

There is also a debug-only aspect on the newly funnelled error paths. All
failures after the IRQ request (VIN and VDD_IO get/enable, ddev allocation,
startup, nfc_digital_register_device) reach

	err_destroy_lock:
		mutex_destroy(&trf->lock);
		return ret;

while the devm IRQ is still armed, and trf7970a_irq() takes the lock as its
first action before any state check:

	mutex_lock(&trf->lock);

	if (trf->state == TRF7970A_ST_RF_OFF) {

With CONFIG_DEBUG_MUTEXES, mutex_destroy() clears lock->magic, so does a
late interrupt on these paths (and on the remove() path) trip
DEBUG_LOCKS_WARN_ON in the mutex debug checks?

Would adding cancel_delayed_work_sync(&trf->timeout_work) plus a
devm_free_irq()/disable_irq() (or registering the mutex and work teardown
with devm_add_action_or_reset() so release order mirrors setup) before
mutex_destroy() in both trf7970a_remove() and the probe unwind, and handling
TRF7970A_ST_LISTENING_MD in trf7970a_shutdown(), close these windows?

Verified with git grep on drivers/nfc/trf7970a.c: no occurrences of
cancel_delayed_work_sync, flush_delayed_work, disable_irq or devm_free_irq.