Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 4 Oct 2020 03:43:23 +0200
From:   Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>
To:     Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Cc:     Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>,
        linux-kernel@vger.kernel.org,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        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
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>
Precedence: bulk


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