Re: [PATCH v3] rtc: wilco-ec: Handle reading invalid times

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

On 01/10/2019 13:42:24-0700, Dmitry Torokhov wrote:
> On Tue, Oct 1, 2019 at 12:53 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> >
> > Hi Nick,
> >
> > On 25/09/2019 14:32:09-0600, Nick Crews wrote:
> > > If the RTC HW returns an invalid time, the rtc_year_days()
> > > call would crash. This patch adds error logging in this
> > > situation, and removes the tm_yday and tm_wday calculations.
> > > These fields should not be relied upon by userspace
> > > according to man rtc, and thus we don't need to calculate
> > > them.
> > >
> > > Signed-off-by: Nick Crews <ncrews@chromium.org>
> > > ---
> > >  drivers/rtc/rtc-wilco-ec.c | 13 +++++++++----
> > >  1 file changed, 9 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c
> > > index 8ad4c4e6d557..53da355d996a 100644
> > > --- a/drivers/rtc/rtc-wilco-ec.c
> > > +++ b/drivers/rtc/rtc-wilco-ec.c
> > > @@ -110,10 +110,15 @@ static int wilco_ec_rtc_read(struct device *dev, struct rtc_time *tm)
> > >       tm->tm_mday     = rtc.day;
> > >       tm->tm_mon      = rtc.month - 1;
> > >       tm->tm_year     = rtc.year + (rtc.century * 100) - 1900;
> > > -     tm->tm_yday     = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year);
> > > -
> > > -     /* Don't compute day of week, we don't need it. */
> > > -     tm->tm_wday = -1;
> > > +     /* Ignore other tm fields, man rtc says userspace shouldn't use them. */
> > > +
> > > +     if (rtc_valid_tm(tm)) {
> > > +             dev_err(dev,
> > > +                      "Time from RTC is invalid: second=%u, minute=%u, hour=%u, day=%u, month=%u, year=%u, century=%u",
> > > +                      rtc.second, rtc.minute, rtc.hour, rtc.day, rtc.month,
> > > +                      rtc.year, rtc.century);
> >
> > Do you mind using %ptR? At this point you already filled the tm struct
> > anyway and if you print century separately, you can infer tm_year.
> 
> I do not think this is a good idea: we have just established that tm
> does not contain valid data. Does %ptR guarantee that it handles junk
> better than, let's say, rtc_year_days(), and does not crash when
> presented with garbage?
> 

It is safe to use. You can also use %ptRr if you want to ensure no
extra operations are done on the value before printing them out.

I'm still not convinced it is useful to have an error in dmesg when the
time is invalid, as long as userspace knows it is invalid. What is the
course of action for the end user when that happens?

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com