Changes in v6:
Rebased [v5 08/10] on
https://lore.kernel.org/linux-arm-msm/[email protected]/#t
Changes in v5:
Drop [v4 11/11] because of a similar change is under review:
https://lore.kernel.org/linux-arm-msm/[email protected]/T/#t
Changes in v4:
In [v4 02/11], separated spurious interrupt handling.
In [v4 03/11], added Fixes tag for ("spmi: pmic-arb: do not ack and clear peripheral").
In [v4 11/11], updated the binding to address few warnings in "make dtbs_check"
Changes in v3:
Drop [v2 07/10] as this is no longer needed after this change:
50fc4c8cd240 ("spmi: spmi-pmic-arb: fix irq_set_type race condition")
In [v3 07/10], updated the author email to match with Signed-off-by.
In [v3 10/10], added the binding change in this series, and addressed issues in "make dt_binding_check"
Changes in v2:
In [v2 01/10], added code to handle spurious interrupt.
In [v2 03/10], adressed minor comments to update the code logic.
In [v2 04/10], minor update to detect spurious interrupt.
In [v2 05/10], added Fixes tag.
In [v2 07/10], added Fixes tag and updated commit text to explain the problem.
In [v2 08/10], added binding change to make interrupt properties as optional.
In [v2 09/10], updated to check presence of "interrupt-controller" property.
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 (6):
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
dt-bindings: spmi: spmi-pmic-arb: make interrupt properties as
optional
spmi: pmic-arb: make interrupt support optional
spmi: pmic-arb: increase SPMI transaction timeout delay
Fenglin Wu (1):
spmi: pmic-arb: handle spurious interrupt
Subbaraman Narayanamurthy (1):
spmi: pmic-arb: do not ack and clear peripheral interrupts in
cleanup_irq
.../bindings/spmi/qcom,spmi-pmic-arb.yaml | 3 -
drivers/spmi/spmi-pmic-arb.c | 136 +++++++++++++++------
2 files changed, 96 insertions(+), 43 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 cf92abc..39f25bc 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -1133,6 +1133,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: 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 2113be4..5a99723 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -590,6 +590,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
Call handle_bad_irq() when the summary interrupt is fired spuriously.
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 5a99723..719bd73 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -605,10 +605,11 @@ 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;
+ int handled = 0;
u8 sid = (pmic_arb->apid_data[apid].ppid >> 8) & 0xF;
u8 per = pmic_arb->apid_data[apid].ppid & 0xFF;
@@ -623,7 +624,10 @@ static void periph_interrupt(struct spmi_pmic_arb *pmic_arb, u16 apid)
continue;
}
generic_handle_irq(irq);
+ handled++;
}
+
+ return handled;
}
static void pmic_arb_chained_irq(struct irq_desc *desc)
@@ -634,7 +638,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);
@@ -649,10 +653,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);
}
--
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 | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 2bc3b88..e19eaec 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -625,21 +625,26 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
struct spmi_pmic_arb *pmic_arb = irq_desc_get_handler_data(desc);
const struct pmic_arb_ver_ops *ver_ops = pmic_arb->ver_ops;
struct irq_chip *chip = irq_desc_get_chip(desc);
- int first = pmic_arb->min_apid >> 5;
- int last = pmic_arb->max_apid >> 5;
+ int first = pmic_arb->min_apid;
+ int last = pmic_arb->max_apid;
u8 ee = pmic_arb->ee;
u32 status, enable, handled = 0;
int i, id, apid;
chained_irq_enter(chip, desc);
- for (i = first; i <= last; ++i) {
+ for (i = first >> 5; i <= last >> 5; ++i) {
status = readl_relaxed(
ver_ops->owner_acc_status(pmic_arb, ee, i));
while (status) {
id = ffs(status) - 1;
status &= ~BIT(id);
apid = id + i * 32;
+ if (apid < first || apid > last) {
+ 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]>
Correct the way that duplicate PPID mappings are handled for PMIC
arbiter v5. The final APID mapped to a given PPID should be the
one which has write owner = APPS EE, if it exists, or if not
that, then the first APID mapped to the PPID, if it exists.
Fixes: 40f318f0ed67 ("spmi: pmic-arb: add support for HW version 5")
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 56f2294..cf92abc 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -1031,7 +1031,8 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
* version 5, there is more than one APID mapped to each PPID.
* The owner field for each of these mappings specifies the EE which is
* allowed to write to the APID. The owner of the last (highest) APID
- * for a given PPID will receive interrupts from the PPID.
+ * which has the IRQ owner bit set for a given PPID will receive
+ * interrupts from the PPID.
*/
for (i = 0; ; i++, apidd++) {
offset = pmic_arb->ver_ops->apid_map_offset(i);
@@ -1054,16 +1055,16 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
apid = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;
prev_apidd = &pmic_arb->apid_data[apid];
- if (valid && is_irq_ee &&
- prev_apidd->write_ee == pmic_arb->ee) {
+ if (!valid || apidd->write_ee == pmic_arb->ee) {
+ /* First PPID mapping or one for this EE */
+ pmic_arb->ppid_to_apid[ppid] = i | PMIC_ARB_APID_VALID;
+ } else if (valid && is_irq_ee &&
+ prev_apidd->write_ee == pmic_arb->ee) {
/*
* Duplicate PPID mapping after the one for this EE;
* override the irq owner
*/
prev_apidd->irq_ee = apidd->irq_ee;
- } else if (!valid || is_irq_ee) {
- /* First PPID mapping or duplicate for another EE */
- pmic_arb->ppid_to_apid[ppid] = i | PMIC_ARB_APID_VALID;
}
apidd->ppid = ppid;
--
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 39f25bc..0496e5d 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -1386,10 +1386,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-controller", 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);
@@ -1449,17 +1451,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;
@@ -1467,8 +1474,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;
@@ -1479,8 +1488,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
hi Steven,
Can you help to review the series of the changes?
Thanks
Fenglin Wu
On 2022/4/28 9:12, Fenglin Wu wrote:
> Changes in v6:
> Rebased [v5 08/10] on
> https://lore.kernel.org/linux-arm-msm/[email protected]/#t
>
> Changes in v5:
> Drop [v4 11/11] because of a similar change is under review:
> https://lore.kernel.org/linux-arm-msm/[email protected]/T/#t
>
> Changes in v4:
> In [v4 02/11], separated spurious interrupt handling.
> In [v4 03/11], added Fixes tag for ("spmi: pmic-arb: do not ack and clear peripheral").
> In [v4 11/11], updated the binding to address few warnings in "make dtbs_check"
>
> Changes in v3:
> Drop [v2 07/10] as this is no longer needed after this change:
> 50fc4c8cd240 ("spmi: spmi-pmic-arb: fix irq_set_type race condition")
> In [v3 07/10], updated the author email to match with Signed-off-by.
> In [v3 10/10], added the binding change in this series, and addressed issues in "make dt_binding_check"
>
> Changes in v2:
> In [v2 01/10], added code to handle spurious interrupt.
> In [v2 03/10], adressed minor comments to update the code logic.
> In [v2 04/10], minor update to detect spurious interrupt.
> In [v2 05/10], added Fixes tag.
> In [v2 07/10], added Fixes tag and updated commit text to explain the problem.
> In [v2 08/10], added binding change to make interrupt properties as optional.
> In [v2 09/10], updated to check presence of "interrupt-controller" property.
>
>
> 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 (6):
> 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
> dt-bindings: spmi: spmi-pmic-arb: make interrupt properties as
> optional
> spmi: pmic-arb: make interrupt support optional
> spmi: pmic-arb: increase SPMI transaction timeout delay
>
> Fenglin Wu (1):
> spmi: pmic-arb: handle spurious interrupt
>
> Subbaraman Narayanamurthy (1):
> spmi: pmic-arb: do not ack and clear peripheral interrupts in
> cleanup_irq
>
> .../bindings/spmi/qcom,spmi-pmic-arb.yaml | 3 -
> drivers/spmi/spmi-pmic-arb.c | 136 +++++++++++++++------
> 2 files changed, 96 insertions(+), 43 deletions(-)
>
On 4/27/22 6:12 PM, Fenglin Wu wrote:
> Changes in v6:
> Rebased [v5 08/10] on
> https://lore.kernel.org/linux-arm-msm/[email protected]/#t
>
> Changes in v5:
> Drop [v4 11/11] because of a similar change is under review:
> https://lore.kernel.org/linux-arm-msm/[email protected]/T/#t
>
> Changes in v4:
> In [v4 02/11], separated spurious interrupt handling.
> In [v4 03/11], added Fixes tag for ("spmi: pmic-arb: do not ack and clear peripheral").
> In [v4 11/11], updated the binding to address few warnings in "make dtbs_check"
>
> Changes in v3:
> Drop [v2 07/10] as this is no longer needed after this change:
> 50fc4c8cd240 ("spmi: spmi-pmic-arb: fix irq_set_type race condition")
> In [v3 07/10], updated the author email to match with Signed-off-by.
> In [v3 10/10], added the binding change in this series, and addressed issues in "make dt_binding_check"
>
> Changes in v2:
> In [v2 01/10], added code to handle spurious interrupt.
> In [v2 03/10], adressed minor comments to update the code logic.
> In [v2 04/10], minor update to detect spurious interrupt.
> In [v2 05/10], added Fixes tag.
> In [v2 07/10], added Fixes tag and updated commit text to explain the problem.
> In [v2 08/10], added binding change to make interrupt properties as optional.
> In [v2 09/10], updated to check presence of "interrupt-controller" property.
>
>
> 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 (6):
> 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
> dt-bindings: spmi: spmi-pmic-arb: make interrupt properties as
> optional
> spmi: pmic-arb: make interrupt support optional
> spmi: pmic-arb: increase SPMI transaction timeout delay
>
> Fenglin Wu (1):
> spmi: pmic-arb: handle spurious interrupt
>
> Subbaraman Narayanamurthy (1):
> spmi: pmic-arb: do not ack and clear peripheral interrupts in
> cleanup_irq
>
> .../bindings/spmi/qcom,spmi-pmic-arb.yaml | 3 -
> drivers/spmi/spmi-pmic-arb.c | 136 +++++++++++++++------
> 2 files changed, 96 insertions(+), 43 deletions(-)
>
Hi Stephen,
Is there any problem with the patch series format? If not, can you please review these changes as they've been pending for a while?
Thanks,
Subbaraman