[PATCH 0/4] edac: qcom: add DDR DRAM ECC reporting for Shikra (QCM2390)

[PATCH 0/4] edac: qcom: add DDR DRAM ECC reporting for Shikra (QCM2390)

[Faiyaz Mohammed]

Add DDR DRAM ECC reporting for QCM2390 (Shikra) via the LLCC EDAC
driver. The LCP/SRP block between the LLCC and DDR PHY signals CE/UE
errors on a dedicated interrupt separate from the TRP/DRP cache ECC
path.

Patch 1 updates the DT binding to allow an optional second interrupt,
adds interrupt-names ("llcc-ecc", "llcc-lcp-ecc"), and enforces both
are present for qcom,shikra-llcc.

Patch 2 adds LCP/SRP register offsets to struct llcc_edac_reg_offset,
populates them for QCM2390, and adds lcp_irq to struct llcc_drv_data.

Patch 3 adds qcom_llcc_lcp_setup() to initialise the LCP block and
llcc_lcp_ecc_irq_handler() to report per-bank CE/UE errors via EDAC.

Patch 4 adds the second interrupt entry and interrupt-names to the
Shikra LLCC DT node.

Signed-off-by: Faiyaz Mohammed <faiyaz.mohammed@oss.qualcomm.com>
---
Faiyaz Mohammed (4):
      dt-bindings: cache: qcom,llcc: allow optional second interrupt for LCP/SRP ECC
      soc: qcom: llcc: add LCP/SRP DDR DRAM ECC register offsets
      edac: qcom: add LCP/SRP DDR DRAM ECC support for Shikra
      arm64: dts: qcom: shikra: add LCP/SRP ECC interrupt to LLCC node

 .../devicetree/bindings/cache/qcom,llcc.yaml       |  26 +++-
 arch/arm64/boot/dts/qcom/shikra.dtsi               |   4 +-
 drivers/edac/qcom_edac.c                           | 147 +++++++++++++++++++++
 drivers/soc/qcom/llcc-qcom.c                       |  45 ++++++-
 include/linux/soc/qcom/llcc-qcom.h                 |  13 ++
 5 files changed, 231 insertions(+), 4 deletions(-)
---
base-commit: cbf6c6b201561342d673223d1e59ef771c7a9dbf
change-id: 20260824-master-a6381b7f53ff

Best regards,
--  
Faiyaz Mohammed <faiyaz.mohammed@oss.qualcomm.com>

[PATCH 1/4] dt-bindings: cache: qcom,llcc: allow optional second interrupt for LCP/SRP ECC

[Faiyaz Mohammed]

QCM2390 (Shikra) has an LCP/SRP block that signals DDR DRAM ECC errors
on a dedicated interrupt line separate from the existing TRP/DRP cache
ECC interrupt.

Allow an optional second interrupt in the LLCC binding, add
interrupt-names with "llcc-ecc" and "llcc-lcp-ecc", and enforce both
are present when compatible is qcom,shikra-llcc.

Signed-off-by: Faiyaz Mohammed <faiyaz.mohammed@oss.qualcomm.com>
---
 .../devicetree/bindings/cache/qcom,llcc.yaml       | 26 +++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
index 2141e5d4de63..3546cf7c2519 100644
--- a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
+++ b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
@@ -64,7 +64,17 @@ properties:
     maxItems: 18
 
   interrupts:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: TRP/DRP LLCC cache ECC (ddrss_apps_interrupt[2])
+      - description: LCP/SRP DDR DRAM ECC (ddrss_apps_interrupt[1]), present
+          on platforms with an LCP/SRP block such as QCM2390
+
+  interrupt-names:
+    minItems: 1
+    items:
+      - const: llcc-ecc
+      - const: llcc-lcp-ecc
 
   memory-region:
     maxItems: 1
@@ -234,6 +244,20 @@ allOf:
             - const: llcc1_base
             - const: llcc_broadcast_base
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: qcom,shikra-llcc
+    then:
+      properties:
+        interrupts:
+          minItems: 2
+        interrupt-names:
+          minItems: 2
+      required:
+        - interrupt-names
+
   - if:
       properties:
         compatible:

-- 
2.34.1

[PATCH 2/4] soc: qcom: llcc: add LCP/SRP DDR DRAM ECC register offsets

[Faiyaz Mohammed]

