linux-kernel - Re: [PATCH v2 2/2] clocksource: Add driver for the Ingenic JZ47xx OST

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]

Message-ID: <87a76wln67.fsf@nanos.tec.linutronix.de>
Date:   Thu, 09 Jan 2020 15:28:00 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Paul Cercueil <paul@...pouillou.net>,
        Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     od@...c.me, linux-kernel@...r.kernel.org,
        Maarten ter Huurne <maarten@...ewalker.org>,
        Paul Cercueil <paul@...pouillou.net>,
        Mathieu Malaterre <malat@...ian.org>,
        Artur Rojek <contact@...ur-rojek.eu>
Subject: Re: [PATCH v2 2/2] clocksource: Add driver for the Ingenic JZ47xx OST

Paul Cercueil <paul@...pouillou.net> writes:
> +static u64 notrace ingenic_ost_clocksource_read64(struct clocksource *cs)
> +{
> +	u32 val1, val2;
> +	u64 count, recount;
> +	s64 diff;
> +
> +	/*
> +	 * The buffering of the upper 32 bits of the timer prevents wrong
> +	 * results from the bottom 32 bits overflowing due to the timer ticking
> +	 * along. However, it does not prevent wrong results from simultaneous
> +	 * reads of the timer, which could reset the buffer mid-read.
> +	 * Since this kind of wrong read can happen only when the bottom bits
> +	 * overflow, there will be minutes between wrong reads, so if we read
> +	 * twice in succession, at least one of the reads will be correct.
> +	 */
> +
> +	/* Bypass the regmap here as we must return as soon as possible */

I have a hard time to understand this comment. "Bypass the regmap ..."
and then use a regmap function?

> +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val1);
> +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTHBUF, &val2);
> +	count = (u64)val1 | (u64)val2 << 32;
> +
> +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val1);
> +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTHBUF, &val2);
> +	recount = (u64)val1 | (u64)val2 << 32;
> +
> +	/*
> +	 * A wrong read will produce a result that is 1<<32 too high: the bottom
> +	 * part from before overflow and the upper part from after overflow.
> +	 * Therefore, the lower value of the two reads is the correct value.
> +	 */
> +
> +	diff = (s64)(recount - count);
> +	if (unlikely(diff < 0))
> +		count = recount;

Is this really the right approach here? What is the 64bit readout buying
you?

The timekeeping code can handle a 32bit counter perfectly fine and the
only advantage you get is that your maximum possible idle time will be
longer with a 64bit counter.

But is that really worth the overhead of four MMIO reads versus one in a
hotpath?

Thanks,

        tglx