Re: [PATCH v8 4/6] rtc: s35390a: Read 24-hour mode on access

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Alarm PM bit is unconditionally set for PM hours, breaking alarms in 24-hour mode.
- [High] Multiple functions read the STATUS1 register but fail to handle the POC (Power-On Clear) and BLD (Battery Low Detect) flags correctly, permanently masking power-loss events.
--

commit b899282922e9e75f3f9690275fbd3d093dfa12e5
Author: Markus Probst <markus.probst@posteo.de>
Subject: rtc: s35390a: Read 24-hour mode on access

This commit modifies the driver to dynamically read the 24-hour mode status
from the chip on access, rather than caching it during probe. This prevents
the mode from going out of sync if the chip is reset while in 12-hour mode.

> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> index 575bb256eb258..12ed504db94d8 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c

[ ... ]

> @@ -292,7 +297,7 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct s35390a *s35390a = i2c_get_clientdata(client);
> -	u8 buf[3], sts = 0;
> +	u8 buf[3], status1, status2 = 0;
>  	int err, i;

[ ... ]

>  	/* 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_STATUS1, &status1, sizeof(status1));
>  	if (err < 0)
>  		return err;

[Severity: High]
This is a pre-existing issue, but does reading S35390A_CMD_STATUS1 here
silently clear the POC (Power-On Clear) and BLD (Battery Low Detect) flags
in the hardware without handling them?

Since these flags are automatically cleared by the hardware upon reading
STATUS1, it appears that s35390a_probe(), s35390a_rtc_set_alarm(), and
s35390a_rtc_ioctl() all inadvertently erase the power-loss state. By bypassing
the proper power-loss handling, will subsequent reads of the time unknowingly
trust and present an invalid or garbage RTC time to the system after a battery
failure?

>  	if (alm->enabled)
> -		sts = S35390A_INT2_MODE_ALARM;
> +		status2 = S35390A_INT2_MODE_ALARM;
>  	else
> -		sts = S35390A_INT2_MODE_NOINTR;
> +		status2 = S35390A_INT2_MODE_NOINTR;
>  
>  	/* set interrupt mode*/
> -	err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
> +	err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &status2, sizeof(status2));
>  	if (err < 0)
>  		return err;
>  
> @@ -325,8 +330,8 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  	else
>  		buf[S35390A_ALRM_BYTE_WDAY] = 0;
>  
> -	buf[S35390A_ALRM_BYTE_HOURS] = s35390a_hr2reg(s35390a,
> -			alm->time.tm_hour) | 0x80;
> +	buf[S35390A_ALRM_BYTE_HOURS] = s35390a_hr2reg(alm->time.tm_hour,
> +						      status1 & S35390A_FLAG_24H) | 0x80;
>  	buf[S35390A_ALRM_BYTE_MINS] = bin2bcd(alm->time.tm_min) | 0x80;
>  
>  	if (alm->time.tm_hour >= 12)
>  		buf[S35390A_ALRM_BYTE_HOURS] |= 0x40;

[Severity: High]
This isn't a bug introduced by this patch, but does unconditionally setting
the 0x40 bit for PM hours here break alarms in 24-hour mode?

In 24-hour mode, bit 6 is not used for PM, so setting it writes an invalid
BCD value that the time register will never match. Will this prevent the
alarm from ever firing for times >= 12:00 when the RTC is configured in
24-hour mode?

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