Re: [PATCH v8 3/6] rtc: s35390a: Fix alarm not disabling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Adding `alarm_irq_enable` exposes a preexisting flaw where `set_alarm(enabled=0)` drops the alarm time, causing subsequent `RTC_AIE_ON` ioctls to enable the alarm with an incorrect/stale time.
--

commit 9902d89ba2e478f9c2ca532d42600946943b66e7
Author: Markus Probst <markus.probst@posteo.de>

rtc: s35390a: Fix alarm not disabling

This commit implements the alarm_irq_enable callback to fix an issue where
the alarm was not disabling.

> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> index 4cfe7034c5165..575bb256eb258 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c
[ ... ]
> @@ -270,6 +270,24 @@ static int s35390a_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  	return 0;
>  }
>  
> +static int s35390a_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> +{
> +	struct s35390a *s35390a = dev_get_drvdata(dev);
> +	u8 sts;
> +	int err;
> +
> +	if (enabled)
> +		sts = S35390A_INT2_MODE_ALARM;
> +	else
> +		sts = S35390A_INT2_MODE_NOINTR;
> +
> +	err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));

[Severity: High]
This is a pre-existing issue, but does enabling the alarm here (e.g., via
the RTC_AIE_ON ioctl) expose a flaw where s35390a_rtc_set_alarm() drops the
alarm time?

When a user issues the RTC_ALM_SET ioctl, it inherently calls
s35390a_rtc_set_alarm() with enabled = 0. The driver sets STATUS2 to NOINTR
mode:

drivers/rtc/rtc-s35390a.c:s35390a_rtc_set_alarm() {
    ...
    if (alm->enabled)
        sts = S35390A_INT2_MODE_ALARM;
    else
        sts = S35390A_INT2_MODE_NOINTR;

    /* set interrupt mode*/
    err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
    ...
}

The driver then attempts to write the time to INT2_REG1:

drivers/rtc/rtc-s35390a.c:s35390a_rtc_set_alarm() {
    ...
    err = s35390a_set_reg(s35390a, S35390A_CMD_INT2_REG1, buf,
                          sizeof(buf));
    ...
}

However, as documented by the driver's own comment in s35390a_rtc_read_alarm(),
the alarm time register is completely inaccessible when the alarm is not
enabled. Thus, the time update is silently dropped by the hardware.

When the user subsequently calls RTC_AIE_ON, this newly added
alarm_irq_enable callback enables the alarm interrupt using the incorrect or
stale time previously residing in the registers.

Note: This issue is not resolved by follow-up patches in this series. In the
final patch (commit "rtc: s35390a: Add synology quirk"),
s35390a_rtc_set_alarm() still leaves the interrupt mode disabled when
alm->enabled == 0 before performing the time register writes, preserving the
issue.

> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}

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