lkml.org 
[lkml]   [2019]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v0] irqchip/gic-v3: Avoid check of lpi configuration for non existent cpu
From
Date


On 12/5/2019 6:17 PM, Marc Zyngier wrote:
> Hi Gaurav,
>
> On 2019-12-05 10:55, Gaurav Kohli wrote:
>> As per GIC specification, we can configure gic for more no of cpus
>> then the available cpus in the soc, But this can cause mem abort
>> while iterating lpi region for non existent cpu as we don't map
>
> Which LPI region? We're talking about RDs, right... Or does LPI mean
> something other than GIC LPIs for you?
>

Yes RDs only.
>> redistrubutor region for non-existent cpu.
>>
>> To avoid this issue, put one more check of valid mpidr.
>
> Sorry, but I'm not sure I grasp your problem. Let me try and rephrase it:
>
> - Your GIC is configured for (let's say) 8 CPUs, and your SoC has only 4.
Yes, suppose gic is configured for 8 cpus but soc has only 4 cpus. Then
in this case gic_iterate will iterate till it get TYPER_LAST.

But as gic is configured for 8, So last bit sets in eight redistributor
regions only.
>
> - As part of the probing, the driver iterates on the RD regions and
> explodes
>   because something isn't mapped?
>
> That'd be a grave bug, but I believe the issue is somewhere else.

There are 4 cpus present, that's why we have mapped 4 redistributor
only, but during probe below function keeps iterating and give mem abort
for 5th cpu.

static void gic_update_vlpi_properties(void)
{
gic_iterate_rdists(__gic_update_vlpi_properties);

}

We can solve this problem by mapping all eight redistributor in dt, but
ideally code should also able to handle this and we can avoid mappin?
>
>>
>> Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org>
>>
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index 1edc993..adc9186 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -766,6 +766,7 @@ static int gic_iterate_rdists(int (*fn)(struct
>> redist_region *, void __iomem *))
>>  {
>>      int ret = -ENODEV;
>>      int i;
>> +    int cpu = 0;
>>
>>      for (i = 0; i < gic_data.nr_redist_regions; i++) {
>>          void __iomem *ptr = gic_data.redist_regions[i].redist_base;
>> @@ -780,6 +781,7 @@ static int gic_iterate_rdists(int (*fn)(struct
>> redist_region *, void __iomem *))
>>          }
>>
>>          do {
>> +            cpu++;
>>              typer = gic_read_typer(ptr + GICR_TYPER);
>>              ret = fn(gic_data.redist_regions + i, ptr);
>>              if (!ret)
>> @@ -795,7 +797,8 @@ static int gic_iterate_rdists(int (*fn)(struct
>> redist_region *, void __iomem *))
>>                  if (typer & GICR_TYPER_VLPIS)
>>                      ptr += SZ_64K * 2; /* Skip VLPI_base + reserved
>> page */
>>              }
>> -        } while (!(typer & GICR_TYPER_LAST));
>> +        } while (!(typer & GICR_TYPER_LAST) &&
>> +                    cpu_logical_map(cpu) != INVALID_HWID);
>>      }
>>
>>      return ret ? -ENODEV : 0;
>
> This makes little sense. A redistributor region contains a bunch of RDs,
> each of which maps onto a given CPU. We iterate on the RDs, and not on the
> CPUs, as it is the RD that tells us which CPU it is affine with, not the
> other way around.
>
> If a RD is for some reason unavailable, then it shouldn't be described in
> the firmware the first place. If you end-up exposing RD regions that do
> not have the last RD having GICR_TYPER.Last set, then your SoC is broken,
> and this needs yet another quirk.
>
>         M.

--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

\
 
 \ /
  Last update: 2019-12-05 14:02    [W:0.060 / U:2.708 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site