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

[Andrea Scian]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/