Re: [PATCH v8 5/6] rtc: s35390a: Add pinctrl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware reset clears persistent pinctrl pin configurations without restoring them, breaking clock outputs.
- [Medium] Lockless read of `s35390a->pinfunction` introduces a TOCTOU race with concurrent pinctrl consumers, potentially overwriting valid pinmux configurations with fallbacks.
--

commit b6394d0bf7b85783daa05f6575f0b6579cf01843
Author: Markus Probst <markus.probst@posteo.de>
Subject: rtc: s35390a: Add pinctrl

This commit adds pinctrl support to the s35390a RTC driver, allowing
the configuration of pin functions like wake-up, minute periodic interrupt,
or disabling the output. It falls back to previous behavior if no function
is configured.

> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c

[ ... ]

> @@ -64,6 +89,10 @@ MODULE_DEVICE_TABLE(of, s35390a_of_match);
>  
>  struct s35390a {
>  	struct i2c_client *client[8];
> +	struct rtc_device *rtc;
> +
> +	struct mutex pinfunction_lock; /* lock preventing concurrent access of pin function */
> +	int pinfunction[2];
>  };

[Severity: High]
If a user triggers a hardware reset at runtime via the RTC_VL_CLR ioctl,
it calls s35390a_init() which completely clears the STATUS2 register:

drivers/rtc/rtc-s35390a.c:s35390a_rtc_ioctl() {
	...
	case RTC_VL_CLR:
		/* update flag and clear register */
		err = s35390a_init(s35390a, &sts);
	...
}

drivers/rtc/rtc-s35390a.c:s35390a_init() {
	...
	*sts = S35390A_FLAG_RESET | S35390A_FLAG_24H;
	ret = s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, sts, 1);
	...
}

Because s35390a->pinfunction retains the previously configured pinctrl
state but the hardware has been reset, will this permanently break the
pinctrl outputs (such as PMIN clock outputs) since the software and
hardware are now out of sync?

[ ... ]

> @@ -539,9 +762,33 @@ static int s35390a_probe(struct i2c_client *client)

[ ... ]

> +	err = pinctrl_enable(pctl);
> +	if (err)
> +		return dev_err_probe(dev, err, "pinctrl enable failed\n");
> +
> +	/* If no pinmux function is defined in DT, fallback to previous behaviour */
> +	fallback[0] = S35390A_FUNC_IGNORE;
> +	fallback[1] = S35390A_FUNC_WAKEUP;
> +
> +	for (i = 0; i < ARRAY_SIZE(s35390a_pin_groups); i++) {
> +		if (s35390a->pinfunction[i] == -1) {
> +			err = s35390a_pinctrl_set_mux(pctl, fallback[i], i);

[Severity: Medium]
Since pinctrl_enable() makes the pinctrl device globally visible before
this loop runs, is there a TOCTOU race condition if a consumer driver
requests a pin state concurrently?

If a consumer (e.g., probing asynchronously) sets the hardware state and
updates pinfunction right before the lockless s35390a->pinfunction[i] == -1
check evaluates to true, could this probe thread subsequently acquire the
lock and overwrite the explicitly requested configuration with the fallback
state?

> +			if (err)
> +				return err;
> +		}
> +	}
> +
>  	nvmem_cfg.priv = s35390a;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-rtc_s35390a_int1-v8-0-dac1dc86ce1f@posteo.de?part=5