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=-3.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no 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 0E44FC46466 for ; Sun, 4 Oct 2020 01:44:32 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B749F2067D for ; Sun, 4 Oct 2020 01:44:31 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=gmx.net header.i=@gmx.net header.b="lkVvKEah" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726277AbgJDBoa (ORCPT ); Sat, 3 Oct 2020 21:44:30 -0400 Received: from mout.gmx.net ([212.227.15.15]:57833 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726114AbgJDBoa (ORCPT ); Sat, 3 Oct 2020 21:44:30 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=badeba3b8450; t=1601775810; bh=uk/zgEMcmh53hyYJsD9rVSTVCXFi5qhVJPo/Njr0ULU=; h=X-UI-Sender-Class:Date:From:To:Cc:Subject:References:In-Reply-To; b=lkVvKEahe14N+6mVJ1eaYHHPhjRQiG9auUhIv7H8IJiWALY3R/rkdNiUDO8OJ6LBr 7BOkd2Kd5l4W12pGgsTC+Swuh6QxWpeli8OrqS2r/9xA44LRuFXI/9eDm9sUs52+sm bfezvwaCEoVlEyf6S5y3lpMUC0323GBJmP1YKWAE= X-UI-Sender-Class: 01bb95c1-4bf8-414a-932a-4f6e2808ef9c Received: from longitude ([5.146.195.151]) by mail.gmx.com (mrgmx004 [212.227.17.190]) with ESMTPSA (Nemesis) id 1MIdiZ-1kCV8e3OGD-00Efw6; Sun, 04 Oct 2020 03:43:29 +0200 Date: Sun, 4 Oct 2020 03:43:23 +0200 From: Jonathan =?utf-8?Q?Neusch=C3=A4fer?= To: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= Cc: Jonathan =?utf-8?Q?Neusch=C3=A4fer?= , linux-kernel@vger.kernel.org, Alexandre Belloni , Heiko Stuebner , linux-pwm@vger.kernel.org, Linus Walleij , Thierry Reding , Fabio Estevam , linux-rtc@vger.kernel.org, Arnd Bergmann , Mauro Carvalho Chehab , Sam Ravnborg , Daniel Palmer , Andy Shevchenko , Andreas Kemnade , NXP Linux Team , devicetree@vger.kernel.org, Stephan Gerhold , allen , Sascha Hauer , Lubomir Rintel , Rob Herring , Lee Jones , linux-arm-kernel@lists.infradead.org, Alessandro Zummo , Mark Brown , Pengutronix Kernel Team , Heiko Stuebner , Josua Mayer , Shawn Guo , "David S. Miller" Subject: Re: [PATCH v3 5/7] rtc: New driver for RTC in Netronix embedded controller Message-ID: <20201004014323.GD500800@latitude> References: <20200924192455.2484005-1-j.neuschaefer@gmx.net> <20200924192455.2484005-6-j.neuschaefer@gmx.net> <20200925054424.snlr3lggnsv575wu@pengutronix.de> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="n2Pv11Ogg/Ox8ay5" Content-Disposition: inline In-Reply-To: <20200925054424.snlr3lggnsv575wu@pengutronix.de> X-Provags-ID: V03:K1:3QmB/hlTSK7COVsSvQUbFXyPQLlzF/h6TMiid01Z4Clm5Arc98v uler7ylH8j9QBxD0tykisMycm+wD9UERrrd+ewA2/OjwuUspDeo2xtT/2Qnkl8U5/H4az8y 4tGKFKCvQ7SDiOJTQPqyHUPXztu5tbDBN2qCAnWjwF5YxsmxGkAAaxXhyN3ZVDti5n96UBi 6gO033n3MxkAvTUF1Iyzg== X-UI-Out-Filterresults: notjunk:1;V03:K0:H2fI1p3h7x0=:BARqSUnfP4EPxkdAyzJvu5 /BptNpO1FFEQ3z3DD/a6KohfY+3CLL6GdWbNRyd6TrTIvd68+OTtOxN0IVBuTYBKlxJwaQ8Wt gzKLRlZjM7kUKwnrsqpmUJYhr/InP3V2a72YN5XImyB3yUNDL7WXwsEDabU4EZL1+I+IiRYYa NNZgMf6DPftbNeP6iKtNsi/+vusWOrNB3FhY78TF0UlOf3FuPF3/KovQHlJfuOD37XGxldg3p Q+u8MacnMYuBL6sF1RmEoEpPD76rGD1Y3CXXdEO7tCTqnx7C7tGiVdZXGyyognnzD827MyEXU b1frg2Nn07i1ePForHRLRZzVROLm7hUsqoLWNpobmNqSay/xDFp6+xcrsSc8jKxPoPnoxcl6Y 5RjtK3DPXX7y6OltqdaFh/TBMCizXFkaFQeSh1Fy+a/miTSSEYMfsTDW/oqT90SkaC3eWCjyM X6cCvKvHqycnPRNN3uTqSaOgHAvqYQMxUhQHgww6KjtmXTUrBArbqbCjeEObFQHtbpW1/RGrK ZN63KHHbGO31mrtIMkjgNmyxZ7L8n0ARxcBNTDvc6NaS/TOyWVZzdl9vFJmSEi9bmIl5C3nD4 6H4pcW82JJ16WrlufWd3Sgy2R4F8ipDvWKchqDiagKHlBtv8O6WZAm/eA3Tj+iB11+FS8tPrn YlUjvsPtwjgXx03PK5r0GZ5Amx7Tr++VQHK2EIIkeQuRgtHhsjZWexbt11gOZ5/CgSniOHyni TgLZZ/Mn6OrDvz9cychA1SkaP+caKyI1gw2L++7dD6PVlfu2mxPuVdAo0Txyv9kCwBl1yeDRL zlikNsvv2tqERa3sn+C4y82HVfRLAQxaE7gJYSBi27foIJbQayOekgBskt+TxwCYN1yiEuzB4 uC4/BWb68qCR8KcH8nuc4vnD8bxDc1yh+WAw5xkWYCCXeuFHrcYVZDg8b4j7JlGQHhvyRnlpD 9icHQe1LmAph3YuJ4mvHQ8Z9I0IDvErkMU6TfPdryqTTnjovTGjdYrQqM2V7ziDy9W3zpP2C5 19GCHCZVjPvA00/++eoR2fg7OGGCzyHTnv72g9mNyhAB9/kFrZZ03piPpQz1TB8B7s9/8Y1D6 xR7iVeJXa6RSupP2lJ5hpMgHuPJ3Swm1j25cP0jS0gSY82mhy06uRwgz3QNwRrhp7J6oVzJSv 3KHnV5sze8908B9gVk3QW0975cwmRxmWr7DW9vrAlxWuP+uNAqR0BCOXDOX+ovsQZrGLXwkY4 6MjryYJHzKT9VnFRpG2yIaRbLukcSBHkXKyvGaA== Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --n2Pv11Ogg/Ox8ay5 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Sep 25, 2020 at 07:44:24AM +0200, Uwe Kleine-K=C3=B6nig wrote: > Hello Jonathan, >=20 > On Thu, Sep 24, 2020 at 09:24:53PM +0200, Jonathan Neusch=C3=A4fer wrote: > > +#define NTXEC_REG_WRITE_YEAR 0x10 > > +#define NTXEC_REG_WRITE_MONTH 0x11 > > +#define NTXEC_REG_WRITE_DAY 0x12 > > +#define NTXEC_REG_WRITE_HOUR 0x13 > > +#define NTXEC_REG_WRITE_MINUTE 0x14 > > +#define NTXEC_REG_WRITE_SECOND 0x15 > > + > > +#define NTXEC_REG_READ_YM 0x20 > > +#define NTXEC_REG_READ_DH 0x21 > > +#define NTXEC_REG_READ_MS 0x23 >=20 > Is this an official naming? I think at least ..._MS is a poor name. > Maybe consider ..._MINSEC instead and make the other two names a bit long= er > for consistency? It's inofficial (the vendor kernel uses 0x10 etc. directly). I'll pick longer names. > > +static int ntxec_read_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct ntxec_rtc *rtc =3D dev_get_drvdata(dev); > > + unsigned int value; > > + int res; > > + > > + res =3D regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value); > > + if (res < 0) > > + return res; > > + > > + tm->tm_year =3D (value >> 8) + 100; > > + tm->tm_mon =3D (value & 0xff) - 1; > > + > > + res =3D regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value); > > + if (res < 0) > > + return res; > > + > > + tm->tm_mday =3D value >> 8; > > + tm->tm_hour =3D value & 0xff; > > + > > + res =3D regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value); > > + if (res < 0) > > + return res; > > + > > + tm->tm_min =3D value >> 8; > > + tm->tm_sec =3D value & 0xff; > > + > > + return 0; > > +} > > + > > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct ntxec_rtc *rtc =3D dev_get_drvdata(dev); > > + int res =3D 0; > > + > > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg= 8(tm->tm_year - 100)); > > + if (res) > > + return res; > > + > > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_re= g8(tm->tm_mon + 1)); > > + if (res) > > + return res; > > + > > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8= (tm->tm_mday)); > > + if (res) > > + return res; > > + > > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg= 8(tm->tm_hour)); > > + if (res) > > + return res; > > + > > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_r= eg8(tm->tm_min)); > > + if (res) > > + return res; > > + > > + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_re= g8(tm->tm_sec)); >=20 > I wonder: Is this racy? If you write minute, does the seconds reset to > zero or something like that? Or can it happen, that after writing the > minute register and before writing the second register the seconds > overflow and you end up with the time set to a minute later than > intended? If so it might be worth to set the seconds to 0 at the start > of the function (with an explaining comment). The setting the minutes does not reset the seconds, so I think this race condition is possible. I'll add the workaround. > .read_time has a similar race. What happens if minutes overflow between > reading NTXEC_REG_READ_DH and NTXEC_REG_READ_MS? Yes, we get read tearing in that case. It could even propagate all the way to the year/month field, for example when the following time rolls over: A | B | C 2020-10-31 23:59:59 2020-11-01 00:00:00 - If the increment happens after reading C, we get 2020-10-31 23:59= :59 - If the increment happens between reading B and C, we get 2020-10-31 23:00= :00 - If the increment happens between reading A and B, we get 2020-10-01 00:00= :00 - If the increment happens before reading A, we get 2020-11-01 00:00= :00 =2E.. both of which are far from correct. To mitigate this issue, I think something like the following is needed: - Read year/month - Read day/hour - Read minute/second - Read day/hour, compare with previously read value, restart on mismatch - Read year/month, compare with previously read value, restart on mismatch The order of the last two steps doesn't matter, as far as I can see, but if I remove one of them, I can't catch all cases of read tearing. > > +static struct platform_driver ntxec_rtc_driver =3D { > > + .driver =3D { > > + .name =3D "ntxec-rtc", > > + }, > > + .probe =3D ntxec_rtc_probe, >=20 > No .remove function? I don't think it would serve a purpose in this driver. There are no device-specific resources to release (no clocks to unprepare, for example). Thanks, Jonathan Neusch=C3=A4fer --n2Pv11Ogg/Ox8ay5 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEvHAHGBBjQPVy+qvDCDBEmo7zX9sFAl95KK0ACgkQCDBEmo7z X9uoww/+PcGp5yYzOom8eUs+afA4KaQWFu+aRNt6DhICWM5RH1t+rEHK5NAqQyFA LsJK0aMFJ3SMrE2IWNiOSH65ihoD9ZO0+npMwue9kiDrNAAk64Brk/jm0mAtYsOE LbtFOeDTtGd1UcUV8/5TFUzfo3BlkaScGAYfG/RBoJ6Celw3cjwo/OT8klmh0Aaj Q0kPMwQhgfdLMpd+NfB249ONuPTp0jWXevpGNJv+oAjwM12RxMJSJw9K+Yeld32h Zx2ZeI8wbsR3KfxRlATXuK/CZZPvLJmKUJZO3/dWdV1QwX3bMbCWo62xaiyaDft5 wBSbSDgKwnvjXp4FT8aGzvVExhNfuhfHdGKYGWB5Lx+PQNGk7rvnAPrWIVikq9Sj IqPUFmHUN/LGyPr8OtKNjOUubjkiuv0GvN4kFHTBrZAV6J7uJEiqHWL6OEBg3Wbo rDlmKBZEqFMNPV78vWQFquN7r+LB8z2ZMsJpJnpaHhOCCgwH2iFKKVHZACU5VMAE g3oJUUFSgddcsKn6I41Om9BsKlVuzkb8tEhGk8/WTaYNPKfiRNLu91/PXVE4g5dK 8arbUOsKDE29RTJz8XjCUhoKUY4W93+IJ14x+Qh1K2FvHBW94ZyCJm2UlRyJmZgF pIjduVQCndusSLmJ7YMxhtFWXpi5DpX6s9FE8kQillC1U2kg/Rg= =bYs8 -----END PGP SIGNATURE----- --n2Pv11Ogg/Ox8ay5-- 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=-3.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no 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 1EB56C4363D for ; Sun, 4 Oct 2020 04:11:53 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 91041206DB for ; Sun, 4 Oct 2020 04:11:52 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="CUf0VceC"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="c9/WbX5n"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=gmx.net header.i=@gmx.net header.b="LvrFpkrZ" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 91041206DB Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=gmx.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type:Cc: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=QdALZFk/dAaKdh7I0aCoK8sKSecQMEImlhxhZ+kG8f8=; b=CUf0VceCp7RHKeGpvvV3DiFIE 8VuyJXwDl2jkGTRZb2Y4vAQK+ZphY0liF6p6esZVracCa3/6sa2Q+3F+AzLqb1M5kI2xOAmtidVu7 MxriYttaWvzJmq2gPrfjejEGtUTuA/JmqeHybiMFrDv+6ItbSPO3muLKppphPVwLIx5WMlUTMUPZJ xMimJUsfbu8mxamQooyWA+RxFgTIpKp6KJymfsyxJwSyklbNfBsKrfSFndA5ibnVUhKPMDq2hhjlg TdNIq2qnHFiwEmEUgtpQQWw5xxb613k/frVgMc2FP/wn7eLGSz47zgoBm2GMuc1Y9lUuAaz6RwT1i i+9obk8Ow==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kOvLy-0006DS-JV; Sun, 04 Oct 2020 04:10:30 +0000 Received: from casper.infradead.org ([2001:8b0:10b:1236::1]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kOvLv-0006CY-Gs for linux-arm-kernel@merlin.infradead.org; Sun, 04 Oct 2020 04:10:27 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=ugdh1gptypTtvfFihSWynQ9ocpWl4t8EpGkozjWIQQ0=; b=c9/WbX5n1tP3CKdj57jwzySvmq LjfUiJtx8469Z6Sc+emaY0yUgp6WN1ulL+KZivhEqdbKbeq/p3oqbcz84r0vSr9g+F7o1HpNApo0v QPLrClnZxP0NOFcOj31MM3z+T38g09ICRsF2vp/rDi2BhhFfmHzTBeWrNvvOsp5rPLKNgyKJ+yH9z dWep74grjvLDMBH1q4svlxdMuU4NuCG3BRmMS/w4A5nQPdvFWUkgdK6ak/tQTmsHFlCyY0uAC9ujo Fk5mxNDNH+joTMSwfHGVkJvIGa9xcQqbkcLayyt9GYfaYRR/gu1K+Ao6Yd9XoUD619FPDo84jyj6+ x/ecbZ9g==; Received: from mout.gmx.net ([212.227.15.19]) by casper.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kOtAP-0001m2-6Z for linux-arm-kernel@lists.infradead.org; Sun, 04 Oct 2020 01:50:30 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=badeba3b8450; t=1601776218; bh=uk/zgEMcmh53hyYJsD9rVSTVCXFi5qhVJPo/Njr0ULU=; h=X-UI-Sender-Class:Date:From:To:Cc:Subject:References:In-Reply-To; b=LvrFpkrZ1Exg8aSUWxcDrfZlqxGq88zhEeZ2eSykYwvDmjJswbl9Dt1Xvzd8euplN yTV2qsquhWRBOCWh+i8O07if1fY9LY1kGM2GtdlxNXvKAjKt86cXbKhqBVpy3GN1ig 6xgoO6oySG2iIMBDIWsk/Jfp9NfpH6O1PcrrTnq8= X-UI-Sender-Class: 01bb95c1-4bf8-414a-932a-4f6e2808ef9c Received: from longitude ([5.146.195.151]) by mail.gmx.com (mrgmx004 [212.227.17.190]) with ESMTPSA (Nemesis) id 1MIdiZ-1kCV8e3OGD-00Efw6; Sun, 04 Oct 2020 03:43:29 +0200 Date: Sun, 4 Oct 2020 03:43:23 +0200 From: Jonathan =?utf-8?Q?Neusch=C3=A4fer?= To: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= Subject: Re: [PATCH v3 5/7] rtc: New driver for RTC in Netronix embedded controller Message-ID: <20201004014323.GD500800@latitude> References: <20200924192455.2484005-1-j.neuschaefer@gmx.net> <20200924192455.2484005-6-j.neuschaefer@gmx.net> <20200925054424.snlr3lggnsv575wu@pengutronix.de> MIME-Version: 1.0 In-Reply-To: <20200925054424.snlr3lggnsv575wu@pengutronix.de> X-Provags-ID: V03:K1:3QmB/hlTSK7COVsSvQUbFXyPQLlzF/h6TMiid01Z4Clm5Arc98v uler7ylH8j9QBxD0tykisMycm+wD9UERrrd+ewA2/OjwuUspDeo2xtT/2Qnkl8U5/H4az8y 4tGKFKCvQ7SDiOJTQPqyHUPXztu5tbDBN2qCAnWjwF5YxsmxGkAAaxXhyN3ZVDti5n96UBi 6gO033n3MxkAvTUF1Iyzg== X-UI-Out-Filterresults: notjunk:1;V03:K0:EILGLLRDsmg=:irZMLvjkeVP3Dqtp50gShH /+9WkWVgpcxsrdPxIT55kPJxHCWiZJqOlr3HpgSlT8Pak6bSte8YGpHpuJivu48yCrqDnIntB r7yKVX1t5llUW9x0g+oTr0npjIukrmydHvt6wg08NvK+yhGJYjyUQv1MHRB41tpdxGj8aqteW 6LwzL4FDT/iqt2xJxdxE/WhAuizXhatvKaqr9EMvfqVgq+JcnsmEx8cxsxn3XREoue97vpzmO u0sBLmtkrCcge31qdWeTiOMUxGn2EXWR+MRI5CFv5CiWZQZtGHPYvmsnZlONxID5jjfIzmdmK P/OkF8bCOAKkmyGOGh7kkhteCqeDL4Fy6byU3OK0CUHcCdfoB/DhGaGsxZIqSmQLKUxdCgZ6F 4b8eKHI2wi987D8G2EIuSAjsgyZOskGZUXYqgx17BrNmwZ1JCvp5fSICZhdn6PXPUvHJ0JOpA dyIeDEXhfzAAGNMeb1yr6EXGp/edDCL33ejTuf6S7DG3fYbRlh26mdCO/QUcP+qhUrtKzXRdv Xt24ePizjxNBPYaath/8X4MBi4qfR8YR6orEsqvh+yvR5ojqeOTtO1uoTjyJ0SGIJydQ+BcIR S8r+gpmOONi21GF8Y+fyiS1rcwZBTu8tK3XA/BfD2MbFRgout0xSS2gDGQt+RqolEUR9e+sno ED8BhwGfA2stgnTtuFdMHrvnBONddZ/yiHBML+8V/IY0ltQLTv3wbGiEUhG3HWPh/c8QwMp/H zlp2yU+XJVRPYEplgQFMUOR0c0ctWG7oYsjsWADRvTqxcEP1LcKAto7/zkjPz5Cvfkn3TOqzA LtU7bbv0G2w2zpnG0xwZVx2iBGoYF5z4QrXi/CPcAKycCVDN7ELib2nAcrK2whOUljfc2V3Xa FucUxvzyoaOWQeNqzA4VYcn7F49T5+OtBl38/cYisKPcu1yw7YcP1JGxlm00peWtW4oVLm2Kq gt0a0+oeCD0FxilEn6w3YNbfZ99LjmPwhSBATA5uCBnqkytw5W7M1vbIgJlYRzeprerPvl9E0 k8PUDujaiqSO2rwRZRgyjv4sekFJv4krfZ3YsN8SnHjur8GMmF4n1UCgSuWJK9SWcOb1YwItY Ap+6UqLee90j5luPx42Y26MNyUfPCBUTWjQzXbAlvwXCUvvYZzAIRPSY1yAYcYQyU6rxPVOin vWbZYBbdjC5xLweYfUfDaZ5P5RymtjkGaxASYPNeuIOI7gjPsa1IXksujLUUpYdaXFvbRzQvI NETKppz3eHzkKfRR6gJPg36NQxBTfH61jB51S0Q== X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201004_025025_846655_3EB8B33E X-CRM114-Status: GOOD ( 34.75 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Alexandre Belloni , Heiko Stuebner , devicetree@vger.kernel.org, Linus Walleij , Thierry Reding , Sam Ravnborg , linux-rtc@vger.kernel.org, Arnd Bergmann , Mauro Carvalho Chehab , Fabio Estevam , Daniel Palmer , Andy Shevchenko , Andreas Kemnade , NXP Linux Team , linux-pwm@vger.kernel.org, Stephan Gerhold , allen , Sascha Hauer , Jonathan =?utf-8?Q?Neusch=C3=A4fer?= , Lubomir Rintel , Rob Herring , Lee Jones , linux-arm-kernel@lists.infradead.org, Alessandro Zummo , linux-kernel@vger.kernel.org, Mark Brown , Pengutronix Kernel Team , Heiko Stuebner , Josua Mayer , Shawn Guo , "David S. Miller" Content-Type: multipart/mixed; boundary="===============0373148185613026190==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============0373148185613026190== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="n2Pv11Ogg/Ox8ay5" Content-Disposition: inline --n2Pv11Ogg/Ox8ay5 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Sep 25, 2020 at 07:44:24AM +0200, Uwe Kleine-K=C3=B6nig wrote: > Hello Jonathan, >=20 > On Thu, Sep 24, 2020 at 09:24:53PM +0200, Jonathan Neusch=C3=A4fer wrote: > > +#define NTXEC_REG_WRITE_YEAR 0x10 > > +#define NTXEC_REG_WRITE_MONTH 0x11 > > +#define NTXEC_REG_WRITE_DAY 0x12 > > +#define NTXEC_REG_WRITE_HOUR 0x13 > > +#define NTXEC_REG_WRITE_MINUTE 0x14 > > +#define NTXEC_REG_WRITE_SECOND 0x15 > > + > > +#define NTXEC_REG_READ_YM 0x20 > > +#define NTXEC_REG_READ_DH 0x21 > > +#define NTXEC_REG_READ_MS 0x23 >=20 > Is this an official naming? I think at least ..._MS is a poor name. > Maybe consider ..._MINSEC instead and make the other two names a bit long= er > for consistency? It's inofficial (the vendor kernel uses 0x10 etc. directly). I'll pick longer names. > > +static int ntxec_read_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct ntxec_rtc *rtc =3D dev_get_drvdata(dev); > > + unsigned int value; > > + int res; > > + > > + res =3D regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value); > > + if (res < 0) > > + return res; > > + > > + tm->tm_year =3D (value >> 8) + 100; > > + tm->tm_mon =3D (value & 0xff) - 1; > > + > > + res =3D regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value); > > + if (res < 0) > > + return res; > > + > > + tm->tm_mday =3D value >> 8; > > + tm->tm_hour =3D value & 0xff; > > + > > + res =3D regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value); > > + if (res < 0) > > + return res; > > + > > + tm->tm_min =3D value >> 8; > > + tm->tm_sec =3D value & 0xff; > > + > > + return 0; > > +} > > + > > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct ntxec_rtc *rtc =3D dev_get_drvdata(dev); > > + int res =3D 0; > > + > > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg= 8(tm->tm_year - 100)); > > + if (res) > > + return res; > > + > > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_re= g8(tm->tm_mon + 1)); > > + if (res) > > + return res; > > + > > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8= (tm->tm_mday)); > > + if (res) > > + return res; > > + > > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg= 8(tm->tm_hour)); > > + if (res) > > + return res; > > + > > + res =3D regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_r= eg8(tm->tm_min)); > > + if (res) > > + return res; > > + > > + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_re= g8(tm->tm_sec)); >=20 > I wonder: Is this racy? If you write minute, does the seconds reset to > zero or something like that? Or can it happen, that after writing the > minute register and before writing the second register the seconds > overflow and you end up with the time set to a minute later than > intended? If so it might be worth to set the seconds to 0 at the start > of the function (with an explaining comment). The setting the minutes does not reset the seconds, so I think this race condition is possible. I'll add the workaround. > .read_time has a similar race. What happens if minutes overflow between > reading NTXEC_REG_READ_DH and NTXEC_REG_READ_MS? Yes, we get read tearing in that case. It could even propagate all the way to the year/month field, for example when the following time rolls over: A | B | C 2020-10-31 23:59:59 2020-11-01 00:00:00 - If the increment happens after reading C, we get 2020-10-31 23:59= :59 - If the increment happens between reading B and C, we get 2020-10-31 23:00= :00 - If the increment happens between reading A and B, we get 2020-10-01 00:00= :00 - If the increment happens before reading A, we get 2020-11-01 00:00= :00 =2E.. both of which are far from correct. To mitigate this issue, I think something like the following is needed: - Read year/month - Read day/hour - Read minute/second - Read day/hour, compare with previously read value, restart on mismatch - Read year/month, compare with previously read value, restart on mismatch The order of the last two steps doesn't matter, as far as I can see, but if I remove one of them, I can't catch all cases of read tearing. > > +static struct platform_driver ntxec_rtc_driver =3D { > > + .driver =3D { > > + .name =3D "ntxec-rtc", > > + }, > > + .probe =3D ntxec_rtc_probe, >=20 > No .remove function? I don't think it would serve a purpose in this driver. There are no device-specific resources to release (no clocks to unprepare, for example). Thanks, Jonathan Neusch=C3=A4fer --n2Pv11Ogg/Ox8ay5 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEvHAHGBBjQPVy+qvDCDBEmo7zX9sFAl95KK0ACgkQCDBEmo7z X9uoww/+PcGp5yYzOom8eUs+afA4KaQWFu+aRNt6DhICWM5RH1t+rEHK5NAqQyFA LsJK0aMFJ3SMrE2IWNiOSH65ihoD9ZO0+npMwue9kiDrNAAk64Brk/jm0mAtYsOE LbtFOeDTtGd1UcUV8/5TFUzfo3BlkaScGAYfG/RBoJ6Celw3cjwo/OT8klmh0Aaj Q0kPMwQhgfdLMpd+NfB249ONuPTp0jWXevpGNJv+oAjwM12RxMJSJw9K+Yeld32h Zx2ZeI8wbsR3KfxRlATXuK/CZZPvLJmKUJZO3/dWdV1QwX3bMbCWo62xaiyaDft5 wBSbSDgKwnvjXp4FT8aGzvVExhNfuhfHdGKYGWB5Lx+PQNGk7rvnAPrWIVikq9Sj IqPUFmHUN/LGyPr8OtKNjOUubjkiuv0GvN4kFHTBrZAV6J7uJEiqHWL6OEBg3Wbo rDlmKBZEqFMNPV78vWQFquN7r+LB8z2ZMsJpJnpaHhOCCgwH2iFKKVHZACU5VMAE g3oJUUFSgddcsKn6I41Om9BsKlVuzkb8tEhGk8/WTaYNPKfiRNLu91/PXVE4g5dK 8arbUOsKDE29RTJz8XjCUhoKUY4W93+IJ14x+Qh1K2FvHBW94ZyCJm2UlRyJmZgF pIjduVQCndusSLmJ7YMxhtFWXpi5DpX6s9FE8kQillC1U2kg/Rg= =bYs8 -----END PGP SIGNATURE----- --n2Pv11Ogg/Ox8ay5-- --===============0373148185613026190== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============0373148185613026190==--