From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BD03AC352AA for ; Wed, 2 Oct 2019 10:32:41 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9E9FC218DE for ; Wed, 2 Oct 2019 10:32:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727936AbfJBKcl (ORCPT ); Wed, 2 Oct 2019 06:32:41 -0400 Received: from relay4-d.mail.gandi.net ([217.70.183.196]:58703 "EHLO relay4-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726109AbfJBKck (ORCPT ); Wed, 2 Oct 2019 06:32:40 -0400 X-Originating-IP: 86.207.98.53 Received: from localhost (aclermont-ferrand-651-1-259-53.w86-207.abo.wanadoo.fr [86.207.98.53]) (Authenticated sender: alexandre.belloni@bootlin.com) by relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 9674FE0007; Wed, 2 Oct 2019 10:32:37 +0000 (UTC) Date: Wed, 2 Oct 2019 12:32:36 +0200 From: Alexandre Belloni To: Dmitry Torokhov Cc: Nick Crews , Alessandro Zummo , linux-rtc@vger.kernel.org, lkml , Pavel Machek , enric.balletbo@collabora.com, Benson Leung , dlaurie@chromium.org, Daniel Kurtz Subject: Re: [PATCH v3] rtc: wilco-ec: Handle reading invalid times Message-ID: <20191002103236.GM4106@piout.net> References: <20190925203209.79941-1-ncrews@chromium.org> <20191001195342.GH4106@piout.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.12.1 (2019-06-15) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/10/2019 13:42:24-0700, Dmitry Torokhov wrote: > On Tue, Oct 1, 2019 at 12:53 PM Alexandre Belloni > 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 > > > --- > > > 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