linux-kernel - Re: [PATCH v4 1/3] rtc: tps6594: Add driver for TPS6594 RTC

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:   Wed, 17 May 2023 18:47:47 +0200
From:   "Esteban Blanc" <eblanc@...libre.com>
To:     <andy.shevchenko@...il.com>
Cc:     <linus.walleij@...aro.org>, <lgirdwood@...il.com>,
        <broonie@...nel.org>, <a.zummo@...ertech.it>,
        <alexandre.belloni@...tlin.com>, <linux-kernel@...r.kernel.org>,
        <linux-gpio@...r.kernel.org>, <linux-rtc@...r.kernel.org>,
        <jpanis@...libre.com>, <jneanne@...libre.com>,
        <aseketeli@...libre.com>, <sterzik@...com>, <u-kumar1@...com>
Subject: Re: [PATCH v4 1/3] rtc: tps6594: Add driver for TPS6594 RTC

On Fri May 12, 2023 at 7:22 PM CEST,  wrote:
> Fri, May 12, 2023 at 04:17:53PM +0200, Esteban Blanc kirjoitti:
> > +/* Multiplier for ppb conversions */
> > +#define PPB_MULT (1000000000LL)
>
> We have something in units.h. Can you use generic macro?

I found GIGA, NANO and NANOHZ_PER_HZ that have the same value in
units.h. However I'm not sure any of them have the correct meaning in
this situation.

> > +	/*
> > +	 * Set GET_TIME to 0. This way, next time we set GET_TIME to 1 we are sure to store an
> > +	 * up-to-date timestamp
> > +	 */
>
> Please, check all your multi-line comments for proper punctuation.

Ok. I will do my best.

> > +	/* Check if RTC is running. */
>
> Please, keep a single style for the one-line comments (with or without period
> at the end).

Sure.

> > +	return ret;
>
> Why not return 0 explicitly? Or do you return positive value?

I should return 0 here, you are right.

> > +	comp_data[0] = (u16)value & 0xFF;
> > +	comp_data[1] = ((u16)value >> 8) & 0xFF;
>
> Use proper bitwise type, i.e. __le16.

I was not aware of theses kind of types. Thanks!

> > +	/* Convert from RTC calibration register format to ppb format */
> > +	tmp = calibration * (s64)PPB_MULT;
>
> Is casting really needed?

No it's not. Thanks!

> > +	if (tmp < 0)
> > +		tmp -= TICKS_PER_HOUR / 2LL;
> > +	else
> > +		tmp += TICKS_PER_HOUR / 2LL;
>
> Is it guaranteed to have no overflow here?

We know from `tps6594_rtc_set_offset` that the loaded value can't be
more than 277774 (register default value is 0), So `tmp` can't exceed
277774000000000 which is lower than 2^63-1. No overflow here.

TICK_PER_HOUR / 2LL = 117964800, so at the end of this computation,
`tmp` can have a maximum value of 277774117964800 which is still
inferior to 2^63-1.

> > +	tmp = div_s64(tmp, TICKS_PER_HOUR);
> > +
> > +	/*
> > +	 * Offset value operates in negative way, so swap sign.
> > +	 * See 8.3.10.5, (32768 - COMP_REG)
> > +	 */
> > +	*offset = (long)-tmp;
> > +
> > +	return ret;
>
> ret?!

That's a mistake. Thanks!

> > +	/* RTC not running */
> > +	if (ret == 0) {
> > +		/* Start rtc */
>
> RTC for the sake of consistency.
>
> But I think one of the comment is redundant.

I will remove the second one

> > +		mdelay(100);
>
> Such long delays have to be explicitly elaborated (in the comment on top).

This should just not be there.


Best regards,

-- 
Esteban Blanc
BayLibre