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

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

[Nick Crews]

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);
+		return -EIO;
+	}
 
 	return 0;
 }
-- 
2.21.0

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

[Dmitry Torokhov]

On Wed, Sep 25, 2019 at 1:32 PM Nick Crews <ncrews@chromium.org> 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>

Reviewed-by: Dmitry Torokhov <dtor@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);
> +               return -EIO;
> +       }
>
>         return 0;
>  }
> --
> 2.21.0
>

Thanks.

-- 
Dmitry

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

[Alexandre Belloni]

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.

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

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

[Dmitry Torokhov]

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?

Thanks,
Dmitry

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

[Alexandre Belloni]

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

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

[Dmitry Torokhov]

On Wed, Oct 2, 2019 at 3:32 AM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> 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.

OK, I'll keeo this in mind then.

>
> 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?

Report it, or, in our case, we will see it in the feedback logs.
However I do agree that it is not the best option, even if we report
error to userspace I am not sure if it will handle it properly. What
userspace is supposed to do when presented with -EIO or similar?

Nick, do we know the root cause of the EC/RTC reporting invalid time?

Thanks,
Dmitry

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

[Alexandre Belloni]

On 02/10/2019 08:20:34-0700, Dmitry Torokhov wrote:
> > 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?
> 
> Report it, or, in our case, we will see it in the feedback logs.
> However I do agree that it is not the best option, even if we report
> error to userspace I am not sure if it will handle it properly. What
> userspace is supposed to do when presented with -EIO or similar?
> 

I would say probably retry a few times if it gets -EIO and simply abort
in case of -EINVAL. I'm not aware of any userspace tool behaving that
way though. They all simply abort.

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

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

[Nick Crews]

On Wed, Oct 2, 2019 at 9:20 AM Dmitry Torokhov <dtor@google.com> wrote:
>
> On Wed, Oct 2, 2019 at 3:32 AM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> >
> > 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.
>
> OK, I'll keeo this in mind then.

I will resend this using %ptRr, chromium is using 4.19 so I didn't see
that this was added.

>
> >
> > 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?
>
> Report it, or, in our case, we will see it in the feedback logs.
> However I do agree that it is not the best option, even if we report
> error to userspace I am not sure if it will handle it properly. What
> userspace is supposed to do when presented with -EIO or similar?

Yes, we will be able to see this in feedback logs, which would be valuable.

>
> Nick, do we know the root cause of the EC/RTC reporting invalid time?

No, I haven't really looked into it deeply. It's not limited to the RTC
interface though, it's a problem with the EC or EC communication
in general, as I've noticed similar occasional errors with the other EC
drivers.

>
> Thanks,
> Dmitry