linux-kernel - Re: [PATCH v6 2/2] clocksource: Add Intel Keem Bay timer 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: <f8af5711-4cc4-7513-5ad3-0f17ce04b346@linaro.org>
Date:   Thu, 23 Dec 2021 15:16:00 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     "Sanil, Shruthi" <shruthi.sanil@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Cc:     "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>,
        "kris.pan@...ux.intel.com" <kris.pan@...ux.intel.com>,
        "mgross@...ux.intel.com" <mgross@...ux.intel.com>,
        "Thokala, Srikanth" <srikanth.thokala@...el.com>,
        "Raja Subramanian, Lakshmi Bai" 
        <lakshmi.bai.raja.subramanian@...el.com>,
        "Sangannavar, Mallikarjunappa" 
        <mallikarjunappa.sangannavar@...el.com>
Subject: Re: [PATCH v6 2/2] clocksource: Add Intel Keem Bay timer support

On 11/11/2021 11:42, Sanil, Shruthi wrote:
>> -----Original Message-----
>> From: Thomas Gleixner <tglx@...utronix.de>
>> Sent: Monday, September 27, 2021 3:11 AM
>> To: Sanil, Shruthi <shruthi.sanil@...el.com>; daniel.lezcano@...aro.org;
>> robh+dt@...nel.org; linux-kernel@...r.kernel.org;
>> devicetree@...r.kernel.org
>> Cc: andriy.shevchenko@...ux.intel.com; kris.pan@...ux.intel.com;
>> mgross@...ux.intel.com; Thokala, Srikanth <srikanth.thokala@...el.com>;
>> Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@...el.com>;
>> Sangannavar, Mallikarjunappa <mallikarjunappa.sangannavar@...el.com>;
>> Sanil, Shruthi <shruthi.sanil@...el.com>
>> Subject: Re: [PATCH v6 2/2] clocksource: Add Intel Keem Bay timer support
>>
>> On Tue, Sep 07 2021 at 00:06, shruthi sanil wrote:
>>> +
>>> +/* Provides a unique ID for each timer */ static
>>> +DEFINE_IDA(keembay_timer_ida);
>>
>>> +
>>> +	timer_id = ida_alloc(&keembay_timer_ida, GFP_KERNEL);
>>> +	if (timer_id < 0) {
>>> +		ret = timer_id;
>>> +		goto err_keembay_ce_to_free;
>>> +	}
>>
>> May I ask what the purpose of the IDA, which is backed by a full blown
>> xarray, is here?
>>
>> AFAICT all you want is a unique number for the timer name for up to 8
>> timers.
>>
>>> +	timer_name = kasprintf(GFP_KERNEL, "keembay_timer%d",
>> timer_id);
>>
>> So what's wrong about:
>>
>> static unsigned int keembay_timer_id;
>>
>> 	timer_name = kasprintf(GFP_KERNEL, "keembay_timer%d",
>> keembay_timer_id++);
>>
>> Hmm?
> 
> Yes, we had initially implemented it in the similar way, 
> but in the course of review it got changed to use IDA.
> 
>>
>>> +
>>> +	clockevents_config_and_register(&keembay_ce_to->clkevt,
>>> +					timer_of_rate(keembay_ce_to),
>>> +					1,
>>> +					U32_MAX);
>>
>> Aside of that what's the point of registering more than one of those timers as
>> clock event? The core will only use one and the rest is just going to use
>> memory for no value.
> 
> Instead of
> keembay_ce_to->clkevt.cpumask = cpumask_of(0); 
> can I update it as 
> keembay_ce_to->clkevt.cpumask = cpu_possible_mask; 
> so that each timer would be associated with different cores?

Let me try to clarify:

The Intel Keem bay Soc is a 4 x Cortex-A53

The arch ARM timer is per CPU on this platform.

Case 1:
-------
 - the architected timer is not desired and this timer is wanted to be
used instead (but rating tells the opposite) => rewrite per cpu code

Case 2:
-------
 - the architected timer are desired and this timer is used as a
broadcast timer when a core is going done with cpuidle. One timer is needed.

 - In order to prevent useless wakeup, the timer uses the flag DYNIRQ.
However, cpumask_of(0) is set and makes inoperative this flag. In order
to make full use of it, clkevt.cpumask must be cpu_possible_mask

Hope that helps

  -- Daniel








-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog