lkml.org 
[lkml]   [2020]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 1/3] irqchip: qcom-pdc: Fix phantom irq when changing between rising/falling
Hi,

On Tue, Nov 24, 2020 at 1:00 AM Marc Zyngier <maz@kernel.org> 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

\
 
 \ /
  Last update: 2020-11-24 18:03    [W:0.110 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site