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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Adding a hard dependency on `PINCTRL` silently breaks the build for existing platforms that do not use the pin control subsystem.
- [High] The driver references `pinconf_generic_dt_node_to_map_all` without selecting `GENERIC_PINCONF`, causing link errors on some configurations.
- [High] The `s35390a->pinfunction[i]` array is accessed without holding `pinfunction_lock` during the probe fallback loop, leading to a TOCTOU race with concurrent pinmux requests.
- [Medium] The 'clock' pin function is advertised to pinctrl consumers but is unimplemented and silently disables the pin.
- [Low] The driver calls `dmi_match()` without including `<linux/dmi.h>`.
--

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

rtc: s35390a: Add pinctrl

Allow configuration of pin functions, including:
- keeping the previous configured function
- disabling output
- waking up the system
- minute periodical interrupt (1: 50% duty, 2: 7.81 ms)

If no function has been configured, fallback to previous behaviour.

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

[Severity: High]
Will adding this hard dependency on PINCTRL silently break the build or
drop the driver for existing platforms that do not use the pin control
subsystem? Since the driver worked previously without it, should the pin
control features be optional or structured to avoid dropping the driver
entirely on existing configurations?

>  	select BITREVERSE
>  	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 4d7fecfbe38e8..80e5413289ac0 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c

[ ... ]

> +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]
Does this silently intercept the unimplemented clock function, configure the
hardware incorrectly, and then return success? If a device tree requests the
clock function, shouldn't this return an error instead of falling through
to the disable case?

[ ... ]

> +static const struct pinctrl_ops s35390a_pinctrl_ops = {
> +	.get_groups_count = s35390a_pinctrl_get_groups_count,
> +	.get_group_name = s35390a_pinctrl_get_group_name,
> +	.get_group_pins = s35390a_pinctrl_get_group_pins,
> +#if IS_ENABLED(CONFIG_OF)
> +	.dt_node_to_map = pinconf_generic_dt_node_to_map_all,
> +	.dt_free_map = pinconf_generic_dt_free_map
> +#endif
> +};

[Severity: High]
Does this cause link errors when CONFIG_OF is enabled but CONFIG_GENERIC_PINCONF
is not?

pinconf_generic_dt_node_to_map_all calls pinconf_generic_dt_node_to_map(),
which does not provide an empty inline stub in
include/linux/pinctrl/pinconf-generic.h.

Should the driver Kconfig select GENERIC_PINCONF and PINMUX to prevent this?

[ ... ]

> +	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 */

[Severity: Low]
Does the fallback logic here (or added in a later commit) use dmi_match()
without including <linux/dmi.h>? If so, could this cause an implicit
declaration build failure depending on the architecture and include order?

> +	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: High]
Is there a race condition here since pinctrl_enable() has already exposed
the pin controller to consumers?

If a concurrent consumer requests a pin and securely updates the hardware
and s35390a->pinfunction[i] under pinfunction_lock in
s35390a_pinctrl_set_mux(), couldn't this fallback loop read a stale -1
because it accesses s35390a->pinfunction[i] without holding the lock?

If it reads the stale -1, won't it invoke s35390a_pinctrl_set_mux() and
overwrite the consumer's established hardware state?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260815-rtc_s35390a_int1-v4-0-3133309c365f@posteo.de?part=5