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

[sashiko-bot@kernel.org]

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