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