Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Fu Wei]

Hi Mark,

On 7 April 2017 at 01:52, Mark Rutland  wrote:
> On Fri, Apr 07, 2017 at 01:39:09AM +0800, Fu Wei wrote:
>> On 7 April 2017 at 01:24, Mark Rutland  wrote:
>> > On Fri, Apr 07, 2017 at 12:47:47AM +0800, Fu Wei wrote:
>> >> On 6 April 2017 at 02:38, Mark Rutland  wrote:
>> >> > On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.org wrote:
>
>> > 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;
>
> What I meant was that we won't look at the frame->valid flag at all;
> only the interrupts. e.g.
>
> for (int i = 0; i < ARCH_TIMER_MEM_MAX_FRAMES; i++) {
> if (frame->phys_irq > 0)
> free_the_phys_irq_somehow();
> if (frame->virt_irq > 0)
> free_the_virt_irq_somehow();
> }

Since we use "kcalloc" to allocate struct arch_timer_mem, this should be OK.

>
> ... where we somehow figure out the GSI, or we introduce an api like
> unregister_gsi_for_irq(irq).

Yes, If you are OK with introducing a new API , this problem is solved :-)

>
> Since the !valid frames should all have zero for their interrupt fields,
> no special handling is necessary.
>
> That way, we only free the IRQs in one place, it's obvious that we
> consistently free all of them, etc.
>
>> 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'd prefer to keep this constrained to the ACPI code. ;)
>
> Thanks,
> Mark.



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Fu Wei]

Hi Mark,

On 7 April 2017 at 01:52, Mark Rutland  wrote:
> On Fri, Apr 07, 2017 at 01:39:09AM +0800, Fu Wei wrote:
>> On 7 April 2017 at 01:24, Mark Rutland  wrote:
>> > On Fri, Apr 07, 2017 at 12:47:47AM +0800, Fu Wei wrote:
>> >> On 6 April 2017 at 02:38, Mark Rutland  wrote:
>> >> > On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.org wrote:
>
>> > 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;
>
> What I meant was that we won't look at the frame->valid flag at all;
> only the interrupts. e.g.
>
> for (int i = 0; i < ARCH_TIMER_MEM_MAX_FRAMES; i++) {
> if (frame->phys_irq > 0)
> free_the_phys_irq_somehow();
> if (frame->virt_irq > 0)
> free_the_virt_irq_somehow();
> }

Since we use "kcalloc" to allocate struct arch_timer_mem, this should be OK.

>
> ... where we somehow figure out the GSI, or we introduce an api like
> unregister_gsi_for_irq(irq).

Yes, If you are OK with introducing a new API , this problem is solved :-)

>
> Since the !valid frames should all have zero for their interrupt fields,
> no special handling is necessary.
>
> That way, we only free the IRQs in one place, it's obvious that we
> consistently free all of them, etc.
>
>> 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'd prefer to keep this constrained to the ACPI code. ;)
>
> Thanks,
> Mark.



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Mark Rutland]

On Fri, Apr 07, 2017 at 01:39:09AM +0800, Fu Wei wrote:
> On 7 April 2017 at 01:24, Mark Rutland  wrote:
> > On Fri, Apr 07, 2017 at 12:47:47AM +0800, Fu Wei wrote:
> >> On 6 April 2017 at 02:38, Mark Rutland  wrote:
> >> > On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.org wrote:

> > 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;

What I meant was that we won't look at the frame->valid flag at all;
only the interrupts. e.g.

for (int i = 0; i < ARCH_TIMER_MEM_MAX_FRAMES; i++) {
if (frame->phys_irq > 0)
free_the_phys_irq_somehow();
if (frame->virt_irq > 0)
free_the_virt_irq_somehow();
}

... where we somehow figure out the GSI, or we introduce an api like
unregister_gsi_for_irq(irq).

Since the !valid frames should all have zero for their interrupt fields,
no special handling is necessary.

That way, we only free the IRQs in one place, it's obvious that we
consistently free all of them, etc.

> 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'd prefer to keep this constrained to the ACPI code. ;)

Thanks,
Mark.

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Mark Rutland]

On Fri, Apr 07, 2017 at 01:39:09AM +0800, Fu Wei wrote:
> On 7 April 2017 at 01:24, Mark Rutland  wrote:
> > On Fri, Apr 07, 2017 at 12:47:47AM +0800, Fu Wei wrote:
> >> On 6 April 2017 at 02:38, Mark Rutland  wrote:
> >> > On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.org wrote:

