linux-kernel - Re: [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC

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:	Wed, 1 Jun 2016 09:58:08 +0800
From:	"Huang, Tao" <huangtao@...k-chips.com>
To:	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Caesar Wang <wxt@...k-chips.com>,
	Heiko Stuebner <heiko@...ech.de>
Cc:	dianders@...omium.org, briannorris@...gle.com, smbarber@...gle.com,
	linux-rockchip@...ts.infradead.org,
	Thomas Gleixner <tglx@...utronix.de>, cf@...k-chips.com,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC

Hi Daniel:
On 2016年05月31日 22:06, Daniel Lezcano wrote:
>>
>> @@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct
>> clock_event_device *ce)
>>   	return rk_timer(ce)->base;
>>   }
>>
>> +static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
>> +{
>> +	return rk_timer(ce)->base + rk_timer(ce)->ctrl;
> 
> You can do a small optimization by pre-computing 'ctrl' at init time, so 
> no need to do this addition each time.

I understand what you mean, please see comment below.
And even we use ctrl as pointer, we still will get addition LDR other
then ADD.
This is disassemble code before:
   0:	f9408021 	ldr	x1, [x1,#256]
   4:	52800003 	mov	w3, #0x0
   8:	91004022 	add	x2, x1, #0x10
   c:	b9000043 	str	w3, [x2]
This is disassemble code after change:
   0:	52800003 	mov	w3, #0x0
   4:	f9408422 	ldr	x2, [x1,#264]
   8:	b9000043 	str	w3, [x2]
   c:	f9408021 	ldr	x1, [x1,#256]
Of course we can assume cache hit.

> 
>> +}
>> +
>>   static inline void rk_timer_disable(struct clock_event_device *ce)
>>   {
>> -	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
>> +	writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
>>   }
>>
>>   static inline void rk_timer_enable(struct clock_event_device *ce, u32
>> flags)
>>   {
>>   	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
>> -		       rk_base(ce) + TIMER_CONTROL_REG);
>> +		       rk_ctrl(ce));
>>   }
>>
>>   static void rk_timer_update_counter(unsigned long cycles,
>> @@ -179,4 +186,19 @@ out_unmap:
>>   	iounmap(bc_timer.base);
>>   }
>>
>> -CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
>> +static void __init rk3288_timer_init(struct device_node *np)
>> +{
>> +	bc_timer.ctrl = TIMER_CONTROL_REG3288;
>> +	rk_timer_init(np);
> 
> 	rk_timer_init(np);
> 	bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3288;

No. It's not such simple. You will access null pointer when
rk_timer_init, if we keep rk_timer_disable call in init or after
request_irq/clockevents_config_and_register and interrupt happen
immediately.

So the code maybe:
static void __init rk3288_timer_init(struct device_node *np)
{
	bc_timer.base = of_iomap(np, 0);
	if (!bc_timer.base) {
		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
		return;
	}
	bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3288;
	rk_imter_init(np); // of course remove of_iomap from init.

Is this what you want?