Message-ID: <20201004014323.GD500800@latitude>
Date:   Sun, 4 Oct 2020 03:43:23 +0200
From:   Jonathan Neuschäfer <j.neuschaefer@....net>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Jonathan Neuschäfer <j.neuschaefer@....net>,
        linux-kernel@...r.kernel.org,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Heiko Stuebner <heiko@...ech.de>, linux-pwm@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Fabio Estevam <festevam@...il.com>, linux-rtc@...r.kernel.org,
        Arnd Bergmann <arnd@...db.de>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        Sam Ravnborg <sam@...nborg.org>,
        Daniel Palmer <daniel@...f.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Andreas Kemnade <andreas@...nade.info>,
        NXP Linux Team <linux-imx@....com>,
        devicetree@...r.kernel.org, Stephan Gerhold <stephan@...hold.net>,
        allen <allen.chen@....com.tw>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Lubomir Rintel <lkundrak@...sk>,
        Rob Herring <robh+dt@...nel.org>,
        Lee Jones <lee.jones@...aro.org>,
        linux-arm-kernel@...ts.infradead.org,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Mark Brown <broonie@...nel.org>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Heiko Stuebner <heiko.stuebner@...obroma-systems.com>,
        Josua Mayer <josua.mayer@....eu>,
        Shawn Guo <shawnguo@...nel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v3 5/7] rtc: New driver for RTC in Netronix embedded
 controller

On Fri, Sep 25, 2020 at 07:44:24AM +0200, Uwe Kleine-König wrote:
> Hello Jonathan,
> 
> On Thu, Sep 24, 2020 at 09:24:53PM +0200, Jonathan Neuschäfer 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
> 
> 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 longer
> 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 = dev_get_drvdata(dev);
> > +	unsigned int value;
> > +	int res;
> > +
> > +	res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value);
> > +	if (res < 0)
> > +		return res;
> > +
> > +	tm->tm_year = (value >> 8) + 100;
> > +	tm->tm_mon = (value & 0xff) - 1;
> > +
> > +	res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value);
> > +	if (res < 0)
> > +		return res;
> > +
> > +	tm->tm_mday = value >> 8;
> > +	tm->tm_hour = value & 0xff;
> > +
> > +	res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value);
> > +	if (res < 0)
> > +		return res;
> > +
> > +	tm->tm_min = value >> 8;
> > +	tm->tm_sec = value & 0xff;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +	struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> > +	int res = 0;
> > +
> > +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100));
> > +	if (res)
> > +		return res;
> > +
> > +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1));
> > +	if (res)
> > +		return res;
> > +
> > +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday));
> > +	if (res)
> > +		return res;
> > +
> > +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour));
> > +	if (res)
> > +		return res;
> > +
> > +	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.

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

> > +static struct platform_driver ntxec_rtc_driver = {
> > +	.driver = {
> > +		.name = "ntxec-rtc",
> > +	},
> > +	.probe = ntxec_rtc_probe,
> 
> 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äfer

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists