linux-kernel - Re: [PATCH 5/6] RTC: SMBus support for the M41T80, etc. driver (#2)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 13 May 2008 17:57:52 +0100 (BST)
From:	"Maciej W. Rozycki" <macro@...ux-mips.org>
To:	Jean Delvare <khali@...ux-fr.org>
cc:	Alessandro Zummo <a.zummo@...ertech.it>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Atsushi Nemoto <anemo@....ocn.ne.jp>,
	David Woodhouse <dwmw2@...radead.org>,
	Ralf Baechle <ralf@...ux-mips.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	rtc-linux@...glegroups.com, i2c@...sensors.org,
	linux-mips@...ux-mips.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/6] RTC: SMBus support for the M41T80, etc. driver (#2)

Hi Jean,

> The I2C part of the changes look OK to me. With one comment below:
> 
> > +static int m41t80_get_datetime(struct i2c_client *client, struct rtc_time *tm)
> > +{
> > +	u8 buf[M41T80_DATETIME_REG_SIZE];
> > +	int loops = 2;
> > +	int sec0, sec1;
> > +
> > +	/*
> > +	 * Time registers are latched by this chip if an I2C block
> > +	 * transfer is used, but with SMBus-style byte accesses
> > +	 * this is not the case, so check seconds for a wraparound.
> > +	 */
> > +	do {
> > +		if (m41t80_read_block_data(client, M41T80_REG_SEC,
> > +					   M41T80_DATETIME_REG_SIZE -
> > +					   M41T80_REG_SEC,
> > +					   buf + M41T80_REG_SEC) < 0) {
> > +			dev_err(&client->dev, "read error\n");
> > +			return -EIO;
> > +		}
> > +		sec0 = buf[M41T80_REG_SEC];
> >  
> > -	tm->tm_sec = BCD2BIN(buf[M41T80_REG_SEC] & 0x7f);
> > +		sec1 = i2c_smbus_read_byte_data(client, M41T80_REG_SEC);
> > +		if (sec1 < 0) {
> > +			dev_err(&client->dev, "read error\n");
> > +			return -EIO;
> > +		}
> > +
> > +		sec0 = BCD2BIN(sec0 & 0x7f);
> > +		sec1 = BCD2BIN(sec1 & 0x7f);
> > +	} while (sec1 < sec0 && --loops);
> 
> You will do this even if all the registers were read as a block and the
> RTC latched the register values so they have to be correct. Isn't it a
> bit unfair / inefficient? If client->adapter has the
> I2C_FUNC_SMBUS_READ_I2C_BLOCK functionality you can skip the comparison
> and retry mechanism completely, saving some time and CPU cycles.

 Well, actually there is a reason beyond that.  It may change if we
support subsecond resolution, but we currently do not.  The reason is if
we return the original timestamp and the seconds register changes while
the timestamp is being read, then effectively we return a value that is
off by one second.  This is why for seconds I decided to return the second
value read all the time.

  Maciej
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/