Hi, On Tue, Nov 24, 2020 at 1:00 AM Marc Zyngier <maz@xxxxxxxxxx> wrote: > > > @@ -187,9 +189,24 @@ static int qcom_pdc_gic_set_type(struct irq_data > > *d, unsigned int type) > > return -EINVAL; > > } > > > > + old_pdc_type = pdc_reg_read(IRQ_i_CFG, pin_out); > > pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type); > > > > - return irq_chip_set_type_parent(d, type); > > + ret = irq_chip_set_type_parent(d, type); > > + > > + /* > > + * When we change types the PDC can give a phantom interrupt. > > + * Clear it. Specifically the phantom shows up if a line is already > > + * high and we change to rising or if a line is already low and we > > + * change to falling but let's be consistent and clear it always. > > + * > > + * Doing this works because we have IRQCHIP_SET_TYPE_MASKED so the > > + * interrupt will be cleared before the rest of the system sees it. > > + */ > > + if (old_pdc_type != pdc_type) > > + irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0); > > nit: s/0/false/. I'll fix this. > You could also make it conditional on the parent side having been > successful. Good idea. > And while we're looking at this: do you need to rollback the PDC state > if the GIC side has failed? It's all very hypothetical, but just in > case... I'm going to go ahead and say "no", but I'll make this change if you insist. Specifically: * We're still leaving things in a self-consistent state if we fail to clear the parent, we'll just get a spurious interrupt. It won't cause any crashes / hangs / whatever. * Since it seems very unlikely we'd ever trip this and if we ever do it's not the end of the world, I'd rather not add extra code. Hopefully that's OK. -Doug
