lkml.org 
[lkml]   [2015]   [Jun]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [rtc-linux] [PATCH] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user

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



\
 
 \ /
  Last update: 2015-06-15 18:21    [W:0.086 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site