linux-kernel - Re: [PATCH v16 11/15] acpi/arm64: Add GTDT table parse driver

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: <CADyBb7s8sg1RRC1xa7GD3r4b5gGzNHotpxrCfH3b-CEsrK_b4g@mail.gmail.com>
Date:   Wed, 23 Nov 2016 20:06:14 +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 v16 11/15] acpi/arm64: Add GTDT table parse driver

Hi Mark,


On 19 November 2016 at 04:12, Mark Rutland <mark.rutland@....com> wrote:
> On Wed, Nov 16, 2016 at 09:49:04PM +0800, fu.wei@...aro.org wrote:
>
>> +#define for_each_platform_timer(_g) for (; _g; _g = next_platform_timer(_g))
>
> This doesn't fit the usual for_each_* pattern, since _g has to be
> manually initialised first. Either come up with a way of maknig this fit
> the usual pattern, or get rid of this, and use:
>
>         t = however_you_get_the_first_timer();
>
>         if (!t)
>                 bailt_out_somehow();
>
>         do {
>                 ...
>         } while (t = next_platform_timer(t));

Thanks, will do

>
>> +/*
>> + * Release the memory we have allocated in acpi_gtdt_init.
>> + * This should be called, when the driver who called "acpi_gtdt_init" previously
>> + * doesn't need the GTDT info anymore.
>> + */
>> +void __init acpi_gtdt_release(void)
>> +{
>> +     kfree(timer_block);
>> +     kfree(watchdog);
>> +     timer_block = NULL;
>> +     watchdog = NULL;
>> +}
>
> Why is this not simply in the error path of acpi_gtdt_init()?
>
>> +
>> +/*
>> + * Get some basic info from GTDT table, and init the global variables above
>> + * for all timers initialization of Generic Timer.
>> + * This function does some validation on GTDT table.
>> + */
>> +int __init acpi_gtdt_init(struct acpi_table_header *table)
>> +{
>
>> +     timer_block = kcalloc(timer_count,
>> +                           sizeof(struct acpi_gtdt_timer_block *),
>> +                           GFP_KERNEL);
>> +     if (!timer_block)
>> +             return -ENOMEM;
>> +
>> +     watchdog = kcalloc(timer_count, sizeof(struct acpi_gtdt_watchdog *),
>> +                        GFP_KERNEL);
>> +     if (!watchdog) {
>> +             kfree(timer_block);
>> +             timer_block = NULL;
>> +             return -ENOMEM;
>> +     }
>
> Please have a common error path below, and branch to that when you need
> to free these.

OK , will do

>
>> +error:
>> +     acpi_gtdt_release();
>> +     return -EINVAL;
>> +}
>
> [...]
>
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 61a3d90..a1611d1 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -577,6 +577,13 @@ enum acpi_reconfig_event  {
>>  int acpi_reconfig_notifier_register(struct notifier_block *nb);
>>  int acpi_reconfig_notifier_unregister(struct notifier_block *nb);
>>
>> +#ifdef CONFIG_ACPI_GTDT
>> +int acpi_gtdt_init(struct acpi_table_header *table);
>> +int acpi_gtdt_map_ppi(int type);
>> +bool acpi_gtdt_c3stop(int type);
>> +void acpi_gtdt_release(void);
>
> Why do these need to be here?
>
> What possible value is ther in exporting acpi_gtdt_release() !?

because I need to release these after mem timer init,
But like your comment in 14/15, if I fill in the entire arch_timer_mem
in one go, I don't need to export acpi_gtdt_release.

Will try this way in v17

>
> Thanks,
> Mark.



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat