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

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/[email protected]?part=1
---
--- Patch [1]: [PATCH net] nfc: trf7970a: initialize lock and timeout_work before requesting the IRQ ---
commit 4be20d05d9fa2b3917899acbab0b48bcd2d1125f
Author: Fan Wu <[email protected]>

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.