* [PATCH v3] rtc: wilco-ec: Handle reading invalid times @ 2019-09-25 20:32 Nick Crews 2019-09-25 20:51 ` Dmitry Torokhov 2019-10-01 19:53 ` Alexandre Belloni 0 siblings, 2 replies; 8+ messages in thread From: Nick Crews @ 2019-09-25 20:32 UTC (permalink / raw) To: Alexandre Belloni, Alessandro Zummo Cc: linux-rtc, linux-kernel, Pavel Machek, enric.balletbo, bleung, dlaurie, djkurtz, dtor, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] rtc: wilco-ec: Handle reading invalid times 2019-09-25 20:32 [PATCH v3] rtc: wilco-ec: Handle reading invalid times Nick Crews @ 2019-09-25 20:51 ` Dmitry Torokhov 2019-10-01 19:53 ` Alexandre Belloni 1 sibling, 0 replies; 8+ messages in thread From: Dmitry Torokhov @ 2019-09-25 20:51 UTC (permalink / raw) To: Nick Crews Cc: Alexandre Belloni, Alessandro Zummo, linux-rtc, lkml, Pavel Machek, enric.balletbo, Benson Leung, dlaurie, Daniel Kurtz 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] rtc: wilco-ec: Handle reading invalid times 2019-09-25 20:32 [PATCH v3] rtc: wilco-ec: Handle reading invalid times Nick Crews 2019-09-25 20:51 ` Dmitry Torokhov @ 2019-10-01 19:53 ` Alexandre Belloni 2019-10-01 20:42 ` Dmitry Torokhov 1 sibling, 1 reply; 8+ messages in thread From: Alexandre Belloni @ 2019-10-01 19:53 UTC (permalink / raw) To: Nick Crews Cc: Alessandro Zummo, linux-rtc, linux-kernel, Pavel Machek, enric.balletbo, bleung, dlaurie, djkurtz, dtor 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] rtc: wilco-ec: Handle reading invalid times 2019-10-01 19:53 ` Alexandre Belloni @ 2019-10-01 20:42 ` Dmitry Torokhov 2019-10-02 10:32 ` Alexandre Belloni 0 siblings, 1 reply; 8+ messages in thread From: Dmitry Torokhov @ 2019-10-01 20:42 UTC (permalink / raw) To: Alexandre Belloni Cc: Nick Crews, Alessandro Zummo, linux-rtc, lkml, Pavel Machek, enric.balletbo, Benson Leung, dlaurie, Daniel Kurtz 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] rtc: wilco-ec: Handle reading invalid times 2019-10-01 20:42 ` Dmitry Torokhov @ 2019-10-02 10:32 ` Alexandre Belloni 2019-10-02 15:20 ` Dmitry Torokhov 0 siblings, 1 reply; 8+ messages in thread From: Alexandre Belloni @ 2019-10-02 10:32 UTC (permalink / raw) To: Dmitry Torokhov Cc: Nick Crews, Alessandro Zummo, linux-rtc, lkml, Pavel Machek, enric.balletbo, Benson Leung, dlaurie, Daniel Kurtz 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] rtc: wilco-ec: Handle reading invalid times 2019-10-02 10:32 ` Alexandre Belloni @ 2019-10-02 15:20 ` Dmitry Torokhov 2019-10-02 15:34 ` Alexandre Belloni 2019-10-03 20:37 ` Nick Crews 0 siblings, 2 replies; 8+ messages in thread From: Dmitry Torokhov @ 2019-10-02 15:20 UTC (permalink / raw) To: Alexandre Belloni Cc: Nick Crews, Alessandro Zummo, linux-rtc, lkml, Pavel Machek, enric.balletbo, Benson Leung, dlaurie, Daniel Kurtz 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] rtc: wilco-ec: Handle reading invalid times 2019-10-02 15:20 ` Dmitry Torokhov @ 2019-10-02 15:34 ` Alexandre Belloni 2019-10-03 20:37 ` Nick Crews 1 sibling, 0 replies; 8+ messages in thread From: Alexandre Belloni @ 2019-10-02 15:34 UTC (permalink / raw) To: Dmitry Torokhov Cc: Nick Crews, Alessandro Zummo, linux-rtc, lkml, Pavel Machek, enric.balletbo, Benson Leung, dlaurie, Daniel Kurtz 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] rtc: wilco-ec: Handle reading invalid times 2019-10-02 15:20 ` Dmitry Torokhov 2019-10-02 15:34 ` Alexandre Belloni @ 2019-10-03 20:37 ` Nick Crews 1 sibling, 0 replies; 8+ messages in thread From: Nick Crews @ 2019-10-03 20:37 UTC (permalink / raw) To: Dmitry Torokhov Cc: Alexandre Belloni, Alessandro Zummo, linux-rtc, lkml, Pavel Machek, Enric Balletbo i Serra, Benson Leung, Duncan Laurie, Daniel Kurtz 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-10-03 20:37 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-25 20:32 [PATCH v3] rtc: wilco-ec: Handle reading invalid times Nick Crews 2019-09-25 20:51 ` Dmitry Torokhov 2019-10-01 19:53 ` Alexandre Belloni 2019-10-01 20:42 ` Dmitry Torokhov 2019-10-02 10:32 ` Alexandre Belloni 2019-10-02 15:20 ` Dmitry Torokhov 2019-10-02 15:34 ` Alexandre Belloni 2019-10-03 20:37 ` Nick Crews
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).