linux-kernel - Re: [PATCH v14 7/9] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT

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: <CADyBb7vGU1cZVF4-NZR7OVXQfwvr3Ee32W-hxr52nwHifmTSRg@mail.gmail.com>
Date:   Thu, 27 Oct 2016 00:07:00 +0800
From:   Fu Wei <fu.wei@...aro.org>
To:     Mark Rutland <mark.rutland@....com>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <marc.zyngier@....com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Sudeep Holla <sudeep.holla@....com>,
        Hanjun Guo <hanjun.guo@...aro.org>,
        linux-arm-kernel@...ts.infradead.org,
        Linaro ACPI Mailman List <linaro-acpi@...ts.linaro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        rruigrok@...eaurora.org, "Abdulhamid, Harb" <harba@...eaurora.org>,
        Christopher Covington <cov@...eaurora.org>,
        Timur Tabi <timur@...eaurora.org>,
        G Gregory <graeme.gregory@...aro.org>,
        Al Stone <al.stone@...aro.org>, Jon Masters <jcm@...hat.com>,
        Wei Huang <wei@...hat.com>, Arnd Bergmann <arnd@...db.de>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>,
        Leo Duran <leo.duran@....com>,
        Wim Van Sebroeck <wim@...ana.be>,
        Guenter Roeck <linux@...ck-us.net>,
        linux-watchdog@...r.kernel.org, Tomasz Nowicki <tn@...ihalf.com>,
        Christoffer Dall <christoffer.dall@...aro.org>,
        Julien Grall <julien.grall@....com>
Subject: Re: [PATCH v14 7/9] clocksource/drivers/arm_arch_timer: Refactor the
 timer init code to prepare for GTDT

Hi Mark,

On 26 October 2016 at 23:46, Mark Rutland <mark.rutland@....com> wrote:
> On Wed, Oct 26, 2016 at 11:24:32PM +0800, Fu Wei wrote:
>> On 21 October 2016 at 19:32, Mark Rutland <mark.rutland@....com> wrote:
>> > On Thu, Sep 29, 2016 at 02:17:15AM +0800, fu.wei@...aro.org wrote:
>> >> +static int __init arch_timer_mem_register(struct device_node *np, void *frame)
>> >>  {
>> >> -     int ret;
>> >> -     irq_handler_t func;
>> >> +     struct device_node *frame_node = NULL;
>> >>       struct arch_timer *t;
>> >> +     void __iomem *base;
>> >> +     irq_handler_t func;
>> >> +     unsigned int irq;
>> >> +     int ret;
>> >> +
>> >> +     if (!frame)
>> >> +             return -EINVAL;
>> >
>> > Why would we call this without a frame?
>>
>> Sorry, I just verify it , make sure frame is not NULL,
>> Because it is a "static" function, so we do need this check?
>
> I'd rather we simply don't call the function rather than passing a NULL
> frame in.
>

OK,  NP, will do

>> >> +
>> >> +     if (np) {
>> >
>> > ... or without a node?
>>
>> For "np", for now, we just  just verify it, but it is just paperation
>> for GTDT support,
>> Because in next patch, if np == NULL, that means we call this function
>> from GTDT, but not DT.
>
> Please don't do that. More on that below.
>
>> > Please as Marc requested several versions ago: split the FW parsing
>> > (ACPI and DT) so that happens first, *then* once we have the data in a
>> > common format, use that to drive poking the HW, requesting IRQs, etc,
>> > completely independent of the source.
>> >
>> > In patches, do this by:
>> >
>> > (1) adding the data structures
>> > (2) splitting the existing DT probing to use them
>> > (3) Adding ACPI functionality atop
>>
>> this patch is a preparation for GTDT support, I have splitted some
>> functions for reusing them in next patch(GTDT support)
>>
>> if np == NULL, that means we call this function from GTDT, but
>> if np != NULL, that means we call this function from DT
>
> As above, please structure the patches such that that never happens.
>
> We currently have:
>
> +--------------------------+
> | DT Parsing + Common code |
> +--------------------------+
>
> Per (1 and 2) make this:
>
> +------------+                       +-------------+
> | DT parsing |--(common structure)-->| Common code |
> +------------+                       +-------------+
>
> Then per (3) make this:
>
> +------------+
> | DT parsing |--(common structure)------+
> +------------+                          |
>                                         v
>                                     +-------------+
>                                     | Common code |
>                                     +-------------+
>                                         ^
> +--------------+                        |
> | ACPI parsing |--(common structure)----+

Thanks for your suggestion , I will do this way in my next patchset

Thanks :-)


> +--------------+
>
> Thanks,
> Mark.



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat