Re: [PATCH V2 1/1] GIC: introduce method to deactive interupts

From: Liu hua
Date: Wed Aug 06 2014 - 04:44:24 EST


ä 2014/8/4 17:43, Marc Zyngier åé:
> Hi Liu,
>
> On 04/08/14 05:17, Liu Hua wrote:
>> When using kdump on ARM platform, if kernel panics in interrupt handler
>> (maybe PPI), the capture kernel can not recive certain interrupt, and
>> fails to boot.
>>
>> On this situation, We have read register GICC_IAR. But we have no chance
>> to write relative bit to register GICC_EOIR (kernel paniced before). So
>> the state of this type interrupt remains active. And that makes gic not
>> deliver this type interrupt to cpu interface.
>>
>> So we should not assume that all interrut states of GIC are inactive when
>> kernel inittailize the GIC. This patch will identify these type interrupts
>> and deactive them
>>
>> Signed-off-by: Liu Hua <sdu.liu@xxxxxxxxxx>
>> ---
>> drivers/irqchip/irq-gic.c | 26 ++++++++++++++++++++++++++
>> 1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index b2648fc..7708df1 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -351,12 +351,37 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
>> return mask;
>> }
>>
>> +void gic_eois(u32 active, int irq_off, void __iomem *cpu_base)
>> +{
>> + int bit = -1;
>> +
>> + for_each_set_bit(bit, (unsigned long *)&active, 32)
>> + writel_relaxed(bit + irq_off, cpu_base + GIC_CPU_EOI);
>> +}
>> +
>> +void gic_dist_clear_active(void __iomem *dist_base,
>> + void __iomem *cpu_base, int gic_irqs)
>> +{
>> + int irq, offset;
>> + u32 active;
>> +
>> + for (irq = 0; irq < gic_irqs; irq += 32) {
>> + offset = GIC_DIST_ACTIVE_SET + irq * 4 / 32;
>> + active = readl_relaxed(dist_base + offset);
>> + if (!active)
>> + continue;
>> + gic_eois(active, irq, cpu_base);
>> + }
>> +}
>> +
>> +
>> static void __init gic_dist_init(struct gic_chip_data *gic)
>> {
>> unsigned int i;
>> u32 cpumask;
>> unsigned int gic_irqs = gic->gic_irqs;
>> void __iomem *base = gic_data_dist_base(gic);
>> + void __iomem *cpu_base = gic_data_cpu_base(gic);
>>
>> writel_relaxed(0, base + GIC_DIST_CTRL);
>>
>> @@ -371,6 +396,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
>>
>> gic_dist_config(base, gic_irqs, NULL);
>>
>> + gic_dist_clear_active(base, cpu_base, gic_irqs);
>> writel_relaxed(1, base + GIC_DIST_CTRL);
>> }
>
> So while this is solving a real issue, I don't think you can just fix it
> for the UP case. You'll have to fix the same thing for secondary CPUs
> (shouldn't be too hard to split things between local and global interrupts).
Hi Marc,

Thanks very much for you reply!

when I tried to implement your ideas. I found that: when kdump is deployed
and without my patch,

(1) panic in PPI, the capture kernel can not boot up.
(2) panic in SPI, the capture kernel boot up regularly.

I was confused and there may be something I did not catch. I glanced the kdump
code and found that function machine_kexec_mask_interrupts. It will clear the
GIC active state only if the IRQD_IRQ_INPROGRESS bit in d->state_use_accessors
is set.

And the PPI handler does not set this flag. So there are two ways to solve this
problem.

(1) consider this problem common, as you and I thought before. we should fix secondary CPUs issues;


(2)just set flag IRQD_IRQ_INPROGRESS in PPI. we need patch like this:

-------------(2) patch start-----------
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index a2b28a2..0a5dfe0 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -677,10 +677,18 @@ void handle_percpu_devid_irq(unsigned int irq, struct irq_desc *desc)
if (chip->irq_ack)
chip->irq_ack(&desc->irq_data);

+ raw_spin_lock(&desc->lock);
+ irqd_set(&desc->irq_data, IRQD_IRQ_INPROGRESS);
+ raw_spin_unlock(&desc->lock);
+
trace_irq_handler_entry(irq, action);
res = action->handler(irq, dev_id);
trace_irq_handler_exit(irq, action, res);

+ raw_spin_lock(&desc->lock);
+ irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);
+ raw_spin_unlock(&desc->lock);
+
if (chip->irq_eoi)
chip->irq_eoi(&desc->irq_data);
}
-------------(2) patch end-----------

Way 2 seems to be needed anyway.
For way 1, I do not find another situation that the gic interrupt states remains active when kernel booting.
And for kdump process, Way 2 is enough.

What do you think about them?

Thanks,
Liu Hua

> Thanks,
>
> M.
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/