On 10/15/2021 9:27 AM, Fenglin Wu wrote:
On 10/15/2021 9:09 AM, Stephen Boyd wrote:
Quoting Fenglin Wu (2021-10-13 19:26:55)
On 10/14/2021 3:35 AM, Stephen Boyd wrote:
Quoting Fenglin Wu (2021-10-12 21:15:42)
On 10/13/2021 1:46 AM, Stephen Boyd wrote:
Quoting Fenglin Wu (2021-09-16 23:32:56)
From: Abhijeet Dharmapurikar <adharmap@xxxxxxxxxxxxxx>
The cleanup_irq() was meant to clear and mask interrupts that were
left enabled in the hardware but there was no interrupt handler
registered for it. Add an error print when it gets invoked.
Why? Don't we get the genirq spurious irq message in this scenario?
Thanks for reviewing the change.
No, there is no existing message printed out in this special case
( IRQ
fired for not registered interrupt).
Ah I see so the irq doesn't have a flow handler? Shouldn't you call
handle_bad_irq() in this case so we get a irq descriptor print?
In such case, the irq number is not valid and there won't be a valid
irq_desc, hence it's not possible to call handle_bad_irq() here.
I mean handle_bad_irq() on the irqdesc for the spmi pmic arb chained
irq. Because things are not good with the chained irq.
Okay, how about this, Update periph_interrupt() function with a return
value, and return -EINVAL once an invalid IRQ is detected. In
pmic_arb_chained_irq(), call handle_bad_irq() if periph_interrupt()
returned -EINVAL.
Combined with your comments in "[PATCH v1 3/9] spmi: pmic-arb:check apid
againstlimits before calling irq handler",it seemslike that it can be
a independentpatch for handling spuriousinterrupt, something like this
in my mind:
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 295e19f..bd01ad4 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -504,10 +504,10 @@ static void cleanup_irq(struct spmi_pmic_arb
*pmic_arb, u16 apid, int id)
irq_mask, ppid);
}
-static void periph_interrupt(struct spmi_pmic_arb *pmic_arb, u16 apid)
+static int periph_interrupt(struct spmi_pmic_arb *pmic_arb, u16 apid)
{
unsigned int irq;
- u32 status, id;
+ u32 status, id, handled = 0;
u8 sid = (pmic_arb->apid_data[apid].ppid >> 8) & 0xF;
u8 per = pmic_arb->apid_data[apid].ppid & 0xFF;
@@ -522,7 +522,10 @@ static void periph_interrupt(struct spmi_pmic_arb
*pmic_arb, u16 apid)
continue;
}
generic_handle_irq(irq);
+ handled++;
}
+
+ return (handled == 0) ? -EINVAL : 0;
}
static void pmic_arb_chained_irq(struct irq_desc *desc)
@@ -533,7 +536,7 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
int first = pmic_arb->min_apid >> 5;
int last = pmic_arb->max_apid >> 5;
u8 ee = pmic_arb->ee;
- u32 status, enable;
+ u32 status, enable, handled = 0;
int i, id, apid;
chained_irq_enter(chip, desc);
@@ -548,10 +551,14 @@ static void pmic_arb_chained_irq(struct irq_desc
*desc)
enable = readl_relaxed(
ver_ops->acc_enable(pmic_arb, apid));
if (enable & SPMI_PIC_ACC_ENABLE_BIT)
- periph_interrupt(pmic_arb, apid);
+ if (periph_interrupt(pmic_arb, apid) == 0)
+ handled++;
}
}
+ if (handled == 0)
+ handle_bad_irq(desc);
+
chained_irq_exit(chip, desc);
}
Is this what you expected? The original patch is only for printing a
debug message when any
sub-irq is detected as enabled but not registered, some other sub-IRQ
maybe still valid and
be handled after that, which means the chained-irq may still be a good
one.Should I keep
the original patch unchanged and submit a separate one to handle the
spuriousinterrupt?
