| 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
| ||
|
Date: Tue, 18 Jun 2013 17:56:32 +0900
From: Magnus Damm <magnus.damm@...il.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: linux-kernel <linux-kernel@...r.kernel.org>,
Mark Rutland <mark.rutland@....com>,
SH-Linux <linux-sh@...r.kernel.org>,
Marc Zyngier <marc.zyngier@....com>,
Catalin Marinas <catalin.marinas@....com>,
"Simon Horman [Horms]" <horms@...ge.net.au>,
John Stultz <john.stultz@...aro.org>,
Shinya Kuribayashi <shinya.kuribayashi.px@...esas.com>,
Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH/RFC] clockevents: Ignore C3STOP when CPUIdle is disabled
Hi Daniel,
On Tue, Jun 18, 2013 at 5:30 PM, Daniel Lezcano
<daniel.lezcano@...aro.org> wrote:
> On 06/18/2013 09:17 AM, Magnus Damm wrote:
>> From: Magnus Damm <damm@...nsource.se>
>>
>> Introduce the function tick_device_may_c3stop() that
>> ignores the C3STOP flag in case CPUIdle is disabled.
>>
>> The C3STOP flag tells the system that a clock event
>> device may be stopped during deep sleep, but if this
>> will happen or not depends on things like if CPUIdle
>> is enabled and if a CPUIdle driver is available.
>>
>> This patch assumes that if CPUIdle is disabled then
>> the sleep mode triggering C3STOP will never be entered.
>> So by ignoring C3STOP when CPUIdle is disabled then it
>> becomes possible to use high resolution timers with only
>> per-cpu local timers - regardless if they have the
>> C3STOP flag set or not.
>>
>> Observed on the r8a73a4 SoC that at this point only uses
>> ARM architected timers for clock event and clock sources.
>>
>> Without this patch high resolution timers are run time
>> disabled on the r8a73a4 SoC - this regardless of CPUIdle
>> is disabled or not.
>>
>> The less short term fix is to add support for more timers
>> on the r8a73a4 SoC, but until CPUIdle support is enabled
>> it must be possible to use high resoultion timers without
>> additional timers.
>>
>> I'd like to hear some feedback and also test this on more
>> systems before merging the code, see the non-SOB below.
>>
>> Not-Yet-Signed-off-by: Magnus Damm <damm@...nsource.se>
>> ---
>>
>> An earlier ARM arch timer specific version of this patch was
>> posted yesterday as:
>> "[PATCH/RFC] arm: arch_timer: Do not set C3STOP in case CPU_IDLE=n"
>>
>> Many thanks to Mark Rutland for his kind feedback.
>>
>> kernel/time/tick-broadcast.c | 8 ++++----
>> kernel/time/tick-common.c | 2 +-
>> kernel/time/tick-internal.h | 11 +++++++++++
>> 3 files changed, 16 insertions(+), 5 deletions(-)
>>
>> --- 0001/kernel/time/tick-broadcast.c
>> +++ work/kernel/time/tick-broadcast.c 2013-06-18 15:36:21.000000000 +0900
>> @@ -71,7 +71,7 @@ int tick_check_broadcast_device(struct c
>> if ((dev->features & CLOCK_EVT_FEAT_DUMMY) ||
>> (tick_broadcast_device.evtdev &&
>> tick_broadcast_device.evtdev->rating >= dev->rating) ||
>> - (dev->features & CLOCK_EVT_FEAT_C3STOP))
>> + tick_device_may_c3stop(dev))
>> return 0;
>>
>> clockevents_exchange_device(tick_broadcast_device.evtdev, dev);
>> @@ -146,7 +146,7 @@ int tick_device_uses_broadcast(struct cl
>> * feature and the cpu is marked in the broadcast mask
>> * then clear the broadcast bit.
>> */
>> - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) {
>> + if (!tick_device_may_c3stop(dev)) {
>> int cpu = smp_processor_id();
>> cpumask_clear_cpu(cpu, tick_broadcast_mask);
>> tick_broadcast_clear_oneshot(cpu);
>> @@ -270,7 +270,7 @@ static void tick_do_broadcast_on_off(uns
>> /*
>> * Is the device not affected by the powerstate ?
>> */
>> - if (!dev || !(dev->features & CLOCK_EVT_FEAT_C3STOP))
>> + if (!dev || !tick_device_may_c3stop(dev))
>> goto out;
>>
>> if (!tick_device_is_functional(dev))
>> @@ -568,7 +568,7 @@ void tick_broadcast_oneshot_control(unsi
>> td = &per_cpu(tick_cpu_device, cpu);
>> dev = td->evtdev;
>>
>> - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP))
>> + if (!tick_device_may_c3stop(dev))
>> return;
>>
>> bc = tick_broadcast_device.evtdev;
>> --- 0001/kernel/time/tick-common.c
>> +++ work/kernel/time/tick-common.c 2013-06-18 15:36:29.000000000 +0900
>> @@ -52,7 +52,7 @@ int tick_is_oneshot_available(void)
>>
>> if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT))
>> return 0;
>> - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP))
>> + if (!tick_device_may_c3stop(dev))
>> return 1;
>> return tick_broadcast_oneshot_available();
>> }
>> --- 0001/kernel/time/tick-internal.h
>> +++ work/kernel/time/tick-internal.h 2013-06-18 15:40:10.000000000 +0900
>> @@ -141,6 +141,17 @@ static inline int tick_device_is_functio
>> return !(dev->features & CLOCK_EVT_FEAT_DUMMY);
>> }
>>
>> +/*
>> + * Check, if the device has C3STOP behavior and CPU Idle is enabled
>> + */
>> +static inline bool tick_device_may_c3stop(struct clock_event_device *dev)
>
> I prefer tick_device_is_reliable(struct clock_event_device *dev).
Sure. I took the name from the flag, thought that made it easy to follow.
I wonder what the timekeeping maintainers prefer?
>> +{
>> + /* The C3 sleep mode can only trigger when CPU Idle is enabled,
>> + * so if CPU Idle is disabled then the C3STOP flag can be ignored */
>> + return (IS_ENABLED(CONFIG_CPU_IDLE) &&
>> + (dev->features & CLOCK_EVT_FEAT_C3STOP));
>> +}
>
> Preferably you may use the format:
>
> #ifdef CONFIG_CPU_IDLE
> static inline bool tick_device_is_reliable(struct clock_event_device *dev)
> {
> return dev->features & CLOCK_EVT_FEAT_C3STOP;
> }
> #else
> static inline bool tick_device_is_reliable(struct clock_event_device *dev)
> {
> return true;
> }
> #endif
>
> to conform the header style format already present in the file.
I agree with you about following the same style. Actually, I wrote
the code to follow the code right above the function, but I decided to
return bool instead of int. I don't mind so much in general though,
except trying to keep the code at least half well-commented and
relatively compact.
So regarding stylistic things, sure, we can move around things.
Question is just if this is acceptable or not. =)
Thanks,
/ magnus
--
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/
Powered by blists - more mailing lists