Re: [PATCH] nfc: st-nci: Add error handling to IRQ handlers
From: Griffin Kroah-Hartman
Date: Mon Jul 27 2026 - 05:07:43 EST
On 7/19/26 2:27 PM, David Heidelberg wrote:
On 07/07/2026 16:21, Greg Kroah-Hartman wrote:
From: Griffin Kroah-Hartman <griffin@xxxxxxxxx>Heya,
Add ndlc_remove() to st_nci_i2c_probe() and st_nci_spi_probe() if the
devm_request_threaded_irq() function fails. This is to properly unwind
after ndlc_probe() was called prior to this.
Assisted-by: gkh_clanker_2000
Cc: David Heidelberg <david@xxxxxxx>
Cc: "Uwe Kleine-König (The Capable Hub)" <u.kleine-koenig@xxxxxxxxxxxx>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxxxxxxxx>
Signed-off-by: Griffin Kroah-Hartman <griffin@xxxxxxxxx>
Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
---
drivers/nfc/st-nci/i2c.c | 4 +++-
drivers/nfc/st-nci/spi.c | 4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)
the Sashiko review look reasonable here, check?
> diff --git a/drivers/nfc/st-nci/i2c.c b/drivers/nfc/st-nci/i2c.c
> index 9ae839a6f5cc0..7f727d9b97ba8 100644
> --- a/drivers/nfc/st-nci/i2c.c
> +++ b/drivers/nfc/st-nci/i2c.c
> @@ -243,8 +243,10 @@ static int st_nci_i2c_probe(struct i2c_client *client)
> st_nci_irq_thread_fn,
> IRQF_ONESHOT,
> ST_NCI_DRIVER_NAME, phy);
If userspace opens the device in the brief window between ndlc_probe()
and devm_request_threaded_irq(), could st_nci_open() attempt to enable
an unrequested IRQ and cause communication timeouts?
> - if (r < 0)
> + if (r < 0) {
> nfc_err(&client->dev, "Unable to register IRQ handler\n");
> + ndlc_remove(phy->ndlc);
Does calling ndlc_remove() here lead to an unbalanced IRQ disable?
When devm_request_threaded_irq() fails, this new error path calls
ndlc_remove(). This function eventually calls st_nci_i2c_disable(),
which unconditionally executes disable_irq_nosync().
Since the IRQ registration just failed, would disabling this unrequested
shared IRQ line break other devices and trigger an unbalanced IRQ disable
warning?
Will this error path also cause a blocking wait and memory leak?
When ndlc_remove() cascades to ndlc_close(), it sends a synchronous NCI
mode-set command via nci_prop_cmd().
Since the device was never opened by userspace during this failed probe,
the ST_NCI_RUNNING flag is not set. This causes st_nci_send() to return
-EBUSY and drop the transmission without freeing the sk_buff.
Because nci_prop_cmd() waits synchronously for a response that will never
arrive, won't this block the probe failure path for the full 2-second
timeout?
I agree, unwinding this error tree was much more complicated than I had assumed,
these warning messages look reasonable to me as well.
I will forgo this patch and ignore this change for now, Thanks for the catch!
Griffin