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: <alpine.DEB.2.02.1306212304480.4013@ionos.tec.linutronix.de>
Date:	Fri, 21 Jun 2013 23:18:26 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Stephen Boyd <sboyd@...eaurora.org>
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 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.

> +		}
> +	} 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?

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/