Message ID | 1638403212-29265-2-git-send-email-quic_fenglinw@quicinc.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | A bunch of fix and optimization patches in spmi-pmic-arb.c | expand |
On 02/12/2021 00:00, Fenglin Wu wrote: > Call handle_bad_irq() for handling spurious interrupt. While at it, > add an error print in cleanup_irq() for any spurious interrupt which > is fired but not having interrupt handler registered. Being excruciatingly pedantic, I'd suggest breaking this up into two patches, one for the ratelimit one for the logical change to the irq handling flow. > Signed-off-by: Abhijeet Dharmapurikar<adharmap@codeaurora.org> > Signed-off-by: David Collins<collinsd@codeaurora.org> > Signed-off-by: Fenglin Wu<quic_fenglinw@quicinc.com> > --- > drivers/spmi/spmi-pmic-arb.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c > index bbbd311..da629cc 100644 > --- a/drivers/spmi/spmi-pmic-arb.c > +++ b/drivers/spmi/spmi-pmic-arb.c > @@ -489,6 +489,8 @@ static void cleanup_irq(struct spmi_pmic_arb *pmic_arb, u16 apid, int id) > u8 per = ppid & 0xFF; > u8 irq_mask = BIT(id); > > + dev_err_ratelimited(&pmic_arb->spmic->dev, "%s apid=%d sid=0x%x per=0x%x irq=%d\n", > + __func__, apid, sid, per, id); > writel_relaxed(irq_mask, pmic_arb->ver_ops->irq_clear(pmic_arb, apid)); > > if (pmic_arb_write_cmd(pmic_arb->spmic, SPMI_CMD_EXT_WRITEL, sid, > @@ -502,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; If handled were an int > u8 sid = (pmic_arb->apid_data[apid].ppid >> 8) & 0xF; > u8 per = pmic_arb->apid_data[apid].ppid & 0xFF; > > @@ -520,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; > } you could "return handled;" and then have if (periph_interrupt(pmic_arb, apid)) handled++; later on Its not important I suppose but please do at least break this up into two separate patches. --- bod
On 2021/12/2 10:39, Bryan O'Donoghue wrote: > On 02/12/2021 00:00, Fenglin Wu wrote: >> Call handle_bad_irq() for handling spurious interrupt. While at it, >> add an error print in cleanup_irq() for any spurious interrupt which >> is fired but not having interrupt handler registered. > > Being excruciatingly pedantic, I'd suggest breaking this up into two > patches, one for the ratelimit one for the logical change to the irq > handling flow. > The original patch actually only prints a message for any interrupt that's fired but not registered, and it got reviewed and commented to add logic to handle spurious interrupt like this.I might have misunderstood the comments so I combined them together, I agreed theyare not very related and I can separate them and send them again. Thanks. >> Signed-off-by: Abhijeet Dharmapurikar<adharmap@codeaurora.org> >> Signed-off-by: David Collins<collinsd@codeaurora.org> >> Signed-off-by: Fenglin Wu<quic_fenglinw@quicinc.com> >> --- >> drivers/spmi/spmi-pmic-arb.c | 17 +++++++++++++---- >> 1 file changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c >> index bbbd311..da629cc 100644 >> --- a/drivers/spmi/spmi-pmic-arb.c >> +++ b/drivers/spmi/spmi-pmic-arb.c >> @@ -489,6 +489,8 @@ static void cleanup_irq(struct spmi_pmic_arb >> *pmic_arb, u16 apid, int id) >> u8 per = ppid & 0xFF; >> u8 irq_mask = BIT(id); >> + dev_err_ratelimited(&pmic_arb->spmic->dev, "%s apid=%d >> sid=0x%x per=0x%x irq=%d\n", >> + __func__, apid, sid, per, id); >> writel_relaxed(irq_mask, pmic_arb->ver_ops->irq_clear(pmic_arb, >> apid)); >> if (pmic_arb_write_cmd(pmic_arb->spmic, SPMI_CMD_EXT_WRITEL, >> sid, >> @@ -502,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; > > If handled were an int > >> u8 sid = (pmic_arb->apid_data[apid].ppid >> 8) & 0xF; >> u8 per = pmic_arb->apid_data[apid].ppid & 0xFF; >> @@ -520,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; >> } > > you could "return handled;" and then have > > if (periph_interrupt(pmic_arb, apid)) > handled++; > > later on > > Its not important I suppose but please do at least break this up into > two separate patches. > Got it, will update it. Thanks > --- > bod
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c index bbbd311..da629cc 100644 --- a/drivers/spmi/spmi-pmic-arb.c +++ b/drivers/spmi/spmi-pmic-arb.c @@ -489,6 +489,8 @@ static void cleanup_irq(struct spmi_pmic_arb *pmic_arb, u16 apid, int id) u8 per = ppid & 0xFF; u8 irq_mask = BIT(id); + dev_err_ratelimited(&pmic_arb->spmic->dev, "%s apid=%d sid=0x%x per=0x%x irq=%d\n", + __func__, apid, sid, per, id); writel_relaxed(irq_mask, pmic_arb->ver_ops->irq_clear(pmic_arb, apid)); if (pmic_arb_write_cmd(pmic_arb->spmic, SPMI_CMD_EXT_WRITEL, sid, @@ -502,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; @@ -520,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; } static void pmic_arb_chained_irq(struct irq_desc *desc) @@ -531,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); @@ -546,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); }