Re: [PATCH v8 5/6] rtc: s35390a: Add pinctrl
From: sashiko-bot@kernel.org
Date: 2026-08-31 21:00:34
Also in:
linux-rtc
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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?
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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