Add LCP/SRP ECC register offsets to struct llcc_edac_reg_offset and
create llcc_lcp_srp_edac_reg_offset for QCM2390, leaving the shared
llcc_v2_1_edac_reg_offset table unchanged. Switch shikra_cfg to the
new table so other platforms with the same block can reuse it.

Add lcp_irq to struct llcc_drv_data and retrieve the optional second
interrupt in qcom_llcc_probe(), propagating non-absence errors so the
driver defers correctly.

Signed-off-by: Faiyaz Mohammed <faiyaz.mohammed@oss.qualcomm.com>
---
 drivers/soc/qcom/llcc-qcom.c       | 45 ++++++++++++++++++++++++++++++++++++--
 include/linux/soc/qcom/llcc-qcom.h | 13 +++++++++++
 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
index d7a42a8040a1..39e0da14b36d 100644
--- a/drivers/soc/qcom/llcc-qcom.c
+++ b/drivers/soc/qcom/llcc-qcom.c
@@ -4509,6 +4509,41 @@ static const struct llcc_edac_reg_offset llcc_v2_1_edac_reg_offset = {
 	.drp_ecc_db_err_syn0 = 0x52120,
 };
 
+static const struct llcc_edac_reg_offset llcc_lcp_srp_edac_reg_offset = {
+	.trp_ecc_error_status0 = 0x20344,
+	.trp_ecc_error_status1 = 0x20348,
+	.trp_ecc_sb_err_syn0 = 0x2034c,
+	.trp_ecc_db_err_syn0 = 0x20370,
+	.trp_ecc_error_cntr_clear = 0x20440,
+	.trp_interrupt_0_status = 0x20480,
+	.trp_interrupt_0_clear = 0x20484,
+	.trp_interrupt_0_enable = 0x20488,
+
+	/* LLCC Common registers */
+	.cmn_status0 = 0x3400c,
+	.cmn_interrupt_0_enable = 0x3401c,
+	.cmn_interrupt_1_enable = 0x3402c,
+	.cmn_interrupt_2_enable = 0x3403c,
+
+	/* LLCC DRP registers */
+	.drp_ecc_error_cfg = 0x50000,
+	.drp_ecc_error_cntr_clear = 0x50004,
+	.drp_interrupt_status = 0x50020,
+	.drp_interrupt_clear = 0x50028,
+	.drp_interrupt_enable = 0x5002c,
+	.drp_ecc_error_status0 = 0x520f4,
+	.drp_ecc_error_status1 = 0x520f8,
+	.drp_ecc_sb_err_syn0 = 0x520fc,
+	.drp_ecc_db_err_syn0 = 0x52120,
+
+	/* LCP/SRP DDR DRAM ECC registers */
+	.lcp_srp_ecc_error_cntr_clear = 0x59004,
+	.lcp_srp_ecc_error_status1 = 0x59010,
+	.lcp_srp_interrupt_status = 0x59070,
+	.lcp_srp_interrupt_clear = 0x59074,
+	.lcp_srp_interrupt_enable = 0x59078,
+};
+
 static const struct llcc_edac_reg_offset llcc_v6_edac_reg_offset = {
 	.trp_ecc_error_status0 = 0x47448,
 	.trp_ecc_error_status1 = 0x47450,
@@ -4767,7 +4802,7 @@ static const struct qcom_llcc_config shikra_cfg[] = {
 		.sct_data	= shikra_data,
 		.size		= ARRAY_SIZE(shikra_data),
 		.reg_offset	= llcc_v2_1_reg_offset,
-		.edac_reg_offset = &llcc_v2_1_edac_reg_offset,
+		.edac_reg_offset = &llcc_lcp_srp_edac_reg_offset,
 		.irq_configured = true,
 	},
 };
@@ -5798,7 +5833,13 @@ static int qcom_llcc_probe(struct platform_device *pdev)
 	if (drv_data->ecc_irq < 0 && drv_data->ecc_irq != -ENXIO)
 		return dev_err_probe(&pdev->dev, drv_data->ecc_irq,
 				     "failed to get IRQ resource\n");
-
+	if (cfg->edac_reg_offset && cfg->edac_reg_offset->lcp_srp_interrupt_enable) {
+		drv_data->lcp_irq = platform_get_irq_optional(pdev, 1);
+		if (drv_data->lcp_irq < 0 && drv_data->lcp_irq != -ENXIO) {
+			ret = drv_data->lcp_irq;
+			goto err;
+		}
+	}
 	drv_data->edac_reg_offset = cfg->edac_reg_offset;
 	drv_data->ecc_irq_configured = cfg->irq_configured;
 	drv_data->dev = dev;
diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h
index 713cb0221b14..020d0b4116d2 100644
--- a/include/linux/soc/qcom/llcc-qcom.h
+++ b/include/linux/soc/qcom/llcc-qcom.h
@@ -139,6 +139,7 @@ struct llcc_edac_reg_offset {
 	/* LLCC Common registers */
 	u32 cmn_status0;
 	u32 cmn_interrupt_0_enable;
+	u32 cmn_interrupt_1_enable;
 	u32 cmn_interrupt_2_enable;
 
 	/* LLCC DRP registers */
@@ -151,6 +152,16 @@ struct llcc_edac_reg_offset {
 	u32 drp_ecc_error_status1;
 	u32 drp_ecc_sb_err_syn0;
 	u32 drp_ecc_db_err_syn0;
+
+	/*
+	 * LCP/SRP DDR DRAM ECC registers (LLCC broadcast space).
+	 * Zero on platforms that do not support LCP/SRP ECC reporting.
+	 */
+	u32 lcp_srp_ecc_error_cntr_clear;
+	u32 lcp_srp_ecc_error_status1;
+	u32 lcp_srp_interrupt_status;
+	u32 lcp_srp_interrupt_clear;
+	u32 lcp_srp_interrupt_enable;
 };
 
 /**
@@ -165,6 +176,7 @@ struct llcc_edac_reg_offset {
  * @cfg_size: size of the config data table
  * @num_banks: Number of LLCC banks
  * @ecc_irq: interrupt for LLCC cache error detection and reporting
+ * @lcp_irq: interrupt for LCP/SRP DDR DRAM ECC error reporting or negative errno on failure
  * @ecc_irq_configured: 'True' if firmware has already configured the irq propagation
  * @version: Indicates the LLCC version
  * @desc: Array pointer of pre-allocated LLCC slice descriptors
@@ -180,6 +192,7 @@ struct llcc_drv_data {
 	u32 cfg_size;
 	u32 num_banks;
 	int ecc_irq;
+	int lcp_irq;
 	bool ecc_irq_configured;
 	u32 version;
 	struct llcc_slice_desc *desc;

-- 
2.34.1

[PATCH 3/4] edac: qcom: add LCP/SRP DDR DRAM ECC support for Shikra

[Faiyaz Mohammed]

Add qcom_llcc_lcp_setup() to enable LCP/SRP interrupt routing when
firmware has not pre-configured it, and llcc_lcp_ecc_irq_handler() to
report per-bank correctable and uncorrectable DDR DRAM ECC errors.

LCP/SRP setup is skipped when ecc_irq_configured is set (e.g. Shikra,
where DSF firmware handles IRQ routing). A missing lcp_irq is
non-fatal; TRP/DRP ECC reporting continues unaffected.

Signed-off-by: Faiyaz Mohammed <faiyaz.mohammed@oss.qualcomm.com>
---
 drivers/edac/qcom_edac.c | 147 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 147 insertions(+)

diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
index f3da9385ca0d..b4f7b2493650 100644
--- a/drivers/edac/qcom_edac.c
+++ b/drivers/edac/qcom_edac.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2018, The Linux Foundation. All rights reserved.
  */
 
+#include <linux/bitfield.h>
 #include <linux/edac.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -44,8 +45,16 @@
 #define SB_DB_TRP_INTERRUPT_ENABLE      0x3
 #define TRP0_INTERRUPT_ENABLE           0x1
 #define DRP0_INTERRUPT_ENABLE           BIT(6)
+#define SRP_INTERRUPT_ENABLE            BIT(7)
 #define SB_DB_DRP_INTERRUPT_ENABLE      0x3
 
+/* LCP/SRP DDR DRAM ECC */
+#define LCP_SB_ECC_ERROR                BIT(0)
+#define LCP_DB_ECC_ERROR                BIT(1)
+#define LCP_SRP_SB_DB_INTERRUPT_ENABLE  0x3
+#define LCP_DB_ERR_COUNT_MASK           GENMASK(4, 0)
+#define LCP_SB_ERR_COUNT_MASK           GENMASK(23, 16)
+
 #define ECC_POLL_MSEC			5000
 
 enum {
@@ -124,6 +133,29 @@ static int qcom_llcc_core_setup(struct llcc_drv_data *drv, struct regmap *llcc_b
 	return ret;
 }
 
+/*
+ * Enable LCP/SRP DDR DRAM ECC interrupt generation and route events to
+ * ddrss_apps_interrupt[1] via cmn_interrupt_1_enable.  Only called when
+ * !ecc_irq_configured; on platforms like Shikra where firmware pre-configures
+ * both TRP/DRP and LCP/SRP routing, the driver skips this and only requests
+ * the IRQ handler.
+ */
+static int qcom_llcc_lcp_setup(struct llcc_drv_data *drv,
+			       struct regmap *llcc_bcast_regmap)
+{
+	int ret;
+
+	ret = regmap_set_bits(llcc_bcast_regmap,
+			      drv->edac_reg_offset->cmn_interrupt_1_enable,
+			      SRP_INTERRUPT_ENABLE);
+	if (ret)
+		return ret;
+
+	return regmap_set_bits(llcc_bcast_regmap,
+			       drv->edac_reg_offset->lcp_srp_interrupt_enable,
+			       LCP_SRP_SB_DB_INTERRUPT_ENABLE);
+}
+
 /* Clear the error interrupt and counter registers */
 static int
 qcom_llcc_clear_error_status(int err_type, struct llcc_drv_data *drv)
@@ -334,6 +366,92 @@ static void llcc_ecc_check(struct edac_device_ctl_info *edev_ctl)
 	llcc_ecc_irq_handler(0, edev_ctl);
 }
 
+/*
+ * Handler for the optional LCP/SRP DDR DRAM ECC interrupt
+ * (ddrss_apps_interrupt[1]).  The LCP/SRP registers live in each LLCC
+ * instance's address space, so iterate over all banks and read status
+ * per-instance.  Broadcast-write the clear after the full loop, mirroring
+ * the TRP/DRP handler pattern.
+ */
+static irqreturn_t llcc_lcp_ecc_irq_handler(int irq, void *edev_ctl)
+{
+	struct edac_device_ctl_info *edac_dev_ctl = edev_ctl;
+	struct llcc_drv_data *drv = edac_dev_ctl->dev->platform_data;
+	const struct llcc_edac_reg_offset *reg = drv->edac_reg_offset;
+	irqreturn_t irq_rc = IRQ_NONE;
+	u32 intr_status, err_status, clr_reg;
+	int i, ret;
+
+	/* Iterate over banks: LCP/SRP status registers are per-LLCC instance */
+	for (i = 0; i < drv->num_banks; i++) {
+		ret = regmap_read(drv->regmaps[i], reg->lcp_srp_interrupt_status,
+				  &intr_status);
+		if (ret)
+			continue;
+
+		intr_status &= LCP_SB_ECC_ERROR | LCP_DB_ECC_ERROR;
+		if (!intr_status)
+			continue;
+
+		irq_rc = IRQ_HANDLED;
+
+		ret = regmap_read(drv->regmaps[i], reg->lcp_srp_ecc_error_status1,
+				  &err_status);
+		if (ret) {
+			edac_printk(KERN_ERR, EDAC_LLCC,
+				    "LCP/SRP error status read failed for bank%u: %d\n",
+				    i, ret);
+			continue;
+		}
+
+		if (intr_status & LCP_DB_ECC_ERROR) {
+			u32 db_cnt = FIELD_GET(LCP_DB_ERR_COUNT_MASK, err_status);
+
+			edac_printk(KERN_CRIT, EDAC_LLCC,
+				    "LCP/SRP DDR DRAM Double-bit ECC error in bank%u: count=%u status=0x%08x\n",
+				    i, db_cnt, err_status);
+			edac_device_handle_ue(edac_dev_ctl, 0, i,
+					      "LCP/SRP DDR DRAM uncorrectable error");
+		}
+
+		if (intr_status & LCP_SB_ECC_ERROR) {
+			u32 sb_cnt = FIELD_GET(LCP_SB_ERR_COUNT_MASK, err_status);
+
+			edac_printk(KERN_WARNING, EDAC_LLCC,
+				    "LCP/SRP DDR DRAM Single-bit ECC error in bank%u: count=%u status=0x%08x\n",
+				    i, sb_cnt, err_status);
+			edac_device_handle_ce(edac_dev_ctl, 0, i,
+					      "LCP/SRP DDR DRAM correctable error");
+		}
+	}
+
+	if (irq_rc != IRQ_HANDLED)
+		return irq_rc;
+
+	clr_reg = reg->lcp_srp_interrupt_clear ?
+			reg->lcp_srp_interrupt_clear :
+			reg->lcp_srp_interrupt_status;
+
+	if (clr_reg) {
+		ret = regmap_write(drv->bcast_regmap, clr_reg,
+				   LCP_SB_ECC_ERROR | LCP_DB_ECC_ERROR);
+		if (ret)
+			edac_printk(KERN_ERR, EDAC_LLCC,
+				    "LCP/SRP interrupt clear failed: %d\n", ret);
+	}
+
+	if (reg->lcp_srp_ecc_error_cntr_clear) {
+		ret = regmap_write(drv->bcast_regmap,
+				   reg->lcp_srp_ecc_error_cntr_clear,
+				   DRP_TRP_CNT_CLEAR);
+		if (ret)
+			edac_printk(KERN_ERR, EDAC_LLCC,
+				    "LCP/SRP counter clear failed: %d\n", ret);
+	}
+
+	return irq_rc;
+}
+
 static int qcom_llcc_edac_probe(struct platform_device *pdev)
 {
 	struct llcc_drv_data *llcc_driv_data = pdev->dev.platform_data;
@@ -372,6 +490,8 @@ static int qcom_llcc_edac_probe(struct platform_device *pdev)
 			edac_op_state = EDAC_OPSTATE_INT;
 			goto irq_done;
 		}
+		dev_warn(dev, "failed to request LLCC ECC irq %d: %d, falling back to polling\n",
+			 ecc_irq, rc);
 	}
 
 	/* Fall back to polling mode otherwise */
@@ -386,6 +506,33 @@ static int qcom_llcc_edac_probe(struct platform_device *pdev)
 		return rc;
 	}
 
+	/*
+	 * Set up LCP/SRP DDR DRAM ECC after the EDAC device is published.
+	 * Skip hardware routing setup if firmware already configured IRQ
+	 * propagation (irq_configured); only request the IRQ handler.
+	 * Guard on a non-zero lcp_srp_interrupt_enable offset to confirm
+	 * this LLCC variant actually has an LCP/SRP block.
+	 */
+	if (llcc_driv_data->lcp_irq > 0 &&
+	    llcc_driv_data->edac_reg_offset->lcp_srp_interrupt_enable) {
+		if (!llcc_driv_data->ecc_irq_configured) {
+			rc = qcom_llcc_lcp_setup(llcc_driv_data,
+						 llcc_driv_data->bcast_regmap);
+			if (rc) {
+				dev_warn(dev, "LCP/SRP interrupt setup failed: %d\n", rc);
+				goto lcp_done;
+			}
+		}
+		rc = devm_request_irq(dev, llcc_driv_data->lcp_irq,
+				      llcc_lcp_ecc_irq_handler,
+				      0, "llcc_lcp_ecc", edev_ctl);
+		if (rc)
+			dev_warn(dev,
+				 "failed to request LCP ECC irq %d: %d, DDR DRAM ECC reporting disabled\n",
+				 llcc_driv_data->lcp_irq, rc);
+	}
+lcp_done:
+	rc = 0;
 	platform_set_drvdata(pdev, edev_ctl);
 
 	return rc;

