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

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

New issues:
- [Medium] Incomplete migration from 12-hour to 24-hour mode silently breaks existing hardware wake alarms.

Pre-existing issues:
- [High] Forcing 24-hour mode permanently breaks all alarms for PM hours due to legacy AM/PM bit manipulation.
--

--- Patch [4]: [PATCH v5 4/6] rtc: s35390a: force 24-hour mode ---
commit 8f484adee5ad67584475e4c03516c2ee9307afc4
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..b34a3f7e8476c 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 forcing 24-hour mode permanently break 
all alarms for PM hours here?

In s35390a_rtc_set_alarm(), the code unconditionally sets bit 6 (|= 0x40) of 
the hours register when tm_hour >= 12. While this was correct for 12-hour 
mode to indicate PM, in 24-hour mode the time register uses raw 24-hour BCD 
without a PM bit (e.g., 13:00 is 0x13). 

Writing 0x13 | 0x40 yields 0x53. The hardware comparator will attempt to match
the current time (0x13) with the alarm (0x53) and fail, meaning hardware wake 
alarms for PM hours will never trigger. Since 24-hour mode is now forced 
universally, will this affect all users?

[ ... ]
> @@ -503,10 +475,31 @@ static int s35390a_probe(struct i2c_client *client)
>  		return err_read;
>  	}
>  
> -	if (status1 & S35390A_FLAG_24H)
> -		s35390a->twentyfourhour = 1;
> -	else
> -		s35390a->twentyfourhour = 0;
> +	if (!(status1 & S35390A_FLAG_24H) && err_read != 1) {
> +		unsigned int hour;
> +
> +		status1 |= S35390A_FLAG_24H;
> +
> +		err = s35390a_get_reg(s35390a, S35390A_CMD_TIME1, time, sizeof(time));
> +		if (err < 0)
> +			return dev_err_probe(dev, err, "reading 12-hour time failed\n");
> +
> +		/* converting to 24-hour time */
> +		time[S35390A_BYTE_HOURS] = bitrev8(time[S35390A_BYTE_HOURS]);

[Severity: Medium]
Does this migration from 12-hour to 24-hour mode miss the hardware alarm 
registers?

During s35390a_probe(), the driver successfully converts the main time register
(S35390A_CMD_TIME1) to 24-hour format. However, it seems to omit reading, 
converting, and writing back the alarm registers (S35390A_CMD_INT1_REG1 and 
S35390A_CMD_INT2_REG1).

If a system reboots with an active PM wake alarm set by an older kernel (in 
12-hour format), won't the alarm register remain in 12-hour format while the 
RTC switches to 24-hour mode? This could cause the hardware comparator to 
silently fail to match the time and wake the system.

> +		hour = bcd2bin(time[S35390A_BYTE_HOURS] & 0x3f);
> +		if (time[S35390A_BYTE_HOURS] & 0x40)
> +			hour += 12;

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