| 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
| ||
|
Message-ID: <CADyBb7uuYrKu6=3kQm0HbqSHZocW7YBooh+PRGB7_rADyfGEkg@mail.gmail.com>
Date: Tue, 26 Jul 2016 00:06:10 +0800
From: Fu Wei <fu.wei@...aro.org>
To: Lorenzo Pieralisi <lorenzo.pieralisi@....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>,
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, 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@...hat.com, Arnd Bergmann <arnd@...db.de>,
Wim Van Sebroeck <wim@...ana.be>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will.deacon@....com>,
Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>,
Leo Duran <leo.duran@....com>,
Guenter Roeck <linux@...ck-us.net>,
linux-watchdog@...r.kernel.org
Subject: Re: [PATCH v8 7/9] acpi/arm64: Add memory-mapped timer support in
GTDT driver
Hi Lorenzo,
On 21 July 2016 at 20:40, Lorenzo Pieralisi <lorenzo.pieralisi@....com> wrote:
> On Wed, Jul 20, 2016 at 02:18:02AM +0800, fu.wei@...aro.org wrote:
>> From: Fu Wei <fu.wei@...aro.org>
>>
>> This driver adds support for parsing memory-mapped timer in GTDT:
>> provide a kernel APIs to parse GT Block Structure in GTDT,
>> export all the info by filling the struct which provided
>> by parameter(pointer of the struct).
>>
>> By this driver, we can add ACPI support for memory-mapped timer in
>> arm_arch_timer drivers, and separate the ACPI GTDT knowledge from it.
>
> "On platforms booting with ACPI, architected memory timers configuration
> data is provided by firmware through the ACPI GTDT static table.
>
> The clocksource architected timer kernel driver requires a firmware
> interface to collect timer configuration and configure its driver;
> this infrastructure is present for device tree systems but it is
> missing on systems booting with ACPI.
>
> Implement the kernel infrastructure required to parse the static
> ACPI GTDT table so that the architected timer clocksource driver can
> make use of it on systems booting with ACPI, therefore enabling
> the corresponding timers configuration."
Thanks , I have used it in my v9 patchset
>
>> Signed-off-by: Fu Wei <fu.wei@...aro.org>
>> Signed-off-by: Hanjun Guo <hanjun.guo@...aro.org>
>> ---
>> drivers/acpi/arm64/acpi_gtdt.c | 72 ++++++++++++++++++++++++++++++++++++
>> include/clocksource/arm_arch_timer.h | 15 ++++++++
>> include/linux/acpi.h | 1 +
>> 3 files changed, 88 insertions(+)
>>
>> diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c
>> index e1cfc9e..b48e443 100644
>> --- a/drivers/acpi/arm64/acpi_gtdt.c
>> +++ b/drivers/acpi/arm64/acpi_gtdt.c
>> @@ -150,3 +150,75 @@ int __init acpi_gtdt_init(struct acpi_table_header *table)
>>
>> return gtdt->platform_timer_count;
>> }
>> +
>> +static int __init gtdt_parse_gt_block(void *platform_timer,
>> + struct gt_block_data *data)
>> +{
>
> Nit: you can directly pass a struct acpi_gtdt_timer_block* pointer.
good idea, :-)
>
>> + struct acpi_gtdt_timer_block *block = platform_timer;
>> + struct acpi_gtdt_timer_entry *frame;
>> + int i;
>> +
>> + if (!block || !data)
>> + return -EINVAL;
>> +
>> + if (!block->block_address || !block->timer_count)
>> + goto error;
>
> Here we jump to print an error and bail out. The only reason
> why we need a goto is to print a common error message and that's
> exactly what we do _not_ need, if there is a FW error we want
> to understand which one is that, otherwise the information you
> are printing is useless for debugging (provided we can make use
> of this info, which is questionable anyway).
yes, I have fixed in my v9
>
>> + data->cntctlbase_phy = (phys_addr_t)block->block_address;
>> + data->timer_count = block->timer_count;
>> +
>> + frame = (void *)block + block->timer_offset;
>> + if (frame + block->timer_count != (void *)block + block->header.length)
>> + goto error;
>
> See above.
>
>> + /*
>> + * Get the GT timer Frame data for every GT Block Timer
>> + */
>> + for (i = 0; i < block->timer_count; i++) {
>> + frame += i;
>
> This is wrong, if you have multiple frames, pointers would add up and
> that's not correct.
Great thanks for pointing it out, this is a bug, I have fixed it in my v9.
Thanks for your help
>
>> + if (!frame->base_address || !frame->timer_interrupt)
>> + goto error;
>
> See above.
>
>> + data->timer[i].frame_nr = frame->frame_number;
>> + data->timer[i].cntbase_phy = frame->base_address;
>> + data->timer[i].irq =
>> + map_generic_timer_interrupt(frame->timer_interrupt,
>> + frame->timer_flags);
>
> AFAIK map_generic_timer_interrupt() may fail, you should cater for that.
>
>> + if (frame->virtual_timer_interrupt)
>> + data->timer[i].virt_irq =
>> + map_generic_timer_interrupt(
>> + frame->virtual_timer_interrupt,
>> + frame->virtual_timer_flags);
>
> Ditto.
>
>> + }
>> + return 0;
>> +
>> +error:
>> + pr_err(FW_BUG "invalid GT Block data.\n");
>> + return -EINVAL;
>
> See above.
>
>> +}
>> +
>> +/*
>> + * Get the GT block info for memory-mapped timer from GTDT table.
>> + * Please make sure we have called acpi_gtdt_init, because it helps to
>> + * init the global variables.
>
> I already commented on this. I think the second part of this comment
> is useless (it is a function with one caller, make sure you respect
> the calling sequence and be done with this).
I have deleted the second part in my v9 , thanks :-)
>
> Thanks,
> Lorenzo
>
>> + */
>> +int __init gtdt_arch_timer_mem_init(struct gt_block_data *data)
>> +{
>> + void *platform_timer;
>> + int index = 0;
>> + int ret;
>> +
>> + for_each_platform_timer(platform_timer) {
>> + if (!is_timer_block(platform_timer))
>> + continue;
>> + ret = gtdt_parse_gt_block(platform_timer, data + index);
>> + if (ret)
>> + return ret;
>> + index++;
>> + }
>> +
>> + if (index)
>> + pr_info("found %d memory-mapped timer block(s).\n", index);
>> +
>> + return index;
>> +}
>> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
>> index 16dcd10..ece6b3b 100644
>> --- a/include/clocksource/arm_arch_timer.h
>> +++ b/include/clocksource/arm_arch_timer.h
>> @@ -56,6 +56,8 @@ enum spi_nr {
>> #define ARCH_TIMER_MEM_PHYS_ACCESS 2
>> #define ARCH_TIMER_MEM_VIRT_ACCESS 3
>>
>> +#define ARCH_TIMER_MEM_MAX_FRAME 8
>> +
>> #define ARCH_TIMER_USR_PCT_ACCESS_EN (1 << 0) /* physical counter */
>> #define ARCH_TIMER_USR_VCT_ACCESS_EN (1 << 1) /* virtual counter */
>> #define ARCH_TIMER_VIRT_EVT_EN (1 << 2)
>> @@ -71,6 +73,19 @@ struct arch_timer_kvm_info {
>> int virtual_irq;
>> };
>>
>> +struct gt_timer_data {
>> + int frame_nr;
>> + phys_addr_t cntbase_phy;
>> + int irq;
>> + int virt_irq;
>> +};
>> +
>> +struct gt_block_data {
>> + phys_addr_t cntctlbase_phy;
>> + int timer_count;
>> + struct gt_timer_data timer[ARCH_TIMER_MEM_MAX_FRAME];
>> +};
>> +
>> #ifdef CONFIG_ARM_ARCH_TIMER
>>
>> extern u32 arch_timer_get_rate(void);
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index fb8b689..24b1750 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -536,6 +536,7 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *);
>> int acpi_gtdt_init(struct acpi_table_header *table);
>> int acpi_gtdt_map_ppi(int type);
>> bool acpi_gtdt_c3stop(void);
>> +int gtdt_arch_timer_mem_init(struct gt_block_data *data);
>> #endif
>>
>> #else /* !CONFIG_ACPI */
>> --
>> 2.5.5
>>
--
Best regards,
Fu Wei
Software Engineer
Red Hat
Powered by blists - more mailing lists