-- 
2.34.1

[PATCH 4/4] arm64: dts: qcom: shikra: add LCP/SRP ECC interrupt to LLCC node

[Faiyaz Mohammed]

Add a second interrupt for LCP/SRP DDR DRAM ECC reporting and
interrupt-names ("llcc-ecc", "llcc-lcp-ecc") to the Shikra LLCC node,
as required by the updated DT binding.

Signed-off-by: Faiyaz Mohammed <faiyaz.mohammed@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/shikra.dtsi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/shikra.dtsi b/arch/arm64/boot/dts/qcom/shikra.dtsi
index 2623e139196f..ff572c003b39 100644
--- a/arch/arm64/boot/dts/qcom/shikra.dtsi
+++ b/arch/arm64/boot/dts/qcom/shikra.dtsi
@@ -1108,7 +1108,9 @@ llcc: system-cache-controller@e00000 {
 			reg-names = "llcc0_base",
 				    "llcc1_base",
 				    "llcc_broadcast_base";
-			interrupts = <GIC_SPI 539 IRQ_TYPE_LEVEL_HIGH 0>;
+			interrupts = <GIC_SPI 539 IRQ_TYPE_LEVEL_HIGH 0>,
+				     <GIC_SPI 538 IRQ_TYPE_LEVEL_HIGH 0>;
+			interrupt-names = "llcc-ecc", "llcc-lcp-ecc";
 		};
 
 		gcc: clock-controller@1400000 {

-- 
2.34.1

Re: [PATCH 1/4] dt-bindings: cache: qcom,llcc: allow optional second interrupt for LCP/SRP ECC

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dt-bindings: missing maxItems constraint for legacy interrupts
--

commit 50febfa0c04f4bae15f676db2e6662c8855b0e3c
Author: Faiyaz Mohammed <faiyaz.mohammed@oss.qualcomm.com>

dt-bindings: cache: qcom,llcc: allow optional second interrupt for LCP/SRP ECC

QCM2390 (Shikra) has an LCP/SRP block that signals DDR DRAM ECC errors
on a dedicated interrupt line separate from the existing TRP/DRP cache
ECC interrupt. This commit allows an optional second interrupt in the
LLCC binding and enforces both are present for qcom,shikra-llcc.

> diff --git a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
> index 2141e5d4de63b..3546cf7c25195 100644
> --- a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
> +++ b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
> @@ -64,7 +64,17 @@ properties:
>      maxItems: 18
>  
>    interrupts:
> -    maxItems: 1
> +    minItems: 1
> +    items:
> +      - description: TRP/DRP LLCC cache ECC (ddrss_apps_interrupt[2])
> +      - description: LCP/SRP DDR DRAM ECC (ddrss_apps_interrupt[1]), present
> +          on platforms with an LCP/SRP block such as QCM2390

[Severity: Low]
Does this implicitly raise the maximum number of interrupts to 2 for all
compatible strings?

Since maxItems: 1 was removed globally, a device tree for older hardware 
could erroneously define two interrupts without failing schema validation.

> +
> +  interrupt-names:
> +    minItems: 1
> +    items:
> +      - const: llcc-ecc
> +      - const: llcc-lcp-ecc
>  
>    memory-region:
>      maxItems: 1

[ ... ]

> @@ -234,6 +244,20 @@ allOf:
>              - const: llcc1_base
>              - const: llcc_broadcast_base
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: qcom,shikra-llcc
> +    then:
> +      properties:
> +        interrupts:
> +          minItems: 2
> +        interrupt-names:
> +          minItems: 2
> +      required:
> +        - interrupt-names
> +

[Severity: Low]
Should there be an else clause here to restrict legacy platforms back to
maxItems: 1 for interrupts and interrupt-names?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-master-v1-0-aba3cfcfbac9@oss.qualcomm.com?part=1

Re: [PATCH 4/4] arm64: dts: qcom: shikra: add LCP/SRP ECC interrupt to LLCC node

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] edac: qcom: UAF in IRQ handlers due to asymmetrical devm vs manual teardown
--