> > 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;

What I meant was that we won't look at the frame->valid flag at all;
only the interrupts. e.g.

for (int i = 0; i < ARCH_TIMER_MEM_MAX_FRAMES; i++) {
if (frame->phys_irq > 0)
free_the_phys_irq_somehow();
if (frame->virt_irq > 0)
free_the_virt_irq_somehow();
}

... where we somehow figure out the GSI, or we introduce an api like
unregister_gsi_for_irq(irq).

Since the !valid frames should all have zero for their interrupt fields,
no special handling is necessary.

That way, we only free the IRQs in one place, it's obvious that we
consistently free all of them, etc.

> 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'd prefer to keep this constrained to the ACPI code. ;)

Thanks,
Mark.

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Fu Wei]

Hi Mark

On 7 April 2017 at 01:24, Mark Rutland  wrote:
> On Fri, Apr 07, 2017 at 12:47:47AM +0800, Fu Wei wrote:
>> On 6 April 2017 at 02:38, Mark Rutland  wrote:
>> > On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.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 = _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 = _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

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Fu Wei]

Hi Mark

On 7 April 2017 at 01:24, Mark Rutland  wrote:
> On Fri, Apr 07, 2017 at 12:47:47AM +0800, Fu Wei wrote:
>> On 6 April 2017 at 02:38, Mark Rutland  wrote:
>> > On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.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 = _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 = _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

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Mark Rutland]

On Fri, Apr 07, 2017 at 12:47:47AM +0800, Fu Wei wrote:
> On 6 April 2017 at 02:38, Mark Rutland  wrote:
> > On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.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 = _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 = _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.

I can make that update locally; no need to respin.

Thanks,
Mark.

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Mark Rutland]

On Fri, Apr 07, 2017 at 12:47:47AM +0800, Fu Wei wrote:
> On 6 April 2017 at 02:38, Mark Rutland  wrote:
> > On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.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 = _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 = _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.

I can make that update locally; no need to respin.

Thanks,
Mark.

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Fu Wei]

Hi Lorenzo,

