linux-kernel - Re: [PATCH 42/48] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

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: <20160622151329.4f0136ba@bbrezillon>
Date:	Wed, 22 Jun 2016 15:13:29 +0200
From:	Boris Brezillon <boris.brezillon@...e-electrons.com>
To:	Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:	Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Nicolas Ferre <nicolas.ferre@...el.com>,
	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 42/48] clocksource/drivers: Add a new driver for the
 Atmel ARM TC blocks

On Wed, 22 Jun 2016 15:07:00 +0200
Daniel Lezcano <daniel.lezcano@...aro.org> wrote:

> On 06/11/2016 02:48 PM, Boris Brezillon wrote:
> 
> [ ... ]
> 
> >> +static int tcb_clkevt_next_event(unsigned long delta,
> >> +				 struct clock_event_device *d)
> >> +{
> >> +	u32 val;
> >> +
> >> +	regmap_read(tc.regmap, ATMEL_TC_CV(tc.channels[0]), &val);
> >> +	regmap_write(tc.regmap, ATMEL_TC_RC(tc.channels[0]), val + delta);
> >> +	regmap_write(tc.regmap, ATMEL_TC_IER(tc.channels[0]), ATMEL_TC_CPCS);  
> >
> > Hm, not sure this is 100% sure. What happens if by the time you write
> > TC_RC, the delta value has expired? This means you'll have to wait
> > another round before the TC engine generates the "RC reached" interrupt.
> >
> > I know this is very unlikely, but should we take the risk?
> >
> > The core seems to check the ->set_next_event() return value and tries to
> > adjust ->min_delta_ns if it returns an error, so maybe it's worth
> > testing if val + delta has already occurred just before enabling the
> > TC_CPCS interrupt, and if it's the case, return an -ETIME error.
> >
> > Something like:
> >
> > 	u32 val[2], next;
> >
> > 	regmap_read(tc.regmap, ATMEL_TC_CV(tc.channels[0]), &val[0]);
> > 	next = (val[0] + delta) & GENMASK(tc.bits - 1, 0);
> > 	regmap_write(tc.regmap, ATMEL_TC_RC(tc.channels[0]), next);
> > 	regmap_read(tc.regmap, ATMEL_TC_CV(tc.channels[0]), &val[1]);
> >
> > 	if ((next < val[0] && val[1] < val[0] && val[1] >= next) ||
> > 	    (next > val[0] && (val[1] < val[0] || val[1] >= next))) {
> > 		/*
> > 		 * Clear the CPCS bit in the status register to avoid
> > 		 * generating a spurious interrupt next time a valid
> > 		 * timer event is configured.
> > 		 * FIXME: not sure it's safe, since it also clears the
> > 		 * overflow status, but it seems this flag is not used
> > 		 * by the driver anyway.
> > 		 */
> > 		regmap_read(tc.regmap, ATMEL_TC_SR, &val[0]);
> > 		return -ETIME;
> > 	}
> >
> > 	
> > 	regmap_write(tc.regmap, ATMEL_TC_IER(tc.channels[0]),
> > 		     ATMEL_TC_CPCS);
> >
> > Thomas, Daniel, what's your opinion?  
> 
> Are you describing the same as commit 
> f9eccf24615672896dc13251410c3f2f33a14f95 ?

Pretty much, yes. Note that this is purely hypothetical in the TCB
case, but I fear people might experience this problem if they're trying
to configure tiny delay values.