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@xxxxxxxxxxxxxx>
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.