On 3 April 2017 at 18:45, Lorenzo Pieralisi  wrote:
> On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.org wrote:
>> From: Fu Wei 
>>
>> On platforms booting with ACPI, architected memory-mapped 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.
>>
>> Signed-off-by: Fu Wei 
>> Signed-off-by: Hanjun Guo 
>> ---
>>  drivers/acpi/arm64/gtdt.c | 146 
>> ++
>>  include/linux/acpi.h  |   1 +
>>  2 files changed, 147 insertions(+)
>>
>> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c
>> index 3d95af8..3dd33f3 100644
>> --- a/drivers/acpi/arm64/gtdt.c
>> +++ b/drivers/acpi/arm64/gtdt.c
>> @@ -13,6 +13,7 @@
>>
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>
>>  #include 
>> @@ -37,6 +38,28 @@ struct acpi_gtdt_descriptor {
>>
>>  static struct acpi_gtdt_descriptor acpi_gtdt_desc __initdata;
>>
>> +static inline void *next_platform_timer(void *platform_timer)
>> +{
>> + struct acpi_gtdt_header *gh = platform_timer;
>> +
>> + platform_timer += gh->length;
>> + if (platform_timer < acpi_gtdt_desc.gtdt_end)
>> + return platform_timer;
>> +
>> + return NULL;
>> +}
>> +
>> +#define for_each_platform_timer(_g)  \
>> + for (_g = acpi_gtdt_desc.platform_timer; _g;\
>> +  _g = next_platform_timer(_g))
>> +
>> +static inline bool is_timer_block(void *platform_timer)
>> +{
>> + struct acpi_gtdt_header *gh = platform_timer;
>> +
>> + return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK;
>> +}
>> +
>>  static int __init map_gt_gsi(u32 interrupt, u32 flags)
>>  {
>>   int trigger, polarity;
>> @@ -155,3 +178,126 @@ int __init acpi_gtdt_init(struct acpi_table_header 
>> *table,
>>
>>   return 0;
>>  }
>> +
>> +static int __init gtdt_parse_timer_block(struct acpi_gtdt_timer_block 
>> *block,
>> +  struct arch_timer_mem *timer_mem)
>> +{
>> + int i;
>> + struct arch_timer_mem_frame *frame;
>> + struct acpi_gtdt_timer_entry *gtdt_frame;
>> +
>> + if (!block->timer_count) {
>> + pr_err(FW_BUG "GT block present, but frame count is zero.");
>> + return -ENODEV;
>> + }
>> +
>> + if (block->timer_count > ARCH_TIMER_MEM_MAX_FRAMES) {
>> + pr_err(FW_BUG "GT block lists %d frames, ACPI spec only allows 
>> 8\n",
>> +block->timer_count);
>> + return -EINVAL;
>> + }
>> +
>> + timer_mem->cntctlbase = (phys_addr_t)block->block_address;
>> + /*
>> +  * The CNTCTLBase frame is 4KB (register offsets 0x000 - 0xFFC).
>> +  * See ARM DDI 0487A.k_iss10775, page I1-5129, Table I1-3
>> +  * "CNTCTLBase memory map".
>> +  */
>> + timer_mem->size = SZ_4K;
>> +
>> + gtdt_frame = (void *)block + block->timer_offset;
>> + if (gtdt_frame + block->timer_count != (void *)block + 
>> block->header.length)
>> + return -EINVAL;
>> +
>> + /*
>> +  * 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 = _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);
>> + 
>> 

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Fu Wei]

Hi Lorenzo,

On 3 April 2017 at 18:45, Lorenzo Pieralisi  wrote:
> On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.org wrote:
>> From: Fu Wei 
>>
>> On platforms booting with ACPI, architected memory-mapped 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.
>>
>> Signed-off-by: Fu Wei 
>> Signed-off-by: Hanjun Guo 
>> ---
>>  drivers/acpi/arm64/gtdt.c | 146 
>> ++
>>  include/linux/acpi.h  |   1 +
>>  2 files changed, 147 insertions(+)
>>
>> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c
>> index 3d95af8..3dd33f3 100644
>> --- a/drivers/acpi/arm64/gtdt.c
>> +++ b/drivers/acpi/arm64/gtdt.c
>> @@ -13,6 +13,7 @@
>>
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>
>>  #include 
>> @@ -37,6 +38,28 @@ struct acpi_gtdt_descriptor {
>>
>>  static struct acpi_gtdt_descriptor acpi_gtdt_desc __initdata;
>>
>> +static inline void *next_platform_timer(void *platform_timer)
>> +{
>> + struct acpi_gtdt_header *gh = platform_timer;
>> +
>> + platform_timer += gh->length;
>> + if (platform_timer < acpi_gtdt_desc.gtdt_end)
>> + return platform_timer;
>> +
>> + return NULL;
>> +}
>> +
>> +#define for_each_platform_timer(_g)  \
>> + for (_g = acpi_gtdt_desc.platform_timer; _g;\
>> +  _g = next_platform_timer(_g))
>> +
>> +static inline bool is_timer_block(void *platform_timer)
>> +{
>> + struct acpi_gtdt_header *gh = platform_timer;
>> +
>> + return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK;
>> +}
>> +
>>  static int __init map_gt_gsi(u32 interrupt, u32 flags)
>>  {
>>   int trigger, polarity;
>> @@ -155,3 +178,126 @@ int __init acpi_gtdt_init(struct acpi_table_header 
>> *table,
>>
>>   return 0;
>>  }
>> +
>> +static int __init gtdt_parse_timer_block(struct acpi_gtdt_timer_block 
>> *block,
>> +  struct arch_timer_mem *timer_mem)
>> +{
>> + int i;
>> + struct arch_timer_mem_frame *frame;
>> + struct acpi_gtdt_timer_entry *gtdt_frame;
>> +
>> + if (!block->timer_count) {
>> + pr_err(FW_BUG "GT block present, but frame count is zero.");
>> + return -ENODEV;
>> + }
>> +
>> + if (block->timer_count > ARCH_TIMER_MEM_MAX_FRAMES) {
>> + pr_err(FW_BUG "GT block lists %d frames, ACPI spec only allows 
>> 8\n",
>> +block->timer_count);
>> + return -EINVAL;
>> + }
>> +
>> + timer_mem->cntctlbase = (phys_addr_t)block->block_address;
>> + /*
>> +  * The CNTCTLBase frame is 4KB (register offsets 0x000 - 0xFFC).
>> +  * See ARM DDI 0487A.k_iss10775, page I1-5129, Table I1-3
>> +  * "CNTCTLBase memory map".
>> +  */
>> + timer_mem->size = SZ_4K;
>> +
>> + gtdt_frame = (void *)block + block->timer_offset;
>> + if (gtdt_frame + block->timer_count != (void *)block + 
>> block->header.length)
>> + return -EINVAL;
>> +
>> + /*
>> +  * 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 = _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;
>> 

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Fu Wei]

Hi Mark,

On 6 April 2017 at 02:38, Mark Rutland  wrote:
> Hi,
>
> I tried to fix the issue that Lornzo raised, such that I could queue
> these patches. From looking at this patch in more detail however, I
> think there are further issues that need to be addressed.
>
> On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.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 = _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 = _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?

Thanks!

>
>> + return -EINVAL;
>> +}
>> +
>> +/**
>> + * acpi_arch_timer_mem_init() - Get the info of all GT blocks in GTDT table.
>> + * @timer_mem:   The pointer to the array of struct arch_timer_mem for 
>> returning
>> + *   the result of parsing. The element number of this array should
>> + *   be platform_timer_count(the total number of platform timers).
>> + * @timer_count: It points to a integer variable which is used for storing 
>> the
>> + *   number of GT blocks we have parsed.
>> + *
>> + * Return: 0 if success, -EINVAL/-ENODEV if error.
>> + */
>> +int __init acpi_arch_timer_mem_init(struct arch_timer_mem *timer_mem,
>> + int *timer_count)
>> +{
>> + int ret;
>> + void *platform_timer;
>> +
>> + *timer_count = 0;
>> + for_each_platform_timer(platform_timer) {
>> + if (is_timer_block(platform_timer)) {
>> + ret = gtdt_parse_timer_block(platform_timer, 
>> timer_mem);
>> + if (ret)
>> + return ret;
>> + timer_mem++;
>> + (*timer_count)++;
>> + }
>> + }
>
> If we were to have multiple GT blocks, this would leave timer_mem in an
> inconsistent state. In gtdt_parse_timer_block we'll blat any existing
> timer_mem->cntctlbase, 

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Fu Wei]

Hi Mark,

On 6 April 2017 at 02:38, Mark Rutland  wrote:
> Hi,
>
> I tried to fix the issue that Lornzo raised, such that I could queue
> these patches. From looking at this patch in more detail however, I
> think there are further issues that need to be addressed.
>
> On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.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 = _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 = _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?

Thanks!

>
>> + return -EINVAL;
>> +}
>> +
>> +/**
>> + * acpi_arch_timer_mem_init() - Get the info of all GT blocks in GTDT table.
>> + * @timer_mem:   The pointer to the array of struct arch_timer_mem for 
>> returning
>> + *   the result of parsing. The element number of this array should
>> + *   be platform_timer_count(the total number of platform timers).
>> + * @timer_count: It points to a integer variable which is used for storing 
>> the
>> + *   number of GT blocks we have parsed.
>> + *
>> + * Return: 0 if success, -EINVAL/-ENODEV if error.
>> + */
>> +int __init acpi_arch_timer_mem_init(struct arch_timer_mem *timer_mem,
>> + int *timer_count)
>> +{
>> + int ret;
>> + void *platform_timer;
>> +
>> + *timer_count = 0;
>> + for_each_platform_timer(platform_timer) {
>> + if (is_timer_block(platform_timer)) {
>> + ret = gtdt_parse_timer_block(platform_timer, 
>> timer_mem);
>> + if (ret)
>> + return ret;
>> + timer_mem++;
>> + (*timer_count)++;
>> + }
>> + }
>
> If we were to have multiple GT blocks, this would leave timer_mem in an
> inconsistent state. In gtdt_parse_timer_block we'll blat any existing
> timer_mem->cntctlbase, and blat some 

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Mark Rutland]

On Wed, Apr 05, 2017 at 07:38:09PM +0100, Mark Rutland wrote:
> I tried to fix the issue that Lornzo raised, such that I could queue
> these patches. From looking at this patch in more detail however, I
> think there are further issues that need to be addressed.

Looking again, I see that I was mistaken w.r.t. the multi GT block case.

> > +int __init acpi_arch_timer_mem_init(struct arch_timer_mem *timer_mem,
> > +   int *timer_count)
> > +{
> > +   int ret;
> > +   void *platform_timer;
> > +
> > +   *timer_count = 0;
> > +   for_each_platform_timer(platform_timer) {
> > +   if (is_timer_block(platform_timer)) {
> > +   ret = gtdt_parse_timer_block(platform_timer, timer_mem);
> > +   if (ret)
> > +   return ret;
> > +   timer_mem++;
> > +   (*timer_count)++;
> > +   }
> > +   }
> 
> If we were to have multiple GT blocks, this would leave timer_mem in an
> inconsistent state. In gtdt_parse_timer_block we'll blat any existing
> timer_mem->cntctlbase, and blat some arbitrary set of frames. however,
> *some* frames may have been held over from a previous iteration.

This is not the case; I had somehow missed the timer_mem++.

Sorry for the noise.

Thanks,
Mark.

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Mark Rutland]

On Wed, Apr 05, 2017 at 07:38:09PM +0100, Mark Rutland wrote:
> I tried to fix the issue that Lornzo raised, such that I could queue
> these patches. From looking at this patch in more detail however, I
> think there are further issues that need to be addressed.

Looking again, I see that I was mistaken w.r.t. the multi GT block case.

> > +int __init acpi_arch_timer_mem_init(struct arch_timer_mem *timer_mem,
> > +   int *timer_count)
> > +{
> > +   int ret;
> > +   void *platform_timer;
> > +
> > +   *timer_count = 0;
> > +   for_each_platform_timer(platform_timer) {
> > +   if (is_timer_block(platform_timer)) {
> > +   ret = gtdt_parse_timer_block(platform_timer, timer_mem);
> > +   if (ret)
> > +   return ret;
> > +   timer_mem++;
> > +   (*timer_count)++;
> > +   }
> > +   }
> 
> If we were to have multiple GT blocks, this would leave timer_mem in an
> inconsistent state. In gtdt_parse_timer_block we'll blat any existing
> timer_mem->cntctlbase, and blat some arbitrary set of frames. however,
> *some* frames may have been held over from a previous iteration.

This is not the case; I had somehow missed the timer_mem++.

Sorry for the noise.

Thanks,
Mark.

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Mark Rutland]

Hi,

I tried to fix the issue that Lornzo raised, such that I could queue
these patches. From looking at this patch in more detail however, I
think there are further issues that need to be addressed.

On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.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 = _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 = _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
won't free the interrupts of the last frame we parsed.

> + return -EINVAL;
> +}
> +
> +/**
> + * acpi_arch_timer_mem_init() - Get the info of all GT blocks in GTDT table.
> + * @timer_mem:   The pointer to the array of struct arch_timer_mem for 
> returning
> + *   the result of parsing. The element number of this array should
> + *   be platform_timer_count(the total number of platform timers).
> + * @timer_count: It points to a integer variable which is used for storing 
> the
> + *   number of GT blocks we have parsed.
> + *
> + * Return: 0 if success, -EINVAL/-ENODEV if error.
> + */
> +int __init acpi_arch_timer_mem_init(struct arch_timer_mem *timer_mem,
> + int *timer_count)
> +{
> + int ret;
> + void *platform_timer;
> +
> + *timer_count = 0;
> + for_each_platform_timer(platform_timer) {
> + if (is_timer_block(platform_timer)) {
> + ret = gtdt_parse_timer_block(platform_timer, timer_mem);
> + if (ret)
> + return ret;
> + timer_mem++;
> + (*timer_count)++;
> + }
> + }

If we were to have multiple GT blocks, this would leave timer_mem in an
inconsistent state. In gtdt_parse_timer_block we'll blat any existing
timer_mem->cntctlbase, and blat some arbitrary set of frames. however,
*some* frames may have been held over from a previous iteration.

My understanding was that the system level timer had a single CNTCTLBase
frame, and hence we should only have a single GT block.

Judging by ARM DDI 0487A.k_iss10775, I1.3 "Memory-mapped timer
components" and I3.4 "Generic Timer memory-mapped registers overview",
it does appear that the system should only have one CNTCTLBase frame.

What's going on here?

Thanks,
Mark.

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Mark Rutland]

Hi,

I tried to fix the issue that Lornzo raised, such that I could queue
these patches. From looking at this patch in more detail however, I
think there are further issues that need to be addressed.

On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.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 = _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 = _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
won't free the interrupts of the last frame we parsed.

> + return -EINVAL;
> +}
> +
> +/**
> + * acpi_arch_timer_mem_init() - Get the info of all GT blocks in GTDT table.
> + * @timer_mem:   The pointer to the array of struct arch_timer_mem for 
> returning
> + *   the result of parsing. The element number of this array should
> + *   be platform_timer_count(the total number of platform timers).
> + * @timer_count: It points to a integer variable which is used for storing 
> the
> + *   number of GT blocks we have parsed.
> + *
> + * Return: 0 if success, -EINVAL/-ENODEV if error.
> + */
> +int __init acpi_arch_timer_mem_init(struct arch_timer_mem *timer_mem,
> + int *timer_count)
> +{
> + int ret;
> + void *platform_timer;
> +
> + *timer_count = 0;
> + for_each_platform_timer(platform_timer) {
> + if (is_timer_block(platform_timer)) {
> + ret = gtdt_parse_timer_block(platform_timer, timer_mem);
> + if (ret)
> + return ret;
> + timer_mem++;
> + (*timer_count)++;
> + }
> + }

If we were to have multiple GT blocks, this would leave timer_mem in an
inconsistent state. In gtdt_parse_timer_block we'll blat any existing
timer_mem->cntctlbase, and blat some arbitrary set of frames. however,
*some* frames may have been held over from a previous iteration.

My understanding was that the system level timer had a single CNTCTLBase
frame, and hence we should only have a single GT block.

Judging by ARM DDI 0487A.k_iss10775, I1.3 "Memory-mapped timer
components" and I3.4 "Generic Timer memory-mapped registers overview",
it does appear that the system should only have one CNTCTLBase frame.

What's going on here?

Thanks,
Mark.

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Lorenzo Pieralisi]

