linux-kernel - Re: [PATCHv3/RESEND 4/4] clocksource: arch_timer: Add support for memory mapped timers

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: <51C4DB44.7030909@codeaurora.org>
Date:	Fri, 21 Jun 2013 16:01:24 -0700
From:	Stephen Boyd <sboyd@...eaurora.org>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	Daniel Lezcano <daniel.lezcano@...aro.org>,
	linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	John Stultz <john.stultz@...aro.org>,
	Mark Rutland <mark.rutland@....com>,
	Marc Zyngier <Marc.Zyngier@....com>,
	Rob Herring <robherring2@...il.com>
Subject: Re: [PATCHv3/RESEND 4/4] clocksource: arch_timer: Add support for
 memory mapped timers

On 06/21/13 14:18, Thomas Gleixner wrote:
> On Fri, 21 Jun 2013, Stephen Boyd wrote:
>>  /*
>>   * Architected system timer support.
>> @@ -46,13 +73,69 @@ static bool arch_timer_use_virtual = true;
>>  static inline void arch_timer_reg_write(int access, int reg, u32 val,
>>  					struct clock_event_device *clk)
>>  {
>> -	arch_timer_reg_write_cp15(access, reg, val);
>> +	if (access == ARCH_TIMER_MEM_PHYS_ACCESS) {
>> +		struct arch_timer *timer = to_arch_timer(clk);
>> +		switch (reg) {
>> +		case ARCH_TIMER_REG_CTRL:
>> +			writel_relaxed(val, timer->base + CNTP_CTL);
>> +			break;
>> +		case ARCH_TIMER_REG_TVAL:
>> +			writel_relaxed(val, timer->base + CNTP_TVAL);
>> +			break;
>> +		default:
>> +			BUILD_BUG();
> So you are relying on the compiler cleverness to identify a caller
> which calls that inline with a not supported reg value.
>
> How does that work, when the compiler decides not to inline that?
>
> enum without a default case emits at least a reliable warning.

Right now arm and arm64 don't allow inline to be anything besides
always_inline so the compiler is forced to inline. But point taken, I'll
remove the BUILD_BUG and make the register argument into an enum (which
it isn't right now).

>
>> +		}
>> +	} else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
>> +		struct arch_timer *timer = to_arch_timer(clk);
>> +		switch (reg) {
>> +		case ARCH_TIMER_REG_CTRL:
>> +			writel_relaxed(val, timer->base + CNTV_CTL);
>> +			break;
>> +		case ARCH_TIMER_REG_TVAL:
>> +			writel_relaxed(val, timer->base + CNTV_TVAL);
>> +			break;
>> +		default:
>> +			BUILD_BUG();
>> +		}
>> +	} else {
>> +		arch_timer_reg_write_cp15(access, reg, val);
>> +	}
>>  }
> Something in my little brain yells: function pointer
>
> You can't be serious about hacking nested if/else/switch constructs
> into a hot path.
>
> Why not making your cpu data:
>
> struct arch_timer {
>        struct clock_event_device evt;
>        ....
>        void (*write_ctrl)(val, timer);
>        void (*write_tval)(val, timer);
>        ....
> }
>
> and get rid of all that conditionals?

It sounds like that's undesirable according to the comment above
arch_timer_reg_write(). It seems that all this code was written under
the assumption that the compiler is good enough to optimize all the code
paths and only generate the code that is necessary. So far this seems to
be working and the hotpath is optimized for each type of access.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

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