Re: [PATCH net-next 2/2] nfc: s3fwrn5: support the S3NRN4V variant

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in clock request seeding can leave the NFC reference clock permanently enabled, breaking the TX PLL lock.
--

commit 342a387574bb664bcbcb353a01bca5f492266532
Author: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>

nfc: s3fwrn5: support the S3NRN4V variant

This commit adds support for the S3NRN4V NFC controller variant. It skips
the firmware download step and configures the reference clock via the
setup hook. It also adds optional clock request gpio handling to gate the
clock dynamically.

> diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
> index e9a34d27a3698..88a4988791efd 100644
> --- a/drivers/nfc/s3fwrn5/i2c.c
> +++ b/drivers/nfc/s3fwrn5/i2c.c
[ ... ]
> @@ -172,15 +217,63 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client)
[ ... ]
> +		ret = devm_request_threaded_irq(&client->dev, clk_req_irq, NULL,
> +						s3fwrn5_i2c_clk_req_thread,
> +						IRQF_TRIGGER_RISING |
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_ONESHOT,
> +						"s3fwrn5_clk_req", phy);
> +		if (ret)
> +			return ret;
> +
> +		/* Seed the clock state from the current CLK_REQ level. */
> +		s3fwrn5_i2c_clk_set(phy,
> +				    gpiod_get_value_cansleep(phy->gpio_clk_req) > 0);

[Severity: High]
Could this introduce a race condition that leaves the clock permanently enabled?

Because devm_request_threaded_irq() is called before the initial clock state
is seeded, the interrupt is unmasked and can fire immediately. If the
hardware toggles the gpio during this window, the sequence could look like:

Probe thread in s3fwrn5_i2c_probe():
    evaluates gpiod_get_value_cansleep() and reads a high state.

Hardware:
    toggles the gpio to low.

IRQ thread running s3fwrn5_i2c_clk_req_thread():
    reads the new low state
    calls s3fwrn5_i2c_clk_set(phy, false)
    correctly disables the clock

Probe thread resumes:
    calls s3fwrn5_i2c_clk_set(phy, true) using the stale time-of-check state
    acquires clk_lock and overrides the correct state from the IRQ

Since the gpio is already low, no subsequent falling edge will occur to
disable the clock. As mentioned in the commit message, leaving the clock
always-on prevents the chip's transmit pll from locking on a fresh start.

Should the initial clock state be seeded before requesting the interrupt, or
perhaps synchronized with clk_lock to ensure the probe thread does not apply
a stale state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703202601.78563-1-jorijnvdgraaf@catcrafts.net?part=2