linux-kernel - Re: [PATCH v2 4/9] clocksource/cadence_ttc: Use enable/disable

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:	Thu, 28 Nov 2013 15:18:50 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Daniel Lezcano <daniel.lezcano@...aro.org>
cc:	Soren Brinkmann <soren.brinkmann@...inx.com>,
	Rob Herring <rob.herring@...xeda.com>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Stephen Warren <swarren@...dotorg.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Russell King <linux@....linux.org.uk>,
	Michal Simek <michal.simek@...inx.com>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH v2 4/9] clocksource/cadence_ttc: Use enable/disable_irq

On Thu, 28 Nov 2013, Daniel Lezcano wrote:

> On 11/27/2013 02:04 AM, Soren Brinkmann wrote:
> > To ensure that the timer interrupt is properly enabled/disabled across
> > the whole CPU cluster use enable/disable_irq() instead of
> > local_irq_disable().
> > 
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@...inx.com>
> > ---
> >   drivers/clocksource/cadence_ttc_timer.c | 6 ++----
> >   1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/clocksource/cadence_ttc_timer.c
> > b/drivers/clocksource/cadence_ttc_timer.c
> > index a92350b55d32..246d018d1e63 100644
> > --- a/drivers/clocksource/cadence_ttc_timer.c
> > +++ b/drivers/clocksource/cadence_ttc_timer.c
> > @@ -322,18 +322,16 @@ static int ttc_rate_change_clockevent_cb(struct
> > notifier_block *nb,
> >   	switch (event) {
> >   	case POST_RATE_CHANGE:
> >   	{
> > -		unsigned long flags;
> > -
> >   		/*
> >   		 * clockevents_update_freq should be called with IRQ disabled
> > on
> >   		 * the CPU the timer provides events for. The timer we use is
> >   		 * common to both CPUs, not sure if we need to run on both
> >   		 * cores.

Can we adjust that bogus comment as well, please?

> >   		 */
> > -		local_irq_save(flags);
> > +		disable_irq(ttcce->ce.irq);
> >   		clockevents_update_freq(&ttcce->ce,
> >   				ndata->new_rate / PRESCALE);
> > -		local_irq_restore(flags);
> > +		enable_irq(ttcce->ce.irq);
> > 
> >   		/* update cached frequency */
> >   		ttc->freq = ndata->new_rate;
> > 
> 
> I am worried about the 'disable_irq' function calling 'synchronize_irq'. Isn't
> possible to deadlock with the ondemand cpufreq governor ? I added Viresh in
> Cc, he knows better than me the code path.

disable_irq() will only deadlock if called from the interrupt handler
of the device interrupt itself or when the calling code is holding
locks which prevent the function to proceed.

But what's more important is, that the patch violates the calling
convention of clockevents_update_freq(). It must be called with
interrupts disabled. 

Now the problem with this device is that it is not a per cpu
device. It's a global device, so this update can conflict with a
parallel access on the other CPU. Now the disable_irq() only prevents
that the other CPU can handle a device interrupt from that timer. But
it does not prevent any parallel access from e.g. the idle code path
which will try to reprogram it.

Soren, is that timer used as the broadcast device ?

Thanks,

	tglx



--
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/