| Message ID | 1505668548-16616-9-git-send-email-mark.cave-ayland@ilande.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Sep 17, 2017 at 06:15:48PM +0100, Mark Cave-Ayland wrote: > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > Some interrupts get triggered before the OS has setup the > right interrupt type. If an edge interrupt is latched that > way, not delivered (still masked), then the interrupt is > changed to level and isn't asserted anymore, it will be > stuck "pending", causing an interrupt flood. This can happen > with the PMU GPIO interrupt for example. > > There are a few other corner cases like this, so let's keep > track of the input "level" so we can properly re-evaluate > when the trigger type changes. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > --- > hw/intc/openpic.c | 32 +++++++++++++++++++++++++++----- > 1 file changed, 27 insertions(+), 5 deletions(-) > > diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c > index debfcbf..34749f8 100644 > --- a/hw/intc/openpic.c > +++ b/hw/intc/openpic.c > @@ -236,6 +236,7 @@ typedef struct IRQSource { > int last_cpu; > int output; /* IRQ level, e.g. OPENPIC_OUTPUT_INT */ > int pending; /* TRUE if IRQ is pending */ > + int cur_level; /* Cache current level */ > IRQType type; > bool level:1; /* level-triggered */ > bool nomask:1; /* critical interrupts ignore mask on some FSL MPICs */ > @@ -552,14 +553,26 @@ static void openpic_set_irq(void *opaque, int n_IRQ, int level) > } > > src = &opp->src[n_IRQ]; > - DPRINTF("openpic: set irq %d = %d ivpr=0x%08x\n", > - n_IRQ, level, src->ivpr); > + DPRINTF("openpic: set irq %d = %d ivpr=0x%08x (l=%d,cl=%d)\n", > + n_IRQ, level, src->ivpr, src->level, src->cur_level); > + > + /* Keep track of the current input level in order to properly deal > + * with the configuration of the source changing from edge to level > + * after it's been latched. Otherwise the interrupt can get stuck. > + */ > + src->cur_level = level; > + > if (src->level) { > - /* level-sensitive irq */ > src->pending = level; > openpic_update_irq(opp, n_IRQ); > } else { > - /* edge-sensitive irq */ > + /* edge-sensitive irq > + * > + * In an ideal world we would only set pending on an "edge", ie > + * if level is set and src->cur_level as clear. However that would > + * require all the devices to properly send "pulses" rather than > + * just "raise" which isn't the case today. > + */ > if (level) { > src->pending = 1; > openpic_update_irq(opp, n_IRQ); > @@ -676,6 +689,13 @@ static inline void write_IRQreg_ivpr(OpenPICState *opp, int n_IRQ, uint32_t val) > switch (opp->src[n_IRQ].type) { > case IRQ_TYPE_NORMAL: > opp->src[n_IRQ].level = !!(opp->src[n_IRQ].ivpr & IVPR_SENSE_MASK); > + > + /* If we switched to level we need to re-evaluate "pending" based > + * on the actual input state. > + */ > + if (opp->src[n_IRQ].level) { > + opp->src[n_IRQ].pending = opp->src[n_IRQ].cur_level; > + } > break; > > case IRQ_TYPE_FSLINT: > @@ -687,6 +707,7 @@ static inline void write_IRQreg_ivpr(OpenPICState *opp, int n_IRQ, uint32_t val) > break; > } > > + /* Re-evaluate a level irq */ > openpic_update_irq(opp, n_IRQ); > DPRINTF("Set IVPR %d to 0x%08x -> 0x%08x\n", n_IRQ, val, > opp->src[n_IRQ].ivpr); > @@ -1232,7 +1253,7 @@ static uint32_t openpic_iack(OpenPICState *opp, IRQDest *dst, int cpu) > } > > if (!src->level) { > - /* edge-sensitive IRQ */ > + /* edge-sensitive IRQ or level dropped */ > src->ivpr &= ~IVPR_ACTIVITY_MASK; > src->pending = 0; > IRQ_resetbit(&dst->raised, irq); > @@ -1564,6 +1585,7 @@ static const VMStateDescription vmstate_openpic_irqsource = { > VMSTATE_UINT32(destmask, IRQSource), > VMSTATE_INT32(last_cpu, IRQSource), > VMSTATE_INT32(pending, IRQSource), > + VMSTATE_INT32(cur_level, IRQSource), > VMSTATE_END_OF_LIST() This alters the migration stream without bumping the version number. I suspect it will be best to move this hunk into your other patch updating the migration of the openpic. > } > };
On 18/09/17 21:39, David Gibson wrote: > On Sun, Sep 17, 2017 at 06:15:48PM +0100, Mark Cave-Ayland wrote: >> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> >> Some interrupts get triggered before the OS has setup the >> right interrupt type. If an edge interrupt is latched that >> way, not delivered (still masked), then the interrupt is >> changed to level and isn't asserted anymore, it will be >> stuck "pending", causing an interrupt flood. This can happen >> with the PMU GPIO interrupt for example. >> >> There are a few other corner cases like this, so let's keep >> track of the input "level" so we can properly re-evaluate >> when the trigger type changes. >> >> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> --- >> hw/intc/openpic.c | 32 +++++++++++++++++++++++++++----- >> 1 file changed, 27 insertions(+), 5 deletions(-) >> >> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c >> index debfcbf..34749f8 100644 >> --- a/hw/intc/openpic.c >> +++ b/hw/intc/openpic.c >> @@ -236,6 +236,7 @@ typedef struct IRQSource { >> int last_cpu; >> int output; /* IRQ level, e.g. OPENPIC_OUTPUT_INT */ >> int pending; /* TRUE if IRQ is pending */ >> + int cur_level; /* Cache current level */ >> IRQType type; >> bool level:1; /* level-triggered */ >> bool nomask:1; /* critical interrupts ignore mask on some FSL MPICs */ >> @@ -552,14 +553,26 @@ static void openpic_set_irq(void *opaque, int n_IRQ, int level) >> } >> >> src = &opp->src[n_IRQ]; >> - DPRINTF("openpic: set irq %d = %d ivpr=0x%08x\n", >> - n_IRQ, level, src->ivpr); >> + DPRINTF("openpic: set irq %d = %d ivpr=0x%08x (l=%d,cl=%d)\n", >> + n_IRQ, level, src->ivpr, src->level, src->cur_level); >> + >> + /* Keep track of the current input level in order to properly deal >> + * with the configuration of the source changing from edge to level >> + * after it's been latched. Otherwise the interrupt can get stuck. >> + */ >> + src->cur_level = level; >> + >> if (src->level) { >> - /* level-sensitive irq */ >> src->pending = level; >> openpic_update_irq(opp, n_IRQ); >> } else { >> - /* edge-sensitive irq */ >> + /* edge-sensitive irq >> + * >> + * In an ideal world we would only set pending on an "edge", ie >> + * if level is set and src->cur_level as clear. However that would >> + * require all the devices to properly send "pulses" rather than >> + * just "raise" which isn't the case today. >> + */ >> if (level) { >> src->pending = 1; >> openpic_update_irq(opp, n_IRQ); >> @@ -676,6 +689,13 @@ static inline void write_IRQreg_ivpr(OpenPICState *opp, int n_IRQ, uint32_t val) >> switch (opp->src[n_IRQ].type) { >> case IRQ_TYPE_NORMAL: >> opp->src[n_IRQ].level = !!(opp->src[n_IRQ].ivpr & IVPR_SENSE_MASK); >> + >> + /* If we switched to level we need to re-evaluate "pending" based >> + * on the actual input state. >> + */ >> + if (opp->src[n_IRQ].level) { >> + opp->src[n_IRQ].pending = opp->src[n_IRQ].cur_level; >> + } >> break; >> >> case IRQ_TYPE_FSLINT: >> @@ -687,6 +707,7 @@ static inline void write_IRQreg_ivpr(OpenPICState *opp, int n_IRQ, uint32_t val) >> break; >> } >> >> + /* Re-evaluate a level irq */ >> openpic_update_irq(opp, n_IRQ); >> DPRINTF("Set IVPR %d to 0x%08x -> 0x%08x\n", n_IRQ, val, >> opp->src[n_IRQ].ivpr); >> @@ -1232,7 +1253,7 @@ static uint32_t openpic_iack(OpenPICState *opp, IRQDest *dst, int cpu) >> } >> >> if (!src->level) { >> - /* edge-sensitive IRQ */ >> + /* edge-sensitive IRQ or level dropped */ >> src->ivpr &= ~IVPR_ACTIVITY_MASK; >> src->pending = 0; >> IRQ_resetbit(&dst->raised, irq); >> @@ -1564,6 +1585,7 @@ static const VMStateDescription vmstate_openpic_irqsource = { >> VMSTATE_UINT32(destmask, IRQSource), >> VMSTATE_INT32(last_cpu, IRQSource), >> VMSTATE_INT32(pending, IRQSource), >> + VMSTATE_INT32(cur_level, IRQSource), >> VMSTATE_END_OF_LIST() > > This alters the migration stream without bumping the version number. > I suspect it will be best to move this hunk into your other patch > updating the migration of the openpic. Yes, that's fine. I'll wait for your reply to the previous openpic patch before resubmitting though. ATB, Mark.
diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c index debfcbf..34749f8 100644 --- a/hw/intc/openpic.c +++ b/hw/intc/openpic.c @@ -236,6 +236,7 @@ typedef struct IRQSource { int last_cpu; int output; /* IRQ level, e.g. OPENPIC_OUTPUT_INT */ int pending; /* TRUE if IRQ is pending */ + int cur_level; /* Cache current level */ IRQType type; bool level:1; /* level-triggered */ bool nomask:1; /* critical interrupts ignore mask on some FSL MPICs */ @@ -552,14 +553,26 @@ static void openpic_set_irq(void *opaque, int n_IRQ, int level) } src = &opp->src[n_IRQ]; - DPRINTF("openpic: set irq %d = %d ivpr=0x%08x\n", - n_IRQ, level, src->ivpr); + DPRINTF("openpic: set irq %d = %d ivpr=0x%08x (l=%d,cl=%d)\n", + n_IRQ, level, src->ivpr, src->level, src->cur_level); + + /* Keep track of the current input level in order to properly deal + * with the configuration of the source changing from edge to level + * after it's been latched. Otherwise the interrupt can get stuck. + */ + src->cur_level = level; + if (src->level) { - /* level-sensitive irq */ src->pending = level; openpic_update_irq(opp, n_IRQ); } else { - /* edge-sensitive irq */ + /* edge-sensitive irq + * + * In an ideal world we would only set pending on an "edge", ie + * if level is set and src->cur_level as clear. However that would + * require all the devices to properly send "pulses" rather than + * just "raise" which isn't the case today. + */ if (level) { src->pending = 1; openpic_update_irq(opp, n_IRQ); @@ -676,6 +689,13 @@ static inline void write_IRQreg_ivpr(OpenPICState *opp, int n_IRQ, uint32_t val) switch (opp->src[n_IRQ].type) { case IRQ_TYPE_NORMAL: opp->src[n_IRQ].level = !!(opp->src[n_IRQ].ivpr & IVPR_SENSE_MASK); + + /* If we switched to level we need to re-evaluate "pending" based + * on the actual input state. + */ + if (opp->src[n_IRQ].level) { + opp->src[n_IRQ].pending = opp->src[n_IRQ].cur_level; + } break; case IRQ_TYPE_FSLINT: @@ -687,6 +707,7 @@ static inline void write_IRQreg_ivpr(OpenPICState *opp, int n_IRQ, uint32_t val) break; } + /* Re-evaluate a level irq */ openpic_update_irq(opp, n_IRQ); DPRINTF("Set IVPR %d to 0x%08x -> 0x%08x\n", n_IRQ, val, opp->src[n_IRQ].ivpr); @@ -1232,7 +1253,7 @@ static uint32_t openpic_iack(OpenPICState *opp, IRQDest *dst, int cpu) } if (!src->level) { - /* edge-sensitive IRQ */ + /* edge-sensitive IRQ or level dropped */ src->ivpr &= ~IVPR_ACTIVITY_MASK; src->pending = 0; IRQ_resetbit(&dst->raised, irq); @@ -1564,6 +1585,7 @@ static const VMStateDescription vmstate_openpic_irqsource = { VMSTATE_UINT32(destmask, IRQSource), VMSTATE_INT32(last_cpu, IRQSource), VMSTATE_INT32(pending, IRQSource), + VMSTATE_INT32(cur_level, IRQSource), VMSTATE_END_OF_LIST() } };