linux-kernel - Re: [PATCHv3 4/4] arm: Add generic timer broadcast support

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: <20130207171748.GB28072@e106331-lin.cambridge.arm.com>
Date:	Thu, 7 Feb 2013 17:17:48 +0000
From:	Mark Rutland <mark.rutland@....com>
To:	Santosh Shilimkar <santosh.shilimkar@...com>
Cc:	Stephen Warren <swarren@...dotorg.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"nico@...aro.org" <nico@...aro.org>,
	Will Deacon <Will.Deacon@....com>,
	Marc Zyngier <Marc.Zyngier@....com>,
	"john.stultz@...aro.org" <john.stultz@...aro.org>
Subject: Re: [PATCHv3 4/4] arm: Add generic timer broadcast support

[...]

> > The intent of 12ad100046: "clockevents: Add generic timer broadcast function"
> > was to setup the broadcast function both for non-functional/dummy timers and
> > those that stop in low-power states (CLOCK_EVT_FEAT_C3STOP). I missed the
> > CLOCK_EVT_FEAT_C3STOP case.
> >
> I am not sure this exactly the case. Because in my testing, the C3STOP 
> path was exercised already. And if the C3STOP is used then notifiers
> calls are expected for switching of clock-events to broadcast mode.

I don't thing the C3STOP path was fully exercised.

The notifier calls sets up the broadcast source and adds cpus to
tick_broadcast_oneshot_mask, but they don't set up the broadcast function on
the local timer clock_event_devices. While everything else was set up for broadcast,
the CPU in idle's tick_cpu_device->evtdev had a NULL broadcast function pointer.

As soon as the broadcast device receives its interrupt, it attempts to
broadcast to all cpus in tick_broadcast_oneshot_mask, calling the broadcast
function pointer on their local timers' struct clock_event_device. As this was
never set up, it explodes, attempting to branch to NULL.

> 
> And dummy broad-cast hook should come into picture only if the per CPU
> local timer clock-event are not registered.

This is irrelevant here. The issue is that the broadcast function pointer isn't
set up for clocks with CLOCK_EVT_FEAT_C3STOP.

> 
> I just tried "next-20130207" on OMAP and I see the broad-cast mechanism
> works and didn't observe any crash.
> 
> ------------------------------
> Tick Device: mode:     1
> Broadcast device
> Clock Event Device: gp_timer
>   max_delta_ns:   131071523464981
>   min_delta_ns:   91552
>   mult:           70369
>   shift:          31
>   mode:           3
>   next_event:     89984375000 nsecs
>   set_next_event: omap2_gp_timer_set_next_event
>   set_mode:       omap2_gp_timer_set_mode
>   event_handler:  tick_handle_oneshot_broadcast
>   retries:        0
> 
> tick_broadcast_mask: 00000003
> tick_broadcast_oneshot_mask: 00000000

The broadcast clocks' next broadcast is in ~90 seconds time.

[...]

> 
> Tick Device: mode:     1
> Per CPU device: 1
> Clock Event Device: local_timer
>   max_delta_ns:   10737418240
>   min_delta_ns:   1000
>   mult:           858993459
>   shift:          31
>   mode:           3
>   next_event:     89921875000 nsecs
>   set_next_event: twd_set_next_event
>   set_mode:       twd_set_mode
>   event_handler:  hrtimer_interrupt
>   retries:        258

And this clock next expects a tick in ~90 seconds time.

Did you leave the board on for at least 90 seconds after grabbing this output,
with the cpu staying in idle?

If not, the kernel won't have attempted to broadcast.

> 
> #
> 
> ------------------------------
> 
> > I believe the patch below will fix this for Tegra and any other platforms where
> > broadcast is required in low power states.
> >
> Am not sure you really need that patch unless and until am missing a
> scenario in my test.

I suspect that with the time before broadcast being so large, broadcast never
actually occurred (even though the global timer was ready for it to).

I've don't have a suitable OMAP platform to test that theory on, unfortunately.

> 
> Stephan,
> 
> Do we have crash log seen on Tegra ? Which tegra CPUIDLE driver
> is being used when crash is seen ?

I've acquired a Harmony board, and running next-20130207, I encounter the exact
issue I described above (with the PC at NULL). With my suggested patch applied,
the problem disappears.

I believe the idle driver would be tegra20-cpuidle, and I believe it's doing
the right thing.

Thanks,
Mark.

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