On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.org wrote:
> From: Fu Wei 
> 
> On platforms booting with ACPI, architected memory-mapped 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.
> 
> Signed-off-by: Fu Wei 
> Signed-off-by: Hanjun Guo 
> ---
>  drivers/acpi/arm64/gtdt.c | 146 
> ++
>  include/linux/acpi.h  |   1 +
>  2 files changed, 147 insertions(+)
> 
> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c
> index 3d95af8..3dd33f3 100644
> --- a/drivers/acpi/arm64/gtdt.c
> +++ b/drivers/acpi/arm64/gtdt.c
> @@ -13,6 +13,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include 
> @@ -37,6 +38,28 @@ struct acpi_gtdt_descriptor {
>  
>  static struct acpi_gtdt_descriptor acpi_gtdt_desc __initdata;
>  
> +static inline void *next_platform_timer(void *platform_timer)
> +{
> + struct acpi_gtdt_header *gh = platform_timer;
> +
> + platform_timer += gh->length;
> + if (platform_timer < acpi_gtdt_desc.gtdt_end)
> + return platform_timer;
> +
> + return NULL;
> +}
> +
> +#define for_each_platform_timer(_g)  \
> + for (_g = acpi_gtdt_desc.platform_timer; _g;\
> +  _g = next_platform_timer(_g))
> +
> +static inline bool is_timer_block(void *platform_timer)
> +{
> + struct acpi_gtdt_header *gh = platform_timer;
> +
> + return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK;
> +}
> +
>  static int __init map_gt_gsi(u32 interrupt, u32 flags)
>  {
>   int trigger, polarity;
> @@ -155,3 +178,126 @@ int __init acpi_gtdt_init(struct acpi_table_header 
> *table,
>  
>   return 0;
>  }
> +
> +static int __init gtdt_parse_timer_block(struct acpi_gtdt_timer_block *block,
> +  struct arch_timer_mem *timer_mem)
> +{
> + int i;
> + struct arch_timer_mem_frame *frame;
> + struct acpi_gtdt_timer_entry *gtdt_frame;
> +
> + if (!block->timer_count) {
> + pr_err(FW_BUG "GT block present, but frame count is zero.");
> + return -ENODEV;
> + }
> +
> + if (block->timer_count > ARCH_TIMER_MEM_MAX_FRAMES) {
> + pr_err(FW_BUG "GT block lists %d frames, ACPI spec only allows 
> 8\n",
> +block->timer_count);
> + return -EINVAL;
> + }
> +
> + timer_mem->cntctlbase = (phys_addr_t)block->block_address;
> + /*
> +  * The CNTCTLBase frame is 4KB (register offsets 0x000 - 0xFFC).
> +  * See ARM DDI 0487A.k_iss10775, page I1-5129, Table I1-3
> +  * "CNTCTLBase memory map".
> +  */
> + timer_mem->size = SZ_4K;
> +
> + gtdt_frame = (void *)block + block->timer_offset;
> + if (gtdt_frame + block->timer_count != (void *)block + 
> block->header.length)
> + return -EINVAL;
> +
> + /*
> +  * 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 = _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;
> + 

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Lorenzo Pieralisi]

On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.org wrote:
> From: Fu Wei 
> 
> On platforms booting with ACPI, architected memory-mapped 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.
> 
> Signed-off-by: Fu Wei 
> Signed-off-by: Hanjun Guo 
> ---
>  drivers/acpi/arm64/gtdt.c | 146 
> ++
>  include/linux/acpi.h  |   1 +
>  2 files changed, 147 insertions(+)
> 
> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c
> index 3d95af8..3dd33f3 100644
> --- a/drivers/acpi/arm64/gtdt.c
> +++ b/drivers/acpi/arm64/gtdt.c
> @@ -13,6 +13,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include 
> @@ -37,6 +38,28 @@ struct acpi_gtdt_descriptor {
>  
>  static struct acpi_gtdt_descriptor acpi_gtdt_desc __initdata;
>  
> +static inline void *next_platform_timer(void *platform_timer)
> +{
> + struct acpi_gtdt_header *gh = platform_timer;
> +
> + platform_timer += gh->length;
> + if (platform_timer < acpi_gtdt_desc.gtdt_end)
> + return platform_timer;
> +
> + return NULL;
> +}
> +
> +#define for_each_platform_timer(_g)  \
> + for (_g = acpi_gtdt_desc.platform_timer; _g;\
> +  _g = next_platform_timer(_g))
> +
> +static inline bool is_timer_block(void *platform_timer)
> +{
> + struct acpi_gtdt_header *gh = platform_timer;
> +
> + return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK;
> +}
> +
>  static int __init map_gt_gsi(u32 interrupt, u32 flags)
>  {
>   int trigger, polarity;
> @@ -155,3 +178,126 @@ int __init acpi_gtdt_init(struct acpi_table_header 
> *table,
>  
>   return 0;
>  }
> +
> +static int __init gtdt_parse_timer_block(struct acpi_gtdt_timer_block *block,
> +  struct arch_timer_mem *timer_mem)
> +{
> + int i;
> + struct arch_timer_mem_frame *frame;
> + struct acpi_gtdt_timer_entry *gtdt_frame;
> +
> + if (!block->timer_count) {
> + pr_err(FW_BUG "GT block present, but frame count is zero.");
> + return -ENODEV;
> + }
> +
> + if (block->timer_count > ARCH_TIMER_MEM_MAX_FRAMES) {
> + pr_err(FW_BUG "GT block lists %d frames, ACPI spec only allows 
> 8\n",
> +block->timer_count);
> + return -EINVAL;
> + }
> +
> + timer_mem->cntctlbase = (phys_addr_t)block->block_address;
> + /*
> +  * The CNTCTLBase frame is 4KB (register offsets 0x000 - 0xFFC).
> +  * See ARM DDI 0487A.k_iss10775, page I1-5129, Table I1-3
> +  * "CNTCTLBase memory map".
> +  */
> + timer_mem->size = SZ_4K;
> +
> + gtdt_frame = (void *)block + block->timer_offset;
> + if (gtdt_frame + block->timer_count != (void *)block + 
> block->header.length)
> + return -EINVAL;
> +
> + /*
> +  * 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 = _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",
> +  

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Will Deacon]

On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.org wrote:
> From: Fu Wei 
> 
> On platforms booting with ACPI, architected memory-mapped 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.
> 
> Signed-off-by: Fu Wei 
> Signed-off-by: Hanjun Guo 
> ---
>  drivers/acpi/arm64/gtdt.c | 146 
> ++
>  include/linux/acpi.h  |   1 +
>  2 files changed, 147 insertions(+)

We need Lorenzo's ack on this.

Will

Re: [PATCH v23 09/11] acpi/arm64: Add memory-mapped timer support in GTDT driver

[Will Deacon]

On Sat, Apr 01, 2017 at 01:51:03AM +0800, fu@linaro.org wrote:
> From: Fu Wei 
> 
> On platforms booting with ACPI, architected memory-mapped 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.
> 
> Signed-off-by: Fu Wei 
> Signed-off-by: Hanjun Guo 
> ---
>  drivers/acpi/arm64/gtdt.c | 146 
> ++
>  include/linux/acpi.h  |   1 +
>  2 files changed, 147 insertions(+)

We need Lorenzo's ack on this.

Will