LKML Archive on lore.kernel.org
 help / color / Atom feed
From: "Jonathan Neuschäfer" <j.neuschaefer@gmx.net>
To: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: "Jonathan Neuschäfer" <j.neuschaefer@gmx.net>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	linux-kernel@vger.kernel.org, "Heiko Stuebner" <heiko@sntech.de>,
	linux-pwm@vger.kernel.org,
	"Linus Walleij" <linus.walleij@linaro.org>,
	"Thierry Reding" <thierry.reding@gmail.com>,
	"Fabio Estevam" <festevam@gmail.com>,
	linux-rtc@vger.kernel.org, "Arnd Bergmann" <arnd@arndb.de>,
	"Mauro Carvalho Chehab" <mchehab+huawei@kernel.org>,
	"Sam Ravnborg" <sam@ravnborg.org>,
	"Daniel Palmer" <daniel@0x0f.com>,
	"Andy Shevchenko" <andy.shevchenko@gmail.com>,
	"Andreas Kemnade" <andreas@kemnade.info>,
	"NXP Linux Team" <linux-imx@nxp.com>,
	devicetree@vger.kernel.org,
	"Stephan Gerhold" <stephan@gerhold.net>,
	allen <allen.chen@ite.com.tw>,
	"Sascha Hauer" <s.hauer@pengutronix.de>,
	"Lubomir Rintel" <lkundrak@v3.sk>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Lee Jones" <lee.jones@linaro.org>,
	linux-arm-kernel@lists.infradead.org,
	"Alessandro Zummo" <a.zummo@towertech.it>,
	"Mark Brown" <broonie@kernel.org>,
	"Pengutronix Kernel Team" <kernel@pengutronix.de>,
	"Heiko Stuebner" <heiko.stuebner@theobroma-systems.com>,
	"Josua Mayer" <josua.mayer@jm0.eu>,
	"Shawn Guo" <shawnguo@kernel.org>,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH v3 5/7] rtc: New driver for RTC in Netronix embedded controller
Date: Sun, 11 Oct 2020 21:09:10 +0200
Message-ID: <20201011190910.GE500800@latitude> (raw)
In-Reply-To: <20201004084209.GV2804081@piout.net>


[-- Attachment #1: Type: text/plain, Size: 3332 bytes --]

On Sun, Oct 04, 2020 at 10:42:09AM +0200, Alexandre Belloni wrote:
> On 04/10/2020 03:43:23+0200, Jonathan Neuschäfer wrote:
> > > > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm)
....
> > > > +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min));
> > > > +	if (res)
> > > > +		return res;
> > > > +
> > > > +	return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec));
> > > 
> > > 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.
> > 
> 
> Are you sure this happens? Usually, the seconds are not reset but the
> internal 32768kHz counter is so you have a full second to write all the
> registers.

I just checked it, and on this RTC, the phase / sub-second part is not
reset when the time is set.

> > > .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
> > 
> > ... 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.
> > 
> 
> Are you also sure this happens?
> 
> Only one comparison is necessary, the correct order would be:
> 
>  - Read minute/second
>  - Read day/hour
>  - Read year/month
>  - Read minute/second, compare

With this order, every one-second increment is detected, which I
previously tried to avoid. But I suppose it's fine because it simplifies
the logic and the window from first to last read should be short enough
anyway to be relatively unlikely to hit, and thus not cause a lot of retries.

> If day/hour changes but not minute/second, it would mean that it took at
> least an hour to read all the registers. At this point, I think you have
> other problems and the exact time doesn't matter anymore.

Indeed.


Thanks,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-24 19:24 [PATCH v3 0/7] Netronix embedded controller driver for Kobo and Tolino ebook readers Jonathan Neuschäfer
2020-09-24 19:24 ` [PATCH v3 1/7] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer
2020-10-12  7:19   ` Uwe Kleine-König
2020-10-12 11:48     ` Lee Jones
2020-09-24 19:24 ` [PATCH v3 2/7] dt-bindings: mfd: Add binding for Netronix embedded controller Jonathan Neuschäfer
2020-09-29 18:52   ` Rob Herring
2020-09-24 19:24 ` [PATCH v3 3/7] mfd: Add base driver " Jonathan Neuschäfer
2020-09-25  9:29   ` Andy Shevchenko
2020-09-25 21:32     ` Jonathan Neuschäfer
2020-09-29 16:37       ` Andy Shevchenko
2020-10-02 22:54         ` Jonathan Neuschäfer
2020-09-24 19:24 ` [PATCH v3 4/7] pwm: ntxec: Add driver for PWM function in Netronix EC Jonathan Neuschäfer
2020-09-25  6:30   ` Uwe Kleine-König
2020-09-27 21:10     ` Jonathan Neuschäfer
2020-09-28  8:35       ` Uwe Kleine-König
2020-10-02 23:36         ` Jonathan Neuschäfer
2020-10-03  8:17           ` Andy Shevchenko
2020-09-24 19:24 ` [PATCH v3 5/7] rtc: New driver for RTC in Netronix embedded controller Jonathan Neuschäfer
2020-09-24 20:40   ` Andreas Kemnade
2020-10-02 23:41     ` Jonathan Neuschäfer
2020-09-25  5:44   ` Uwe Kleine-König
2020-10-04  1:43     ` Jonathan Neuschäfer
2020-10-04  8:42       ` Alexandre Belloni
2020-10-11 19:09         ` Jonathan Neuschäfer [this message]
2020-10-05  7:35       ` Uwe Kleine-König
2020-09-25  9:36   ` Alexandre Belloni
2020-09-25 22:00     ` Jonathan Neuschäfer
2020-09-24 19:24 ` [PATCH v3 6/7] MAINTAINERS: Add entry for " Jonathan Neuschäfer
2020-09-25  5:08 ` [PATCH v3 7/7] ARM: dts: imx50-kobo-aura: Add " Jonathan Neuschäfer
2020-10-07  7:46   ` Krzysztof Kozlowski
2020-10-11 19:42     ` Jonathan Neuschäfer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201011190910.GE500800@latitude \
    --to=j.neuschaefer@gmx.net \
    --cc=a.zummo@towertech.it \
    --cc=alexandre.belloni@bootlin.com \
    --cc=allen.chen@ite.com.tw \
    --cc=andreas@kemnade.info \
    --cc=andy.shevchenko@gmail.com \
    --cc=arnd@arndb.de \
    --cc=broonie@kernel.org \
    --cc=daniel@0x0f.com \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=festevam@gmail.com \
    --cc=heiko.stuebner@theobroma-systems.com \
    --cc=heiko@sntech.de \
    --cc=josua.mayer@jm0.eu \
    --cc=kernel@pengutronix.de \
    --cc=lee.jones@linaro.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=linux-rtc@vger.kernel.org \
    --cc=lkundrak@v3.sk \
    --cc=mchehab+huawei@kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=s.hauer@pengutronix.de \
    --cc=sam@ravnborg.org \
    --cc=shawnguo@kernel.org \
    --cc=stephan@gerhold.net \
    --cc=thierry.reding@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git