Re: [PATCH] nfc: st-nci: Add error handling to IRQ handlers

[Griffin Kroah-Hartman <griffin@kroah.com>]

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@kroah.com>
>>
>> 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@ixit.cz>
>> Cc: "Uwe Kleine-König (The Capable Hub)" <u.kleine-koenig@baylibre.com>
>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
>> Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> ---
>>   drivers/nfc/st-nci/i2c.c | 4 +++-
>>   drivers/nfc/st-nci/spi.c | 4 +++-
>>   2 files changed, 6 insertions(+), 2 deletions(-)
>>
> Heya,
>
> 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