Re: [PATCH v4 4/6] rtc: s35390a: force 24-hour mode

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

New issues:
- [High] Forcing the RTC into 24-hour mode without converting existing BCD time registers corrupts system time.

Pre-existing issues:
- [High] Unconditional setting of the 12-hour mode PM bit (0x40) breaks alarms for PM hours in 24-hour mode.
- [High] Destructive read of hardware status during probe permanently loses power-failure state, causing corrupted time reads.
--

--- Patch [4]: [PATCH v4 4/6] rtc: s35390a: force 24-hour mode ---
commit 5dfeb8634985eb7940c40692a8b6802f70166610
Author: Markus Probst <[email protected]>

rtc: s35390a: force 24-hour mode
    
Instead of trying to remember the current 12/24-hour mode, which could
result in time corruption if out of sync, we force the use of the 24-hour
mode. Set to 24-hour mode if in 12-hour mode on startup and assume
24-hour mode.

> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> index 575bb256eb258..4d7fecfbe38e8 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c
> @@ -325,8 +299,7 @@ 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] = bin2bcd(alm->time.tm_hour) | 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 is a pre-existing issue, but does this unconditional bitwise OR break
alarms for PM hours in 24-hour mode?

In drivers/rtc/rtc-s35390a.c:s35390a_rtc_set_alarm(), the code
unconditionally sets bit 6 (|= 0x40) for PM hours. Since the driver now
forces the RTC into 24-hour mode, the current time is stored in purely
24-hour BCD format without bit 6 set.

Because the alarm register is written with bit 6 set for PM hours, the
hardware time and alarm registers will never match, causing PM alarms to
silently fail.

> [ ... ]
> @@ -503,10 +475,13 @@ static int s35390a_probe(struct i2c_client *client)
>  		return err_read;
>  	}

[Severity: High]
This is a pre-existing issue, but does reading the status during probe
permanently lose the power-failure state?

Earlier in drivers/rtc/rtc-s35390a.c:s35390a_probe(), the driver calls
s35390a_read_status() to check the chip state. This hardware read
automatically clears the Power-On Clear (POC) and Battery Low Voltage
(BLD) flags.

If these flags were set (returning 1), the probe function seems to ignore
the return value and proceed without resetting the time. Because the flags
are irrevocably cleared, subsequent calls to s35390a_rtc_read_time() will
see a clean status and return invalid time data instead of reporting an
error.

> -	if (status1 & S35390A_FLAG_24H)
> -		s35390a->twentyfourhour = 1;
> -	else
> -		s35390a->twentyfourhour = 0;
> +	if (!(status1 & S35390A_FLAG_24H)) {
> +		status1 |= S35390A_FLAG_24H;
> +
> +		err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, &status1, sizeof(status1));
> +		if (err < 0)
> +			return dev_err_probe(dev, err, "setting 24-hour mode failed\n");
> +	}

[Severity: High]
Could forcing the hardware into 24-hour mode without converting the
existing BCD time registers corrupt system time?

If the RTC was previously running in 12-hour mode, PM times will have
bit 6 set (e.g., 2:00 PM is 0x42). When the mode is flipped to 24-hour
mode on boot in drivers/rtc/rtc-s35390a.c:s35390a_probe(), the driver
masks bit 6 during read.

This would incorrectly interpret the PM time as AM (e.g., 2:00 AM) instead
of properly converting the time registers during the transition.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4