Re: [rtc-linux] [PATCH] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user

[Andrea Scian <rnd4@dave-tech.it>]

Hi Alexandre,

On 12/06/2015 09:42, Alexandre Belloni wrote:
> On 10/06/2015 at 17:21:57 +0200, Andrea Scian wrote :
>>> I would return -EINVAL here because the result might still pass
>>> rtc_valid_tm() but be outdated.
>> At first look I agree with you, but a bit later they say:
>>
>> /* the clock can give out invalid datetime, but we cannot return
>>   * -EINVAL otherwise hwclock will refuse to set the time on bootup.
>>   */
>>
>> http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/rtc/rtc-pcf2127.c#n91
>>
>> so they don't actually return -EINVAL even if rtc_valid_tm() fails.
>> WDYT? I'm not an RTC subsystem expert, so maybe I'm missing something..
>>
> This has been copy pasted from other drivers and this is simply not
> true.

Thanks for point this out.
I'll split the patch to fix this for all PCF drivers (which have nearly 
all the same structure) and later add the OFS flag

>> If the comment above is correct, so we can't return -EINVAL, I will reset
>> the time to epoch, with something like
>>
>> rtc_time64_to_tm((time64_t)0, tm);
>>
> Doing that is worse. You really want userspace to know that the time is
> invalid instead of giving an incorrect value. This allow userspace to
> actually choose its policy when the time is invalid. For example, use
> epoch or any other later date that probabyl makes more sense for the
> product.

Most of minimal RFS I saw reset the date to what's inside /etc/timestamp 
(which is updated in runlevel 6). However, this is OT here.

>>>> @@ -144,7 +153,7 @@ static int pcf2127_rtc_ioctl(struct device *dev,
>>>>   	switch (cmd) {
>>>>   	case RTC_VL_READ:
>>>>   		if (pcf2127->voltage_low)
>>>> -			dev_info(dev, "low voltage detected, date/time is not reliable.\n");
>>>> +			dev_info(dev, "low voltage detected, check/replace battery\n");
>>> I would also print a warning about OFS here.
>>>
>> I'll do.
>> Do you think is better to add another variable inside struct pcf2127 or is
>> better to re-read the RTC registers?
>> (for the former I have also to clear the variable inside
>> pcf2127_set_datetime(), for the latter I have to issue another read in a
>> function that, at the moment, does not read anything..)
>>
> I don't really care. But since one of them is already cached, it is
> probably better to cache OFS. Maybe you could also use voltage_low as a
> bit field which would allow userspace to make the difference between a
> simple low voltage and the time loss condition.
>
>

I'll cache OFS too, in a different variable. Returning different values 
depending on OFS when querying about voltage low may mislead some 
application. Moreover I think that there's may be some cases when OFS is 
set and voltage low is not (e.g. when replacing battery with a brand new 
one).

I'll send the updated patch soon

Thanks for your comments/help,

-- 

Andrea SCIAN

DAVE Embedded Systems