Re: [PATCH v2 4/5] rtc: s35390a: Add pinctrl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Missing `pinctrl_enable()` call after `devm_pinctrl_register_and_init()`.
- [High] Adding `depends on OF` breaks the driver on ACPI-based x86 systems.
- [High] Hardcoded array index `configs[0]` inside loop in `s35390a_pinconf_set` ignores multiple configurations.
- [High] Information leak due to uninitialized stack variable in `s35390a_rtc_alarm_irq_enable()`.
- [High] Hardware pending interrupt flag is no longer cleared, breaking subsequent alarm wakeups.
- [High] Alarm interrupts notify the PM core before the device is configured as wakeup capable, causing lost wake events.
- [Medium] Software state caching updates before hardware programming succeeds, causing permanent desynchronization on I2C failure.
--

--- Patch [4]: [PATCH v2 4/5] rtc: s35390a: Add pinctrl ---
commit 58cfab3a8ceba177a47650a06a107e0c784e6fff
Author: Markus Probst <markus.probst@posteo.de>
Subject: rtc: s35390a: Add pinctrl

Allow configuration of other output modes than wake alarm, including:
keeping the previous configured mode, disabling output, custom frequency,
minute periodical interrupt, and raw 32.768 kHz output.

> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -668,7 +668,10 @@ config RTC_DRV_RC5T619
>  
>  config RTC_DRV_S35390A
>  	tristate "Seiko Instruments S-35390A"
> +	depends on OF

[Severity: High]
Does adding the strict OF dependency here break the driver for ACPI-based x86
systems? 

The driver explicitly supports DMI quirks for such systems, and this might
prevent compilation for previously supported non-OF hardware like Synology NAS
platforms.

> 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
[ ... ]
> @@ -283,33 +309,33 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  		alm->time.tm_min, alm->time.tm_hour, alm->time.tm_mday,
>  		alm->time.tm_mon, alm->time.tm_year, alm->time.tm_wday);
>  
> -	/* disable interrupt (which deasserts the irq line) */
> -	err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
> -	if (err < 0)
> -		return err;
> +	guard(mutex)(&s35390a->mode_lock);
>  
> -	/* clear pending interrupt (in STATUS1 only), if any */
> -	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, &sts, sizeof(sts));
> +	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
>  	if (err < 0)
>  		return err;

[Severity: High]
Is it intentional to remove the read of S35390A_CMD_STATUS1 here?

By removing this read, the RTC chip's internal pending interrupt flag might 
never be cleared when a new alarm is set. Can this cause the physical WAKEUP 
pin to remain asserted, preventing the host from detecting subsequent alarm 
edges?

[ ... ]
> @@ -388,10 +445,10 @@ static int s35390a_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled
>  	struct s35390a *s35390a = dev_get_drvdata(dev);
>  	struct rtc_wkalrm alm;
>  
> -	alm.enabled = enabled;
> -
>  	if (enabled)
> -		alm.time = s35390a->tm_alarm;
> +		alm = s35390a->alarm;
> +
> +	alm.enabled = enabled;
>  
>  	return s35390a_rtc_set_alarm(dev, &alm);
>  }

[Severity: High]
If enabled is 0, is the stack-allocated alm struct left uninitialized except
for the enabled field?

If RTC_AIE_OFF is called, this uninitialized memory is passed to
s35390a_rtc_set_alarm() where it is copied into the permanent device state
s35390a->alarm. A subsequent RTC_ALM_READ could then copy this uninitialized
kernel stack memory to userspace via s35390a_rtc_read_alarm(). Does this
create an information leak?

[ ... ]
> +static int s35390a_update_mode(struct s35390a *s35390a, int pin, int mode, int freq)
> +{
[ ... ]
> +	if (mode == s35390a->mode[pin] && freq == s35390a->freq[pin])
> +		return 0;
> +
> +	s35390a->mode[pin] = mode;
> +	s35390a->freq[pin] = freq;
> +
> +	if (mode == S35390A_MODE_IGNORE)
> +		return 0;
> +
> +	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &buf, 1);
> +	if (err < 0) {
> +		dev_err(&s35390a->client[0]->dev, "error reading status\n");
> +		return err;
> +	}

[Severity: Medium]
Is it safe to update the software state (s35390a->mode and freq) before
confirming the hardware configuration via I2C succeeds?

If s35390a_get_reg() or a later I2C operation fails, the driver state will
incorrectly reflect the new mode. Future attempts to set this same mode would
then be skipped by the check above, permanently desynchronizing the software
state from the hardware until a different mode is applied. Should the state
update happen at the end of the function after hardware success?

[ ... ]
> +static int s35390a_pinconf_set(struct pinctrl_dev *pctldev,
> +			       unsigned int pin, unsigned long *configs,
> +			       unsigned int num_configs)
> +{
[ ... ]
> +	for (i = 0; i < num_configs; i++) {
> +		param = pinconf_to_config_param(configs[0]);
> +		param_val = pinconf_to_config_argument(configs[0]);

[Severity: High]
Does this loop intend to use configs[i] instead of configs[0]?

As written, when multiple pin configurations are defined in the device tree,
the loop will repeatedly process the first configuration while ignoring all
subsequent properties.

[ ... ]
> @@ -477,7 +762,10 @@ static int s35390a_probe(struct i2c_client *client)
[ ... ]
> +	err = devm_pinctrl_register_and_init(dev, &s35390a_pinctrl_desc, s35390a,
> +					     &s35390a->pinctrl);
> +	if (err)
> +		return err;

[Severity: High]
Is a call to pinctrl_enable() missing here?

After devm_pinctrl_register_and_init(), the pinctrl device remains in the
initialization state. Without pinctrl_enable(), it may never be fully
registered or exposed to the pinctrl core, rendering the feature
non-functional.

> +
> +	/* If no pinctrl config is defined in DT, fallback to previous behaviour */
> +	if (!s35390a->mode_init) {
> +		err = s35390a_update_mode(s35390a, 1, S35390A_MODE_WAKEUP, 0);
> +		if (err)
> +			return err;
> +	}
> +
>  	err_read = s35390a_read_status(s35390a, &status1);
>  	if (err_read < 0) {
>  		dev_err(dev, "error resetting chip\n");
> @@ -507,34 +822,8 @@ static int s35390a_probe(struct i2c_client *client)
>  	else
>  		s35390a->twentyfourhour = 0;
>  
>  	device_set_wakeup_capable(dev, 1);

[Severity: High]
Can s35390a_update_mode() trigger rtc_update_irq() prematurely in the probe
sequence?

If an alarm fired while the system was off, s35390a_update_mode() calls
rtc_update_irq(), which relies on pm_stay_awake(). However, since
device_set_wakeup_capable(dev, 1) hasn't been called yet, the PM core may
drop the wakeup notification. Additionally, the queued IRQ work might read
the time before s35390a->twentyfourhour is initialized.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801-rtc_s35390a_int1-v2-0-f10c99ad1d6c@posteo.de?part=4