commit fab541f0482e82717fee45392a6aa89253bbeef9
Author: Faiyaz Mohammed <faiyaz.mohammed@oss.qualcomm.com>

arm64: dts: qcom: shikra: add LCP/SRP ECC interrupt to LLCC node

Add a second interrupt for LCP/SRP DDR DRAM ECC reporting and
interrupt-names ("llcc-ecc", "llcc-lcp-ecc") to the Shikra LLCC node,
as required by the updated DT binding.

> diff --git a/arch/arm64/boot/dts/qcom/shikra.dtsi b/arch/arm64/boot/dts/qcom/shikra.dtsi
> index 2623e139196f6..ff572c003b396 100644
> --- a/arch/arm64/boot/dts/qcom/shikra.dtsi
> +++ b/arch/arm64/boot/dts/qcom/shikra.dtsi
> @@ -1108,7 +1108,9 @@ llcc: system-cache-controller@e00000 {
>  			reg-names = "llcc0_base",
>  				    "llcc1_base",
>  				    "llcc_broadcast_base";
> -			interrupts = <GIC_SPI 539 IRQ_TYPE_LEVEL_HIGH 0>;
> +			interrupts = <GIC_SPI 539 IRQ_TYPE_LEVEL_HIGH 0>,
> +				     <GIC_SPI 538 IRQ_TYPE_LEVEL_HIGH 0>;
> +			interrupt-names = "llcc-ecc", "llcc-lcp-ecc";

[Severity: High]
Does adding this new interrupt expose a use-after-free race condition in the
EDAC driver?

In drivers/edac/qcom_edac.c, devm-managed IRQs remain active until the devres
phase, but edac_device_free_ctl_info() manually frees the payload object:

drivers/edac/qcom_edac.c:qcom_llcc_edac_remove() {
	...
	edac_device_free_ctl_info(edev_ctl);
	...
}

If the newly enabled lcp_irq or the ecc_irq fires during module removal
(after qcom_llcc_edac_remove() frees edev_ctl but before devres deregisters
the IRQs), can the interrupt handler dereference the freed edev_ctl?

drivers/edac/qcom_edac.c:llcc_lcp_ecc_irq_handler() {
	...
	struct llcc_drv_data *drv = edac_dev_ctl->dev->platform_data;
	...
}

This could lead to invalid memory accesses, unpredictable behavior,
or a kernel panic.

The same race seems to exist on the probe error path if
edac_device_add_device() fails, leaving the IRQ active until the function
returns:

drivers/edac/qcom_edac.c:qcom_llcc_edac_probe() {
	...
	rc = edac_device_add_device(edev_ctl);
	if (rc) {
		edac_device_free_ctl_info(edev_ctl);
		return rc;
	}
	...
}

>  		};
>  
>  		gcc: clock-controller@1400000 {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-master-v1-0-aba3cfcfbac9@oss.qualcomm.com?part=4

Re: [PATCH 3/4] edac: qcom: add LCP/SRP DDR DRAM ECC support for Shikra

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] edac: qcom: UAF in llcc_lcp_ecc_irq_handler due to devm_request_irq lifecycle mismatch
- [Medium] edac: LCP/SRP handlers discard hardware ECC counts, causing under-reporting
- [Medium] edac: qcom: data race in EDAC counters due to concurrent IRQs
--

