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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unconditional dependency on PINCTRL removes the RTC driver on non-pinctrl platforms.
- [Medium] The 'clock' pin function is advertised but silently falls back to disabling the pin rather than returning an error.
- [Medium] TOCTOU race in `s35390a_probe` when reading `pinfunction` without lock after `pinctrl_enable`.
--

--- Patch [5]: [PATCH v6 5/6] rtc: s35390a: Add pinctrl ---
commit 73fe5a984406c6bfdc0258a390d1f790625e1082
Author: Markus Probst <markus.probst@posteo.de>

rtc: s35390a: Add pinctrl

This commit adds pinctrl support to the S35390A RTC driver, allowing
configuration of pin functions like wake up, disabled, and periodic
interrupts. If no function is configured, it falls back to the previous
behavior.

> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 01def82318731..2d79710c48375 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -668,7 +668,9 @@ config RTC_DRV_RC5T619
>  
>  config RTC_DRV_S35390A
>  	tristate "Seiko Instruments S-35390A"
> +	depends on PINCTRL

[Severity: High]
Will this unconditional dependency cause the RTC driver to be silently dropped
on platforms without PINCTRL enabled?

This could break backwards compatibility for existing driver users by changing
an optional feature into a hard dependency.

>  	select BITREVERSE
> +	select GENERIC_PINCONF
>  	help
>  	  If you say yes here you will get support for the Seiko

> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> index 8e3616c65d2c0..a2ddce91ef7ac 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c
[ ... ]
> @@ -458,13 +520,170 @@ static int s35390a_nvmem_write(void *priv, unsigned int offset, void *val,
[ ... ]
> +static int s35390a_pinctrl_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
> +				   unsigned int group)
> +{
[ ... ]
> +	switch (function) {
> +	case S35390A_FUNC_DISABLE:
> +	case S35390A_FUNC_CLOCK: /* not implemented */
> +		status2 = (status2 & ~mask) | S35390A_INT_MODE_NOINTR;
> +		break;

[Severity: Medium]
Since the clock function is not implemented, should this return an error
instead of silently disabling the pin?

Users specifying the 'clock' function in the device tree might assume the pin
is outputting a clock signal, but it will be disabled instead.

> +	case S35390A_FUNC_WAKEUP:
> +		flag = group == 0 ? S35390A_INT1_MODE_ALARM : S35390A_INT2_MODE_ALARM;
[ ... ]
> @@ -533,9 +758,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) {

[Severity: Medium]
Is there a data race here since the pinfunction array is accessed locklessly
after pinctrl_enable() has made the pinctrl device available?

If another consumer requests the pins concurrently right after pinctrl_enable()
modifying pinfunction, could their configuration be overwritten by this
fallback logic?

> +			err = s35390a_pinctrl_set_mux(pctl, fallback[i], i);
> +			if (err)
> +				return err;
> +		}
> +	}
> +
>  	nvmem_cfg.priv = s35390a;
>  	err = devm_rtc_nvmem_register(rtc, &nvmem_cfg);
>  	if (err)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-rtc_s35390a_int1-v6-0-df96569e3412@posteo.de?part=5