Messages in this thread | | | Date | Mon, 15 Jun 2015 17:57:17 +0200 | From | Andrea Scian <> | Subject | Re: [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
| |