commit 943c843f6548de65b2dfbf919c2a617470e1b5e9
Author: Faiyaz Mohammed <faiyaz.mohammed@oss.qualcomm.com>

edac: qcom: add LCP/SRP DDR DRAM ECC support for Shikra

Add qcom_llcc_lcp_setup() to enable LCP/SRP interrupt routing when
firmware has not pre-configured it, and llcc_lcp_ecc_irq_handler() to
report per-bank correctable and uncorrectable DDR DRAM ECC errors.

> diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
> index f3da9385ca0d8..b4f7b2493650c 100644
> --- a/drivers/edac/qcom_edac.c
> +++ b/drivers/edac/qcom_edac.c

[ ... ]

> @@ -334,6 +366,92 @@ static void llcc_ecc_check(struct edac_device_ctl_info *edev_ctl)
>  	llcc_ecc_irq_handler(0, edev_ctl);
>  }
>  
> +/*
> + * Handler for the optional LCP/SRP DDR DRAM ECC interrupt
> + * (ddrss_apps_interrupt[1]).  The LCP/SRP registers live in each LLCC
> + * instance's address space, so iterate over all banks and read status
> + * per-instance.  Broadcast-write the clear after the full loop, mirroring
> + * the TRP/DRP handler pattern.
> + */
> +static irqreturn_t llcc_lcp_ecc_irq_handler(int irq, void *edev_ctl)
> +{
> +	struct edac_device_ctl_info *edac_dev_ctl = edev_ctl;
> +	struct llcc_drv_data *drv = edac_dev_ctl->dev->platform_data;
> +	const struct llcc_edac_reg_offset *reg = drv->edac_reg_offset;
> +	irqreturn_t irq_rc = IRQ_NONE;
> +	u32 intr_status, err_status, clr_reg;
> +	int i, ret;
> +
> +	/* Iterate over banks: LCP/SRP status registers are per-LLCC instance */
> +	for (i = 0; i < drv->num_banks; i++) {
> +		ret = regmap_read(drv->regmaps[i], reg->lcp_srp_interrupt_status,
> +				  &intr_status);
> +		if (ret)
> +			continue;
> +
> +		intr_status &= LCP_SB_ECC_ERROR | LCP_DB_ECC_ERROR;
> +		if (!intr_status)
> +			continue;
> +
> +		irq_rc = IRQ_HANDLED;
> +
> +		ret = regmap_read(drv->regmaps[i], reg->lcp_srp_ecc_error_status1,
> +				  &err_status);
> +		if (ret) {
> +			edac_printk(KERN_ERR, EDAC_LLCC,
> +				    "LCP/SRP error status read failed for bank%u: %d\n",
> +				    i, ret);
> +			continue;
> +		}
> +
> +		if (intr_status & LCP_DB_ECC_ERROR) {
> +			u32 db_cnt = FIELD_GET(LCP_DB_ERR_COUNT_MASK, err_status);
> +
> +			edac_printk(KERN_CRIT, EDAC_LLCC,
> +				    "LCP/SRP DDR DRAM Double-bit ECC error in bank%u: count=%u status=0x%08x\n",
> +				    i, db_cnt, err_status);
> +			edac_device_handle_ue(edac_dev_ctl, 0, i,
> +					      "LCP/SRP DDR DRAM uncorrectable error");

[Severity: Medium]
Since db_cnt contains the exact number of double-bit errors, should this
code use edac_device_handle_ue_count(edac_dev_ctl, db_cnt, ...) instead?

The edac_device_handle_ue() wrapper hardcodes an error count of 1, which will
discard the hardware-provided count and lead to under-reporting if the
aggregated count is greater than 1.

[Severity: Medium]
Additionally, since edac_device_handle_ue() performs non-atomic increments on
the EDAC core counters, is a lock needed here?

The ecc_irq and lcp_irq are independent hardware interrupts. If their handlers
(llcc_ecc_irq_handler and llcc_lcp_ecc_irq_handler) run concurrently on
different CPUs, it could result in a data race on the shared edev_ctl
statistics.

> +		}
> +
> +		if (intr_status & LCP_SB_ECC_ERROR) {
> +			u32 sb_cnt = FIELD_GET(LCP_SB_ERR_COUNT_MASK, err_status);
> +
> +			edac_printk(KERN_WARNING, EDAC_LLCC,
> +				    "LCP/SRP DDR DRAM Single-bit ECC error in bank%u: count=%u status=0x%08x\n",
> +				    i, sb_cnt, err_status);
> +			edac_device_handle_ce(edac_dev_ctl, 0, i,
> +					      "LCP/SRP DDR DRAM correctable error");

[Severity: Medium]
Similarly, should this use edac_device_handle_ce_count(edac_dev_ctl, sb_cnt, ...)
to propagate the actual hardware count instead of defaulting to 1?

> +		}
> +	}

[ ... ]

> @@ -386,6 +506,33 @@ static int qcom_llcc_edac_probe(struct platform_device *pdev)
>  		return rc;
>  	}
>  
> +	/*
> +	 * Set up LCP/SRP DDR DRAM ECC after the EDAC device is published.
> +	 * Skip hardware routing setup if firmware already configured IRQ
> +	 * propagation (irq_configured); only request the IRQ handler.
> +	 * Guard on a non-zero lcp_srp_interrupt_enable offset to confirm
> +	 * this LLCC variant actually has an LCP/SRP block.
> +	 */
> +	if (llcc_driv_data->lcp_irq > 0 &&
> +	    llcc_driv_data->edac_reg_offset->lcp_srp_interrupt_enable) {
> +		if (!llcc_driv_data->ecc_irq_configured) {
> +			rc = qcom_llcc_lcp_setup(llcc_driv_data,
> +						 llcc_driv_data->bcast_regmap);
> +			if (rc) {
> +				dev_warn(dev, "LCP/SRP interrupt setup failed: %d\n", rc);
> +				goto lcp_done;
> +			}
> +		}
> +		rc = devm_request_irq(dev, llcc_driv_data->lcp_irq,
> +				      llcc_lcp_ecc_irq_handler,
> +				      0, "llcc_lcp_ecc", edev_ctl);

[Severity: High]
Does the use of devm_request_irq() here create a use-after-free risk during
device removal?

If the device is unbound, the remove callback (qcom_llcc_edac_remove) explicitly
frees the edev_ctl structure via edac_device_free_ctl_info(). However, because
the IRQ is registered with devm_request_irq(), free_irq() isn't called until
devres teardown, which happens after the remove callback completes.

If the LCP ECC interrupt fires in this window, llcc_lcp_ecc_irq_handler() will
dereference a freed edev_ctl pointer.

> +		if (rc)
> +			dev_warn(dev,
> +				 "failed to request LCP ECC irq %d: %d, DDR DRAM ECC reporting disabled\n",
> +				 llcc_driv_data->lcp_irq, rc);
> +	}
> +lcp_done:
> +	rc = 0;
>  	platform_set_drvdata(pdev, edev_ctl);
>  
>  	return rc;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-master-v1-0-aba3cfcfbac9@oss.qualcomm.com?part=3