Just want to resend the series of the changes again and see if anyone can
help to review them and give any comments. Thanks!
This change series includes some fixes and optimizations in spmi-pmic-arb.c.
Please see change detail and description in each of the patch. Thanks!
Abhijeet Dharmapurikar (1):
spmi: pmic-arb: add a print in cleanup_irq
Ashay Jaiswal (1):
spmi: pmic-arb: add support to dispatch interrupt based on IRQ status
David Collins (5):
spmi: pmic-arb: check apid against limits before calling irq handler
spmi: pmic-arb: correct duplicate APID to PPID mapping logic
spmi: pmic-arb: block access for invalid PMIC arbiter v5 SPMI writes
spmi: pmic-arb: make interrupt support optional
spmi: pmic-arb: increase SPMI transaction timeout delay
Subbaraman Narayanamurthy (1):
spmi: pmic-arb: do not ack and clear peripheral interrupts in
cleanup_irq
Yimin Peng (1):
spmi: pmic-arb: support updating interrupt type flags
drivers/spmi/spmi-pmic-arb.c | 127 +++++++++++++++++++++++++++++++------------
1 file changed, 91 insertions(+), 36 deletions(-)
--
2.7.4
From: David Collins <[email protected]>
The system crashes due to an access permission violation when
writing to a PMIC peripheral which is not owned by the current
ee. Add a check for PMIC arbiter version 5 for such invalid
write requests and return an error instead of crashing the
system.
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index f1b72d8..9239830 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -1020,6 +1020,11 @@ static int pmic_arb_offset_v5(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
offset = 0x10000 * pmic_arb->ee + 0x80 * apid;
break;
case PMIC_ARB_CHANNEL_RW:
+ if (pmic_arb->apid_data[apid].write_ee != pmic_arb->ee) {
+ dev_err(&pmic_arb->spmic->dev, "disallowed SPMI write to sid=%u, addr=0x%04X\n",
+ sid, addr);
+ return -EPERM;
+ }
offset = 0x10000 * apid;
break;
}
--
2.7.4
From: Yimin Peng <[email protected]>
Have the qpnpint_irq_set_type function clear unwanted high/low
trigger bits when updating the interrupt flags.
Signed-off-by: Yimin Peng <[email protected]>
Signed-off-by: Subbaraman Narayanamurthy <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 9239830..988204c 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -636,8 +636,12 @@ static int qpnpint_irq_set_type(struct irq_data *d, unsigned int flow_type)
type.type |= BIT(irq);
if (flow_type & IRQF_TRIGGER_RISING)
type.polarity_high |= BIT(irq);
+ else
+ type.polarity_high &= ~BIT(irq);
if (flow_type & IRQF_TRIGGER_FALLING)
type.polarity_low |= BIT(irq);
+ else
+ type.polarity_low &= ~BIT(irq);
flow_handler = handle_edge_irq;
} else {
@@ -646,10 +650,13 @@ static int qpnpint_irq_set_type(struct irq_data *d, unsigned int flow_type)
return -EINVAL;
type.type &= ~BIT(irq); /* level trig */
- if (flow_type & IRQF_TRIGGER_HIGH)
+ if (flow_type & IRQF_TRIGGER_HIGH) {
type.polarity_high |= BIT(irq);
- else
+ type.polarity_low &= ~BIT(irq);
+ } else {
type.polarity_low |= BIT(irq);
+ type.polarity_high &= ~BIT(irq);
+ }
flow_handler = handle_level_irq;
}
--
2.7.4
From: David Collins <[email protected]>
Check that the apid for an SPMI interrupt falls between the
min_apid and max_apid that can be handled by the APPS processor
before invoking the per-apid interrupt handler:
periph_interrupt().
This avoids an access violation in rare cases where the status
bit is set for an interrupt that is not owned by the APPS
processor.
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 4d7ad004..c4adc06 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -535,6 +535,12 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
id = ffs(status) - 1;
status &= ~BIT(id);
apid = id + i * 32;
+ if (apid < pmic_arb->min_apid
+ || apid > pmic_arb->max_apid) {
+ WARN_ONCE(true, "spurious spmi irq received for apid=%d\n",
+ apid);
+ continue;
+ }
enable = readl_relaxed(
ver_ops->acc_enable(pmic_arb, apid));
if (enable & SPMI_PIC_ACC_ENABLE_BIT)
--
2.7.4
From: David Collins <[email protected]>
Increase the SPMI transaction timeout delay from 100 us to
1000 us in order to account for the slower execution time
found on some simulator targets.
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 55fa981..08c2566 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -91,7 +91,7 @@ enum pmic_arb_channel {
/* Maximum number of support PMIC peripherals */
#define PMIC_ARB_MAX_PERIPHS 512
-#define PMIC_ARB_TIMEOUT_US 100
+#define PMIC_ARB_TIMEOUT_US 1000
#define PMIC_ARB_MAX_TRANS_BYTES (8)
#define PMIC_ARB_APID_MASK 0xFF
--
2.7.4
From: Ashay Jaiswal <[email protected]>
Current implementation of SPMI arbiter dispatches interrupt based on the
Arbiter's accumulator status, in some cases the accumulator status may
remain zero and the interrupt remains un-handled. Add logic to dispatch
interrupts based Arbiter's IRQ status if the accumulator status is zero.
Signed-off-by: Ashay Jaiswal <[email protected]>
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index c4adc06..59c445b 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -525,12 +525,18 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
u8 ee = pmic_arb->ee;
u32 status, enable;
int i, id, apid;
+ /* status based dispatch */
+ bool acc_valid = false;
+ u32 irq_status = 0;
chained_irq_enter(chip, desc);
for (i = first; i <= last; ++i) {
status = readl_relaxed(
ver_ops->owner_acc_status(pmic_arb, ee, i));
+ if (status)
+ acc_valid = true;
+
while (status) {
id = ffs(status) - 1;
status &= ~BIT(id);
@@ -548,6 +554,28 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
}
}
+ /* ACC_STATUS is empty but IRQ fired check IRQ_STATUS */
+ if (!acc_valid) {
+ for (i = pmic_arb->min_apid; i <= pmic_arb->max_apid; i++) {
+ /* skip if APPS is not irq owner */
+ if (pmic_arb->apid_data[i].irq_ee != pmic_arb->ee)
+ continue;
+
+ irq_status = readl_relaxed(
+ ver_ops->irq_status(pmic_arb, i));
+ if (irq_status) {
+ enable = readl_relaxed(
+ ver_ops->acc_enable(pmic_arb, i));
+ if (enable & SPMI_PIC_ACC_ENABLE_BIT) {
+ dev_dbg(&pmic_arb->spmic->dev,
+ "Dispatching IRQ for apid=%d status=%x\n",
+ i, irq_status);
+ periph_interrupt(pmic_arb, i);
+ }
+ }
+ }
+ }
+
chained_irq_exit(chip, desc);
}
--
2.7.4
From: Abhijeet Dharmapurikar <[email protected]>
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.
Signed-off-by: Abhijeet Dharmapurikar <[email protected]>
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index bbbd311..295e19f 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,
--
2.7.4
From: David Collins <[email protected]>
Make the support of PMIC peripheral interrupts optional for
spmi-pmic-arb devices. This is useful in situations where
SPMI address mapping is required without the need for IRQ
support.
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 45 +++++++++++++++++++++++++++-----------------
1 file changed, 28 insertions(+), 17 deletions(-)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 988204c..55fa981 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -1280,10 +1280,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
goto err_put_ctrl;
}
- pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
- if (pmic_arb->irq < 0) {
- err = pmic_arb->irq;
- goto err_put_ctrl;
+ if (of_find_property(pdev->dev.of_node, "interrupt-names", NULL)) {
+ pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
+ if (pmic_arb->irq < 0) {
+ err = pmic_arb->irq;
+ goto err_put_ctrl;
+ }
}
err = of_property_read_u32(pdev->dev.of_node, "qcom,channel", &channel);
@@ -1343,17 +1345,22 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
}
}
- dev_dbg(&pdev->dev, "adding irq domain\n");
- pmic_arb->domain = irq_domain_add_tree(pdev->dev.of_node,
- &pmic_arb_irq_domain_ops, pmic_arb);
- if (!pmic_arb->domain) {
- dev_err(&pdev->dev, "unable to create irq_domain\n");
- err = -ENOMEM;
- goto err_put_ctrl;
+ if (pmic_arb->irq > 0) {
+ dev_dbg(&pdev->dev, "adding irq domain\n");
+ pmic_arb->domain = irq_domain_add_tree(pdev->dev.of_node,
+ &pmic_arb_irq_domain_ops, pmic_arb);
+ if (!pmic_arb->domain) {
+ dev_err(&pdev->dev, "unable to create irq_domain\n");
+ err = -ENOMEM;
+ goto err_put_ctrl;
+ }
+
+ irq_set_chained_handler_and_data(pmic_arb->irq,
+ pmic_arb_chained_irq, pmic_arb);
+ } else {
+ dev_dbg(&pdev->dev, "not supporting PMIC interrupts\n");
}
- irq_set_chained_handler_and_data(pmic_arb->irq, pmic_arb_chained_irq,
- pmic_arb);
err = spmi_controller_add(ctrl);
if (err)
goto err_domain_remove;
@@ -1361,8 +1368,10 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
return 0;
err_domain_remove:
- irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
- irq_domain_remove(pmic_arb->domain);
+ if (pmic_arb->irq > 0) {
+ irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
+ irq_domain_remove(pmic_arb->domain);
+ }
err_put_ctrl:
spmi_controller_put(ctrl);
return err;
@@ -1373,8 +1382,10 @@ static int spmi_pmic_arb_remove(struct platform_device *pdev)
struct spmi_controller *ctrl = platform_get_drvdata(pdev);
struct spmi_pmic_arb *pmic_arb = spmi_controller_get_drvdata(ctrl);
spmi_controller_remove(ctrl);
- irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
- irq_domain_remove(pmic_arb->domain);
+ if (pmic_arb->irq > 0) {
+ irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
+ irq_domain_remove(pmic_arb->domain);
+ }
spmi_controller_put(ctrl);
return 0;
}
--
2.7.4
Quoting Fenglin Wu (2021-09-16 23:33:03)
> From: David Collins <[email protected]>
>
> Make the support of PMIC peripheral interrupts optional for
> spmi-pmic-arb devices. This is useful in situations where
> SPMI address mapping is required without the need for IRQ
> support.
>
> Signed-off-by: David Collins <[email protected]>
> Signed-off-by: Fenglin Wu <[email protected]>
> ---
> drivers/spmi/spmi-pmic-arb.c | 45 +++++++++++++++++++++++++++-----------------
Is there a binding update? Can the binding be converted to YAML as well?
> 1 file changed, 28 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 988204c..55fa981 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -1280,10 +1280,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> goto err_put_ctrl;
> }
>
> - pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
> - if (pmic_arb->irq < 0) {
> - err = pmic_arb->irq;
> - goto err_put_ctrl;
> + if (of_find_property(pdev->dev.of_node, "interrupt-names", NULL)) {
I don't think we should be keying off of interrupt-names. Instead we
should be checking for something else. Maybe interrupt-controller
property?
> + pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
> + if (pmic_arb->irq < 0) {
> + err = pmic_arb->irq;
> + goto err_put_ctrl;
Quoting Fenglin Wu (2021-09-16 23:33:02)
> From: Yimin Peng <[email protected]>
>
> Have the qpnpint_irq_set_type function clear unwanted high/low
> trigger bits when updating the interrupt flags.
Why?
>
> Signed-off-by: Yimin Peng <[email protected]>
> Signed-off-by: Subbaraman Narayanamurthy <[email protected]>
> Signed-off-by: Fenglin Wu <[email protected]>
> ---
Does this need a Fixes tag?
Quoting Fenglin Wu (2021-09-16 23:32:56)
> From: Abhijeet Dharmapurikar <[email protected]>
>
> 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?
>
> Signed-off-by: Abhijeet Dharmapurikar <[email protected]>
> Signed-off-by: David Collins <[email protected]>
> Signed-off-by: Fenglin Wu <[email protected]>
Quoting Fenglin Wu (2021-09-16 23:32:58)
> From: David Collins <[email protected]>
>
> Check that the apid for an SPMI interrupt falls between the
> min_apid and max_apid that can be handled by the APPS processor
> before invoking the per-apid interrupt handler:
> periph_interrupt().
>
> This avoids an access violation in rare cases where the status
> bit is set for an interrupt that is not owned by the APPS
> processor.
>
> Signed-off-by: David Collins <[email protected]>
> Signed-off-by: Fenglin Wu <[email protected]>
> ---
Fixes? BTW, a lot of these patches are irqchip specific. It would be
good to get review from irqchip maintainers. Maybe we should split the
irqchip driver off via the auxiliary bus so that irqchip maintainers can
review. Please Cc them on irqchip related patches.
IRQCHIP DRIVERS
M: Thomas Gleixner <[email protected]>
M: Marc Zyngier <[email protected]>
> drivers/spmi/spmi-pmic-arb.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 4d7ad004..c4adc06 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -535,6 +535,12 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
> id = ffs(status) - 1;
> status &= ~BIT(id);
> apid = id + i * 32;
> + if (apid < pmic_arb->min_apid
> + || apid > pmic_arb->max_apid) {
The || goes on the line above. What about making a local variable for
first and last and then shifting by 5 in the loop?
int first = pmic_arb->min_apid;
int last = pmic_arb->max_apid;
for (i = first >> 5; i <= last >> 5; i++)
if (apid < first || apid > last)
> + WARN_ONCE(true, "spurious spmi irq received for apid=%d\n",
> + apid);
Is there any way to recover from this? Or once the mapping is wrong
we're going to get interrupts that we don't know what to do with
forever?
> + continue;
> + }
> enable = readl_relaxed(
> ver_ops->acc_enable(pmic_arb, apid));
> if (enable & SPMI_PIC_ACC_ENABLE_BIT)
On 10/13/2021 1:46 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-09-16 23:32:56)
>> From: Abhijeet Dharmapurikar <[email protected]>
>>
>> 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).
>> Signed-off-by: Abhijeet Dharmapurikar <[email protected]>
>> Signed-off-by: David Collins <[email protected]>
>> Signed-off-by: Fenglin Wu <[email protected]>
On 10/13/2021 2:02 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-09-16 23:32:58)
>> From: David Collins <[email protected]>
>>
>> Check that the apid for an SPMI interrupt falls between the
>> min_apid and max_apid that can be handled by the APPS processor
>> before invoking the per-apid interrupt handler:
>> periph_interrupt().
>>
>> This avoids an access violation in rare cases where the status
>> bit is set for an interrupt that is not owned by the APPS
>> processor.
>>
>> Signed-off-by: David Collins <[email protected]>
>> Signed-off-by: Fenglin Wu <[email protected]>
>> ---
> Fixes? BTW, a lot of these patches are irqchip specific. It would be
> good to get review from irqchip maintainers. Maybe we should split the
> irqchip driver off via the auxiliary bus so that irqchip maintainers can
> review. Please Cc them on irqchip related patches.
>
> IRQCHIP DRIVERS
> M: Thomas Gleixner <[email protected]>
> M: Marc Zyngier <[email protected]>
Sure, copied Thomas and Marc for code review.
This is a fix to avoid the register access violation in a case that an
interrupt is fired in a PMIC module which is not owned by APPS
processor.
>> drivers/spmi/spmi-pmic-arb.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
>> index 4d7ad004..c4adc06 100644
>> --- a/drivers/spmi/spmi-pmic-arb.c
>> +++ b/drivers/spmi/spmi-pmic-arb.c
>> @@ -535,6 +535,12 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
>> id = ffs(status) - 1;
>> status &= ~BIT(id);
>> apid = id + i * 32;
>> + if (apid < pmic_arb->min_apid
>> + || apid > pmic_arb->max_apid) {
> The || goes on the line above. What about making a local variable for
> first and last and then shifting by 5 in the loop?
>
> int first = pmic_arb->min_apid;
> int last = pmic_arb->max_apid;
>
> for (i = first >> 5; i <= last >> 5; i++)
>
> if (apid < first || apid > last)
ACK, will update it following this.
>> + WARN_ONCE(true, "spurious spmi irq received for apid=%d\n",
>> + apid);
> Is there any way to recover from this? Or once the mapping is wrong
> we're going to get interrupts that we don't know what to do with
> forever?
This is a rare case that the unexpected interrupt is fired in a module
not owned by APPS process, so the interrupt itself is not expected hence
no need to recover from this but just bail out to avoid following register
access violation.
>> + continue;
>> + }
>> enable = readl_relaxed(
>> ver_ops->acc_enable(pmic_arb, apid));
>> if (enable & SPMI_PIC_ACC_ENABLE_BIT)
copy IRQCHIP driver maintainers as requested in another patch.
On 10/13/2021 1:42 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-09-16 23:33:02)
>> From: Yimin Peng <[email protected]>
>>
>> Have the qpnpint_irq_set_type function clear unwanted high/low
>> trigger bits when updating the interrupt flags.
> Why?
There was a requirement to update the PMIC module interrupt type
dynamically
(such as from low level trigger to high level trigger), hence it's required
to clear the unnecessary trigger type when setting it.
>> Signed-off-by: Yimin Peng <[email protected]>
>> Signed-off-by: Subbaraman Narayanamurthy <[email protected]>
>> Signed-off-by: Fenglin Wu <[email protected]>
>> ---
> Does this need a Fixes tag?
Maybe no need to a Fixes tag because this is part of the initial code when
interrupt handling is added?
On 10/13/2021 1:41 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-09-16 23:33:03)
>> From: David Collins <[email protected]>
>>
>> Make the support of PMIC peripheral interrupts optional for
>> spmi-pmic-arb devices. This is useful in situations where
>> SPMI address mapping is required without the need for IRQ
>> support.
>>
>> Signed-off-by: David Collins <[email protected]>
>> Signed-off-by: Fenglin Wu <[email protected]>
>> ---
>> drivers/spmi/spmi-pmic-arb.c | 45 +++++++++++++++++++++++++++-----------------
> Is there a binding update? Can the binding be converted to YAML as well?
This change doesn't add/update any dtsi properties but just checking if an
existing property is present to decide if IRQ support is required, so no
binding change is needed.
>
>> 1 file changed, 28 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
>> index 988204c..55fa981 100644
>> --- a/drivers/spmi/spmi-pmic-arb.c
>> +++ b/drivers/spmi/spmi-pmic-arb.c
>> @@ -1280,10 +1280,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>> goto err_put_ctrl;
>> }
>>
>> - pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
>> - if (pmic_arb->irq < 0) {
>> - err = pmic_arb->irq;
>> - goto err_put_ctrl;
>> + if (of_find_property(pdev->dev.of_node, "interrupt-names", NULL)) {
> I don't think we should be keying off of interrupt-names. Instead we
> should be checking for something else. Maybe interrupt-controller
> property?
Sure, I can update it to check the presence of "interrupt-controller"
property.
>> + pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
>> + if (pmic_arb->irq < 0) {
>> + err = pmic_arb->irq;
>> + goto err_put_ctrl;
Quoting Fenglin Wu (2021-10-12 22:31:22)
>
> On 10/13/2021 2:02 AM, Stephen Boyd wrote:
> > Quoting Fenglin Wu (2021-09-16 23:32:58)
> >> From: David Collins <[email protected]>
> >>
> >> Check that the apid for an SPMI interrupt falls between the
> >> min_apid and max_apid that can be handled by the APPS processor
> >> before invoking the per-apid interrupt handler:
> >> periph_interrupt().
> >>
> >> This avoids an access violation in rare cases where the status
> >> bit is set for an interrupt that is not owned by the APPS
> >> processor.
> >>
> >> Signed-off-by: David Collins <[email protected]>
> >> Signed-off-by: Fenglin Wu <[email protected]>
> >> ---
> > Fixes? BTW, a lot of these patches are irqchip specific. It would be
> > good to get review from irqchip maintainers. Maybe we should split the
> > irqchip driver off via the auxiliary bus so that irqchip maintainers can
> > review. Please Cc them on irqchip related patches.
> >
> > IRQCHIP DRIVERS
> > M: Thomas Gleixner <[email protected]>
> > M: Marc Zyngier <[email protected]>
> Sure, copied Thomas and Marc for code review.
> This is a fix to avoid the register access violation in a case that an
> interrupt is fired in a PMIC module which is not owned by APPS
> processor.
Got it.
> >> drivers/spmi/spmi-pmic-arb.c | 6 ++++++
> >> 1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> >> index 4d7ad004..c4adc06 100644
> >> --- a/drivers/spmi/spmi-pmic-arb.c
> >> +++ b/drivers/spmi/spmi-pmic-arb.c
> >> @@ -535,6 +535,12 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
> >> id = ffs(status) - 1;
> >> status &= ~BIT(id);
> >> apid = id + i * 32;
> >> + if (apid < pmic_arb->min_apid
> >> + || apid > pmic_arb->max_apid) {
> > The || goes on the line above. What about making a local variable for
> > first and last and then shifting by 5 in the loop?
> >
> > int first = pmic_arb->min_apid;
> > int last = pmic_arb->max_apid;
> >
> > for (i = first >> 5; i <= last >> 5; i++)
> >
> > if (apid < first || apid > last)
> ACK, will update it following this.
> >> + WARN_ONCE(true, "spurious spmi irq received for apid=%d\n",
> >> + apid);
> > Is there any way to recover from this? Or once the mapping is wrong
> > we're going to get interrupts that we don't know what to do with
> > forever?
> This is a rare case that the unexpected interrupt is fired in a module
> not owned by APPS process, so the interrupt itself is not expected hence
> no need to recover from this but just bail out to avoid following register
> access violation.
And then the irq stops coming? It feels like a misconfiguration in the
firmware that we're trying to hide, hence the WARN_ONCE(). Can we
somehow silence irqs that aren't owned by the APPS when this driver
probes so that they can't even happen after probe?
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 <[email protected]>
> >>
> >> 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?
Quoting Fenglin Wu (2021-10-13 01:36:54)
>
> On 10/13/2021 1:41 AM, Stephen Boyd wrote:
> > Quoting Fenglin Wu (2021-09-16 23:33:03)
> >> From: David Collins <[email protected]>
> >>
> >> Make the support of PMIC peripheral interrupts optional for
> >> spmi-pmic-arb devices. This is useful in situations where
> >> SPMI address mapping is required without the need for IRQ
> >> support.
> >>
> >> Signed-off-by: David Collins <[email protected]>
> >> Signed-off-by: Fenglin Wu <[email protected]>
> >> ---
> >> drivers/spmi/spmi-pmic-arb.c | 45 +++++++++++++++++++++++++++-----------------
> > Is there a binding update? Can the binding be converted to YAML as well?
> This change doesn't add/update any dtsi properties but just checking if an
> existing property is present to decide if IRQ support is required, so no
> binding change is needed.
The property is now optional in the binding. Please update the binding.
> >
> >> 1 file changed, 28 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> >> index 988204c..55fa981 100644
> >> --- a/drivers/spmi/spmi-pmic-arb.c
> >> +++ b/drivers/spmi/spmi-pmic-arb.c
> >> @@ -1280,10 +1280,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> >> goto err_put_ctrl;
> >> }
> >>
> >> - pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
> >> - if (pmic_arb->irq < 0) {
> >> - err = pmic_arb->irq;
> >> - goto err_put_ctrl;
> >> + if (of_find_property(pdev->dev.of_node, "interrupt-names", NULL)) {
> > I don't think we should be keying off of interrupt-names. Instead we
> > should be checking for something else. Maybe interrupt-controller
> > property?
> Sure, I can update it to check the presence of "interrupt-controller"
> property.
Ok.
Quoting Fenglin Wu (2021-10-12 23:27:22)
> copy IRQCHIP driver maintainers as requested in another patch.
>
> On 10/13/2021 1:42 AM, Stephen Boyd wrote:
> > Quoting Fenglin Wu (2021-09-16 23:33:02)
> >> From: Yimin Peng <[email protected]>
> >>
> >> Have the qpnpint_irq_set_type function clear unwanted high/low
> >> trigger bits when updating the interrupt flags.
> > Why?
> There was a requirement to update the PMIC module interrupt type
> dynamically
> (such as from low level trigger to high level trigger), hence it's required
> to clear the unnecessary trigger type when setting it.
Can you clearly describe the problem in the commit text? Is this a bug
fix?
> >> Signed-off-by: Yimin Peng <[email protected]>
> >> Signed-off-by: Subbaraman Narayanamurthy <[email protected]>
> >> Signed-off-by: Fenglin Wu <[email protected]>
> >> ---
> > Does this need a Fixes tag?
> Maybe no need to a Fixes tag because this is part of the initial code when
> interrupt handling is added?
Was it always broken? Or trigger types haven't been changing at runtime
because most users are requesting irqs and forgetting about it? Are you
using gpio-keys or something like that now? Adding a Fixes tag doesn't
hurt.
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 <[email protected]>
>>>>
>>>> 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.
On 10/14/2021 3:25 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-10-12 22:31:22)
>> On 10/13/2021 2:02 AM, Stephen Boyd wrote:
>>> Quoting Fenglin Wu (2021-09-16 23:32:58)
>>>> From: David Collins <[email protected]>
>>>>
>>>> Check that the apid for an SPMI interrupt falls between the
>>>> min_apid and max_apid that can be handled by the APPS processor
>>>> before invoking the per-apid interrupt handler:
>>>> periph_interrupt().
>>>>
>>>> This avoids an access violation in rare cases where the status
>>>> bit is set for an interrupt that is not owned by the APPS
>>>> processor.
>>>>
>>>> Signed-off-by: David Collins <[email protected]>
>>>> Signed-off-by: Fenglin Wu <[email protected]>
>>>> ---
>>> Fixes? BTW, a lot of these patches are irqchip specific. It would be
>>> good to get review from irqchip maintainers. Maybe we should split the
>>> irqchip driver off via the auxiliary bus so that irqchip maintainers can
>>> review. Please Cc them on irqchip related patches.
>>>
>>> IRQCHIP DRIVERS
>>> M: Thomas Gleixner <[email protected]>
>>> M: Marc Zyngier <[email protected]>
>> Sure, copied Thomas and Marc for code review.
>> This is a fix to avoid the register access violation in a case that an
>> interrupt is fired in a PMIC module which is not owned by APPS
>> processor.
> Got it.
>
>>>> drivers/spmi/spmi-pmic-arb.c | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
>>>> index 4d7ad004..c4adc06 100644
>>>> --- a/drivers/spmi/spmi-pmic-arb.c
>>>> +++ b/drivers/spmi/spmi-pmic-arb.c
>>>> @@ -535,6 +535,12 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
>>>> id = ffs(status) - 1;
>>>> status &= ~BIT(id);
>>>> apid = id + i * 32;
>>>> + if (apid < pmic_arb->min_apid
>>>> + || apid > pmic_arb->max_apid) {
>>> The || goes on the line above. What about making a local variable for
>>> first and last and then shifting by 5 in the loop?
>>>
>>> int first = pmic_arb->min_apid;
>>> int last = pmic_arb->max_apid;
>>>
>>> for (i = first >> 5; i <= last >> 5; i++)
>>>
>>> if (apid < first || apid > last)
>> ACK, will update it following this.
>>>> + WARN_ONCE(true, "spurious spmi irq received for apid=%d\n",
>>>> + apid);
>>> Is there any way to recover from this? Or once the mapping is wrong
>>> we're going to get interrupts that we don't know what to do with
>>> forever?
>> This is a rare case that the unexpected interrupt is fired in a module
>> not owned by APPS process, so the interrupt itself is not expected hence
>> no need to recover from this but just bail out to avoid following register
>> access violation.
> And then the irq stops coming? It feels like a misconfiguration in the
> firmware that we're trying to hide, hence the WARN_ONCE(). Can we
> somehow silence irqs that aren't owned by the APPS when this driver
> probes so that they can't even happen after probe?
Actually this is a rarely happened case that couldn't be reproduced easily
and consistently for further debug. I agreed this should be caused by HW
misconfiguration or even some unknown HW bug that it would send out SPMI
interrupt messages with incorrect APID, but we have never had any chance
to find out the root cause. The patch here simply checked the APID and
bail out if it's not in the valid range, it won't cause anything bad but
improves the SW robustness. After that, the IRQ won't be triggered again
because the latched status in PMIC is not cleared. Also, because of the
access restriction to the registers corresponding to this APID, there is
nothing we can do from APPS processor side to keep it silent.
On 10/14/2021 3:37 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-10-12 23:27:22)
>> copy IRQCHIP driver maintainers as requested in another patch.
>>
>> On 10/13/2021 1:42 AM, Stephen Boyd wrote:
>>> Quoting Fenglin Wu (2021-09-16 23:33:02)
>>>> From: Yimin Peng <[email protected]>
>>>>
>>>> Have the qpnpint_irq_set_type function clear unwanted high/low
>>>> trigger bits when updating the interrupt flags.
>>> Why?
>> There was a requirement to update the PMIC module interrupt type
>> dynamically
>> (such as from low level trigger to high level trigger), hence it's required
>> to clear the unnecessary trigger type when setting it.
> Can you clearly describe the problem in the commit text? Is this a bug
> fix?
sure, will do.
>>>> Signed-off-by: Yimin Peng <[email protected]>
>>>> Signed-off-by: Subbaraman Narayanamurthy <[email protected]>
>>>> Signed-off-by: Fenglin Wu <[email protected]>
>>>> ---
>>> Does this need a Fixes tag?
>> Maybe no need to a Fixes tag because this is part of the initial code when
>> interrupt handling is added?
> Was it always broken? Or trigger types haven't been changing at runtime
> because most users are requesting irqs and forgetting about it? Are you
> using gpio-keys or something like that now? Adding a Fixes tag doesn't
> hurt.
You are right, it was reported by someone when using a PMIC GPIO and update
the interrupt at runtime, I am not sure if it's gpio-keys but that can be a
realistic case.
I will add a Fixes tag for it.
On 10/14/2021 3:38 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-10-13 01:36:54)
>> On 10/13/2021 1:41 AM, Stephen Boyd wrote:
>>> Quoting Fenglin Wu (2021-09-16 23:33:03)
>>>> From: David Collins <[email protected]>
>>>>
>>>> Make the support of PMIC peripheral interrupts optional for
>>>> spmi-pmic-arb devices. This is useful in situations where
>>>> SPMI address mapping is required without the need for IRQ
>>>> support.
>>>>
>>>> Signed-off-by: David Collins <[email protected]>
>>>> Signed-off-by: Fenglin Wu <[email protected]>
>>>> ---
>>>> drivers/spmi/spmi-pmic-arb.c | 45 +++++++++++++++++++++++++++-----------------
>>> Is there a binding update? Can the binding be converted to YAML as well?
>> This change doesn't add/update any dtsi properties but just checking if an
>> existing property is present to decide if IRQ support is required, so no
>> binding change is needed.
> The property is now optional in the binding. Please update the binding.
Right, thanks for pointing it out. I forgot that part.
I will update the binding. How about only update the interrupt properties as
optional in this series then I can come up with following patch to convert
the binding to YAML format?
>
>>>> 1 file changed, 28 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
>>>> index 988204c..55fa981 100644
>>>> --- a/drivers/spmi/spmi-pmic-arb.c
>>>> +++ b/drivers/spmi/spmi-pmic-arb.c
>>>> @@ -1280,10 +1280,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>>>> goto err_put_ctrl;
>>>> }
>>>>
>>>> - pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
>>>> - if (pmic_arb->irq < 0) {
>>>> - err = pmic_arb->irq;
>>>> - goto err_put_ctrl;
>>>> + if (of_find_property(pdev->dev.of_node, "interrupt-names", NULL)) {
>>> I don't think we should be keying off of interrupt-names. Instead we
>>> should be checking for something else. Maybe interrupt-controller
>>> property?
>> Sure, I can update it to check the presence of "interrupt-controller"
>> property.
> Ok.
Quoting Fenglin Wu (2021-10-13 20:11:40)
>
> On 10/14/2021 3:25 AM, Stephen Boyd wrote:
> > Quoting Fenglin Wu (2021-10-12 22:31:22)
> >> On 10/13/2021 2:02 AM, Stephen Boyd wrote:
> >>> Quoting Fenglin Wu (2021-09-16 23:32:58)
> >>>> From: David Collins <[email protected]>
> >>>>
> >>>> Check that the apid for an SPMI interrupt falls between the
> >>>> min_apid and max_apid that can be handled by the APPS processor
> >>>> before invoking the per-apid interrupt handler:
> >>>> periph_interrupt().
> >>>>
> >>>> This avoids an access violation in rare cases where the status
> >>>> bit is set for an interrupt that is not owned by the APPS
> >>>> processor.
> >>>>
> >>>> Signed-off-by: David Collins <[email protected]>
> >>>> Signed-off-by: Fenglin Wu <[email protected]>
> >>>> ---
> >>> Fixes? BTW, a lot of these patches are irqchip specific. It would be
> >>> good to get review from irqchip maintainers. Maybe we should split the
> >>> irqchip driver off via the auxiliary bus so that irqchip maintainers can
> >>> review. Please Cc them on irqchip related patches.
> >>>
> >>> IRQCHIP DRIVERS
> >>> M: Thomas Gleixner <[email protected]>
> >>> M: Marc Zyngier <[email protected]>
> >> Sure, copied Thomas and Marc for code review.
> >> This is a fix to avoid the register access violation in a case that an
> >> interrupt is fired in a PMIC module which is not owned by APPS
> >> processor.
> > Got it.
> >
> >>>> drivers/spmi/spmi-pmic-arb.c | 6 ++++++
> >>>> 1 file changed, 6 insertions(+)
> >>>>
> >>>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> >>>> index 4d7ad004..c4adc06 100644
> >>>> --- a/drivers/spmi/spmi-pmic-arb.c
> >>>> +++ b/drivers/spmi/spmi-pmic-arb.c
> >>>> @@ -535,6 +535,12 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
> >>>> id = ffs(status) - 1;
> >>>> status &= ~BIT(id);
> >>>> apid = id + i * 32;
> >>>> + if (apid < pmic_arb->min_apid
> >>>> + || apid > pmic_arb->max_apid) {
> >>> The || goes on the line above. What about making a local variable for
> >>> first and last and then shifting by 5 in the loop?
> >>>
> >>> int first = pmic_arb->min_apid;
> >>> int last = pmic_arb->max_apid;
> >>>
> >>> for (i = first >> 5; i <= last >> 5; i++)
> >>>
> >>> if (apid < first || apid > last)
> >> ACK, will update it following this.
> >>>> + WARN_ONCE(true, "spurious spmi irq received for apid=%d\n",
> >>>> + apid);
> >>> Is there any way to recover from this? Or once the mapping is wrong
> >>> we're going to get interrupts that we don't know what to do with
> >>> forever?
> >> This is a rare case that the unexpected interrupt is fired in a module
> >> not owned by APPS process, so the interrupt itself is not expected hence
> >> no need to recover from this but just bail out to avoid following register
> >> access violation.
> > And then the irq stops coming? It feels like a misconfiguration in the
> > firmware that we're trying to hide, hence the WARN_ONCE(). Can we
> > somehow silence irqs that aren't owned by the APPS when this driver
> > probes so that they can't even happen after probe?
> Actually this is a rarely happened case that couldn't be reproduced easily
> and consistently for further debug. I agreed this should be caused by HW
> misconfiguration or even some unknown HW bug that it would send out SPMI
> interrupt messages with incorrect APID, but we have never had any chance
> to find out the root cause. The patch here simply checked the APID and
> bail out if it's not in the valid range, it won't cause anything bad but
> improves the SW robustness. After that, the IRQ won't be triggered again
> because the latched status in PMIC is not cleared. Also, because of the
> access restriction to the registers corresponding to this APID, there is
> nothing we can do from APPS processor side to keep it silent.
This patch seems like a band-aid for an issue that isn't fully
understood. I suppose it's good that the irq will stay asserted forever
and then it won't happen again until it gets cleared by some other
processor in the SoC. Instead of the WARN_ONCE() can we track if any irq
is handled when the chained irq is raised, and if nothing is handled
then call handle_bad_irq() on the chained descriptor? Take a look at
pinctrl-msm.c to see how they handled spurious irqs that aren't actually
directed at the APPS processor. We should do something similar here.
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 <[email protected]>
> >>>>
> >>>> 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.
Quoting Fenglin Wu (2021-10-13 20:20:57)
>
> On 10/14/2021 3:38 AM, Stephen Boyd wrote:
> > Quoting Fenglin Wu (2021-10-13 01:36:54)
> >> On 10/13/2021 1:41 AM, Stephen Boyd wrote:
> >>> Quoting Fenglin Wu (2021-09-16 23:33:03)
> >>>> From: David Collins <[email protected]>
> >>>>
> >>>> Make the support of PMIC peripheral interrupts optional for
> >>>> spmi-pmic-arb devices. This is useful in situations where
> >>>> SPMI address mapping is required without the need for IRQ
> >>>> support.
> >>>>
> >>>> Signed-off-by: David Collins <[email protected]>
> >>>> Signed-off-by: Fenglin Wu <[email protected]>
> >>>> ---
> >>>> drivers/spmi/spmi-pmic-arb.c | 45 +++++++++++++++++++++++++++-----------------
> >>> Is there a binding update? Can the binding be converted to YAML as well?
> >> This change doesn't add/update any dtsi properties but just checking if an
> >> existing property is present to decide if IRQ support is required, so no
> >> binding change is needed.
> > The property is now optional in the binding. Please update the binding.
> Right, thanks for pointing it out. I forgot that part.
> I will update the binding. How about only update the interrupt properties as
> optional in this series then I can come up with following patch to convert
> the binding to YAML format?
Sure. The benefit of converting it to YAML is that we can use the
checker to quickly validate the binding vs. having to read the whole
thing to understand that it's correct. Converting an existing binding to
YAML shouldn't be that hard.
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 <[email protected]>
>>>>>>
>>>>>> 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.
On 10/15/2021 9:17 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-10-13 20:20:57)
>> On 10/14/2021 3:38 AM, Stephen Boyd wrote:
>>> Quoting Fenglin Wu (2021-10-13 01:36:54)
>>>> On 10/13/2021 1:41 AM, Stephen Boyd wrote:
>>>>> Quoting Fenglin Wu (2021-09-16 23:33:03)
>>>>>> From: David Collins <[email protected]>
>>>>>>
>>>>>> Make the support of PMIC peripheral interrupts optional for
>>>>>> spmi-pmic-arb devices. This is useful in situations where
>>>>>> SPMI address mapping is required without the need for IRQ
>>>>>> support.
>>>>>>
>>>>>> Signed-off-by: David Collins <[email protected]>
>>>>>> Signed-off-by: Fenglin Wu <[email protected]>
>>>>>> ---
>>>>>> drivers/spmi/spmi-pmic-arb.c | 45 +++++++++++++++++++++++++++-----------------
>>>>> Is there a binding update? Can the binding be converted to YAML as well?
>>>> This change doesn't add/update any dtsi properties but just checking if an
>>>> existing property is present to decide if IRQ support is required, so no
>>>> binding change is needed.
>>> The property is now optional in the binding. Please update the binding.
>> Right, thanks for pointing it out. I forgot that part.
>> I will update the binding. How about only update the interrupt properties as
>> optional in this series then I can come up with following patch to convert
>> the binding to YAML format?
> Sure. The benefit of converting it to YAML is that we can use the
> checker to quickly validate the binding vs. having to read the whole
> thing to understand that it's correct. Converting an existing binding to
> YAML shouldn't be that hard.
Thanks, will do that for sure after this series of the changes.
On 10/15/2021 9:15 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-10-13 20:11:40)
>> On 10/14/2021 3:25 AM, Stephen Boyd wrote:
>>> Quoting Fenglin Wu (2021-10-12 22:31:22)
>>>> On 10/13/2021 2:02 AM, Stephen Boyd wrote:
>>>>> Quoting Fenglin Wu (2021-09-16 23:32:58)
>>>>>> From: David Collins <[email protected]>
>>>>>>
>>>>>> Check that the apid for an SPMI interrupt falls between the
>>>>>> min_apid and max_apid that can be handled by the APPS processor
>>>>>> before invoking the per-apid interrupt handler:
>>>>>> periph_interrupt().
>>>>>>
>>>>>> This avoids an access violation in rare cases where the status
>>>>>> bit is set for an interrupt that is not owned by the APPS
>>>>>> processor.
>>>>>>
>>>>>> Signed-off-by: David Collins <[email protected]>
>>>>>> Signed-off-by: Fenglin Wu <[email protected]>
>>>>>> ---
>>>>> Fixes? BTW, a lot of these patches are irqchip specific. It would be
>>>>> good to get review from irqchip maintainers. Maybe we should split the
>>>>> irqchip driver off via the auxiliary bus so that irqchip maintainers can
>>>>> review. Please Cc them on irqchip related patches.
>>>>>
>>>>> IRQCHIP DRIVERS
>>>>> M: Thomas Gleixner <[email protected]>
>>>>> M: Marc Zyngier <[email protected]>
>>>> Sure, copied Thomas and Marc for code review.
>>>> This is a fix to avoid the register access violation in a case that an
>>>> interrupt is fired in a PMIC module which is not owned by APPS
>>>> processor.
>>> Got it.
>>>
>>>>>> drivers/spmi/spmi-pmic-arb.c | 6 ++++++
>>>>>> 1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
>>>>>> index 4d7ad004..c4adc06 100644
>>>>>> --- a/drivers/spmi/spmi-pmic-arb.c
>>>>>> +++ b/drivers/spmi/spmi-pmic-arb.c
>>>>>> @@ -535,6 +535,12 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
>>>>>> id = ffs(status) - 1;
>>>>>> status &= ~BIT(id);
>>>>>> apid = id + i * 32;
>>>>>> + if (apid < pmic_arb->min_apid
>>>>>> + || apid > pmic_arb->max_apid) {
>>>>> The || goes on the line above. What about making a local variable for
>>>>> first and last and then shifting by 5 in the loop?
>>>>>
>>>>> int first = pmic_arb->min_apid;
>>>>> int last = pmic_arb->max_apid;
>>>>>
>>>>> for (i = first >> 5; i <= last >> 5; i++)
>>>>>
>>>>> if (apid < first || apid > last)
>>>> ACK, will update it following this.
>>>>>> + WARN_ONCE(true, "spurious spmi irq received for apid=%d\n",
>>>>>> + apid);
>>>>> Is there any way to recover from this? Or once the mapping is wrong
>>>>> we're going to get interrupts that we don't know what to do with
>>>>> forever?
>>>> This is a rare case that the unexpected interrupt is fired in a module
>>>> not owned by APPS process, so the interrupt itself is not expected hence
>>>> no need to recover from this but just bail out to avoid following register
>>>> access violation.
>>> And then the irq stops coming? It feels like a misconfiguration in the
>>> firmware that we're trying to hide, hence the WARN_ONCE(). Can we
>>> somehow silence irqs that aren't owned by the APPS when this driver
>>> probes so that they can't even happen after probe?
>> Actually this is a rarely happened case that couldn't be reproduced easily
>> and consistently for further debug. I agreed this should be caused by HW
>> misconfiguration or even some unknown HW bug that it would send out SPMI
>> interrupt messages with incorrect APID, but we have never had any chance
>> to find out the root cause. The patch here simply checked the APID and
>> bail out if it's not in the valid range, it won't cause anything bad but
>> improves the SW robustness. After that, the IRQ won't be triggered again
>> because the latched status in PMIC is not cleared. Also, because of the
>> access restriction to the registers corresponding to this APID, there is
>> nothing we can do from APPS processor side to keep it silent.
> This patch seems like a band-aid for an issue that isn't fully
> understood. I suppose it's good that the irq will stay asserted forever
> and then it won't happen again until it gets cleared by some other
> processor in the SoC. Instead of the WARN_ONCE() can we track if any irq
> is handled when the chained irq is raised, and if nothing is handled
> then call handle_bad_irq() on the chained descriptor? Take a look at
> pinctrl-msm.c to see how they handled spurious irqs that aren't actually
> directed at the APPS processor. We should do something similar here.
Sure, I will do it that way.
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 <[email protected]>
>>>>>>>
>>>>>>> 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?