Re: [PATCH v2 2/3] EDAC/cadence: Add Cadence DDR EDAC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] locking: uninitialized mutex priv->lock causes panic on sysfs write
- [Medium] driver-core: premature sysfs registration creates probe failure race
- [Low] build: FIELD_MODIFY fails build with non-constant mask parameter
- [Low] log: missing newline in edac_mc_chipset_printk format string
--

commit a0e3a033fb0a852091db86b577bfa6524860f48e
Author: Paul Louvel <paul.louvel@bootlin.com>

EDAC/cadence: Add Cadence DDR EDAC driver

This commit adds a new EDAC driver for the Cadence DDR memory controller
found on the Renesas RZ/N1x SoC. It supports single bit error correction,
double bit error detection, and software scrubbing.

> diff --git a/drivers/edac/cadence_edac.c b/drivers/edac/cadence_edac.c
> new file mode 100644
> index 0000000000000..7edac7795fa70
> --- /dev/null
> +++ b/drivers/edac/cadence_edac.c
[ ... ]
> +static void cdns_rmw(struct cdns_mc_priv *priv, u32 reg, u32 mask, u32 val)
> +{
> +	u32 regval;
> +
> +	mutex_lock(&priv->lock);
> +	regval = readl(priv->io_base + reg);
> +	FIELD_MODIFY(mask, &regval, val);

[Severity: Low]
Will this cause a build failure on some configurations? FIELD_MODIFY
internally requires its mask parameter to be a compile-time constant,
enforced via a build-time check. Since cdns_rmw() takes mask as a standard
function parameter and isn't explicitly inlined, the compiler might not be
able to resolve it as a constant.

> +	writel(regval, priv->io_base + reg);
> +	mutex_unlock(&priv->lock);
> +}
[ ... ]
> +static int cdns_mc_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	mci->pdev = &pdev->dev;
> +	priv = mci->pvt_info;

[Severity: High]
Is priv->lock missing a call to mutex_init()? 

The edac_mc_alloc() function allocates mci->pvt_info with kzalloc(), meaning
the mutex is zero-initialized but never properly initialized. Calling
mutex_lock() on this in cdns_rmw() can corrupt the wait list or trigger a
kernel panic, which could be reached if userspace writes to the inject_ctrl
sysfs attribute.

> +
> +	priv->io_base = devm_platform_ioremap_resource(pdev, 0);
[ ... ]
> +	ret = edac_mc_add_mc_with_groups(mci, cdns_edac_groups);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to add mc\n");
> +
> +	ret = devm_add_action_or_reset(&pdev->dev, devm_cdns_edac_del, &pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	writel(CDNS_DDR_ECC_INT_CTRL_MASK_ALL, priv->io_base + CDNS_DDR_ECC_INT_CTRL);
> +
> +	irq = platform_get_irq(pdev, 0);

[Severity: Medium]
Could exposing the sysfs interface before claiming the IRQ create a race
condition?

Calling edac_mc_add_mc_with_groups() registers the controller and exposes
sysfs attributes like inject_ctrl to userspace. If platform_get_irq() or the
subsequent devm_request_irq() defers or fails, the probe unwinds and tears
down the device. A concurrent userspace access during this window might
interact with a partially initialized or destroyed state.

> +	if (irq < 0)
> +		return dev_err_probe(&pdev->dev, irq, "unable to get irq\n");
[ ... ]
> +	edac_mc_chipset_printk(mci, KERN_DEBUG, DRV_NAME, "ECC %s",
> +			       (mci->edac_cap == EDAC_FLAG_NONE) ? "disabled" : "enabled");

[Severity: Low]
Is this format string missing a trailing newline? 

The edac_mc_chipset_printk() macro translates directly to a printk() call
and does not append a newline character automatically. Without it, subsequent
logs might end up appended to the same line in dmesg.

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-paul-v7-3-rc1-edac-v2-0-bc1406161ecc@bootlin.com?part=2