Re: [PATCH 1/2] rtc: Add flag for dynamic hctosys control

[Akashdeep Kaur <a-kaur@ti.com>]

On 21/10/26 10:07, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] rtc: checking time_is_valid only in rtc_hctosys() exposes invalid RTC time
> --
> 
> commit 378dbc5f51408e7a59e18fa3e61590784e92a232
> Author: Akashdeep Kaur <a-kaur@ti.com>
> 
> rtc: Add flag for dynamic hctosys control
> 
> This commit adds a time_is_valid flag to struct rtc_device and checks it in
> rtc_hctosys() so drivers can prevent setting the system clock from an
> uninitialized or power-lost hardware clock at boot.
> 
>> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
>> index 01ba04028f1f1..cec27c476545e 100644
>> --- a/drivers/rtc/class.c
>> +++ b/drivers/rtc/class.c
>> @@ -63,6 +63,18 @@ static void rtc_hctosys(struct rtc_device *rtc)
>>   		.tv_nsec = NSEC_PER_SEC >> 1,
>>   	};
>>   
>> +	/*
>> +	 * If the RTC does not have valid time, do not set system time at boot.
>> +	 * This can happen when RTC loses power or has never been initialized.
>> +	 * PM resume compensation will still work via rtc_resume().
>> +	 */
>> +	if (!rtc->time_is_valid) {
>> +		dev_info(rtc->dev.parent,
>> +			 "hctosys: RTC time not valid, skipping\n");
>> +		err = -EINVAL;
>> +		goto err_read;
>> +	}
>> +
>>   	err = rtc_read_time(rtc, &tm);
>>   	if (err) {
>>   		dev_err(rtc->dev.parent,
> 
> [Severity: Medium]
> When a driver sets rtc->time_is_valid to false instead of returning an error
> from ops->read_time(), checking rtc->time_is_valid only in rtc_hctosys()
> leaves rtc_read_time() in drivers/rtc/interface.c returning 0 with bogus
> 1970-01-01 time to all other callers:
> 
> drivers/rtc/interface.c:rtc_read_time() {
> 	err = mutex_lock_interruptible(&rtc->ops_lock);
> 	if (err)
> 		return err;
> 
> 	err = __rtc_read_time(rtc, tm);
> 	mutex_unlock(&rtc->ops_lock);
> 	...
> 	return err;
> }
> 
> drivers/rtc/dev.c:rtc_dev_ioctl() {
> 	case RTC_RD_TIME:
> 		mutex_unlock(&rtc->ops_lock);
> 
> 		err = rtc_read_time(rtc, &tm);
> 		if (err < 0)
> 			return err;
> }
> 
> On boot before rtc_set_time() is called, any userspace read via
> rtc_dev_ioctl() -> rtc_read_time() (such as hwclock --hctosys, timedatectl,
> or chronyd), sysfs reads in date_show(), time_show(), and since_epoch_show()
> in drivers/rtc/sysfs.c, procfs reads in rtc_proc_show(), and alarm
> initialization in __devm_rtc_register_device() -> __rtc_read_alarm() ->
> rtc_initialize_alarm() -> rtc_read_time() will all succeed and consume the
> uninitialized 1970-01-01 timestamp.
> 
> Additionally, rtc_set_time() in drivers/rtc/interface.c never updates
> rtc->time_is_valid to true once a valid time is written.
> 
> Could drivers return -EINVAL from ops->read_time() until ops->set_time() is
> called (which already causes rtc_hctosys() to skip setting system time via
> rtc_read_time()), or should __rtc_read_time() and rtc_set_time() check and
> update this flag so all callers see consistent validity reporting?
> 
> [ ... ]
> 
Thank you for the review. The time_is_valid flag is specifically to 
control boot-time synchronization, not to block RTC reads.

The RTC hardware is readable - userspace can read it via hwclock/sysfs. 
Reading "1970-01-01" is acceptable as it reflects actual hardware state. 
The problem we're solving is specific: preventing boot-time sync from 
overwriting correct system time (from bootloader/NTP) with uninitialized 
RTC value.

Returning -EINVAL from __rtc_read_time() would block all reads, which is 
not the intent. The RTC is readable; we just shouldn't use it to set 
system time on this particular boot.

Regards,
Akashdeep Kaur