| 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
| ||
|
Date: Fri, 7 Apr 2017 01:39:09 +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 v23 09/11] acpi/arm64: Add memory-mapped timer support in
GTDT driver
Hi Mark
On 7 April 2017 at 01:24, Mark Rutland <mark.rutland@....com> wrote:
> On Fri, Apr 07, 2017 at 12:47:47AM +0800, Fu Wei wrote:
>> On 6 April 2017 at 02:38, Mark Rutland <mark.rutland@....com> wrote:
>> > On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu.wei@...aro.org wrote:
>> >> + /*
>> >> + * Get the GT timer Frame data for every GT Block Timer
>> >> + */
>> >> + for (i = 0; i < block->timer_count; i++, gtdt_frame++) {
>> >> + if (gtdt_frame->common_flags & ACPI_GTDT_GT_IS_SECURE_TIMER)
>> >> + continue;
>> >> +
>> >> + if (!gtdt_frame->base_address || !gtdt_frame->timer_interrupt)
>> >> + goto error;
>> >> +
>> >> + frame = &timer_mem->frame[gtdt_frame->frame_number];
>> >> + frame->phys_irq = map_gt_gsi(gtdt_frame->timer_interrupt,
>> >> + gtdt_frame->timer_flags);
>> >> + if (frame->phys_irq <= 0) {
>> >> + pr_warn("failed to map physical timer irq in frame %d.\n",
>> >> + gtdt_frame->frame_number);
>> >> + goto error;
>> >> + }
>> >> +
>> >> + if (gtdt_frame->virtual_timer_interrupt) {
>> >> + frame->virt_irq =
>> >> + map_gt_gsi(gtdt_frame->virtual_timer_interrupt,
>> >> + gtdt_frame->virtual_timer_flags);
>> >> + if (frame->virt_irq <= 0) {
>> >> + pr_warn("failed to map virtual timer irq in frame %d.\n",
>> >> + gtdt_frame->frame_number);
>> >> + acpi_unregister_gsi(gtdt_frame->timer_interrupt);
>> >> + goto error;
>> >> + }
>> >> + } else {
>> >> + frame->virt_irq = 0;
>> >> + pr_debug("virtual timer in frame %d not implemented.\n",
>> >> + gtdt_frame->frame_number);
>> >> + }
>> >> +
>> >> + frame->cntbase = gtdt_frame->base_address;
>> >> + /*
>> >> + * The CNTBaseN frame is 4KB (register offsets 0x000 - 0xFFC).
>> >> + * See ARM DDI 0487A.k_iss10775, page I1-5130, Table I1-4
>> >> + * "CNTBaseN memory map".
>> >> + */
>> >> + frame->size = SZ_4K;
>> >> + frame->valid = true;
>> >> + }
>> >> +
>> >> + return 0;
>> >> +
>> >> +error:
>> >> + for (i = 0; i < ARCH_TIMER_MEM_MAX_FRAMES; i++) {
>> >> + frame = &timer_mem->frame[i];
>> >> + if (!frame->valid)
>> >> + continue;
>> >> + irq_dispose_mapping(frame->phys_irq);
>> >> + if (frame->virt_irq)
>> >> + irq_dispose_mapping(frame->virt_irq);
>> >> + }
>> >
>> > We assign interrupts and may goto error before setting valid, so here we
>>
>> yes, I mean to do it.(setting valid at the end of loop)
>>
>> > won't free the interrupts of the last frame we parsed.
>>
>> that won't be a problem, we may assign two interrupts in a round:
>> First of all, if the assignment goes wrong, that means the current
>> interrupt haven't been successfully assigned.
>> (1)if the first goes wrong, the we goto error to unwind the irqs
>> assigned in previous rounds.
>> (2)if the second one goes wrong , we acpi_unregister_gsi the first one
>> and then goto error to unwind the irqs assigned in previous rounds.
>> (3)If the two assignments are successful, set up valid flag
>>
>> So we won't miss freeing the interrupts of the last frame we parsed.
>>
>> Did I miss something?
>
> No; you are correct, and I was mistaken.
>
> However, I would prefer to simplify this such that we only free the
> IRQs in the error path.
>
> We should be able to iterate over all freams, freeing any non-zero
> interrupt, since !valid frames shouldn't have non-zero interrupts.
Yes, that is what I am doing :
if (!frame->valid)
continue;
phys_irq must be non-zero, otherwise it was registered incorrectly (an error)
but we need to check virt_irq, it maybe 0 because, this timer frame
may not implement virt timer.
Can we simplify it? any idear?
Lorenzo addressed the API issue, we may can fix it by getting GSI info
from DT, then register it until we figure the best frame.
It may need some big change in DT code
I can do it in V24 , any thought?
Great thanks for your review and help! :-)
>
> I can make that update locally; no need to respin.
>
> Thanks,
> Mark.
--
Best regards,
Fu Wei
Software Engineer
Red Hat
Powered by blists - more mailing lists