Re: [PATCH 1/2] dmaengine: pl08x: Release the reset before reading registers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dmaengine: pl08x: exclusive reset breaks shared resets in pl08x_probe()
- [Medium] dmaengine: pl08x: MMIO mapping leak on reset error in pl08x_probe()
--

commit 5b724fceef572df4410288b36fa49f5ba64ab7da
Author: David Yang <mmyangfl@gmail.com>

dmaengine: pl08x: Release the reset before reading registers

This commit acquires an optional exclusive reset control and deasserts it
in pl08x_probe() before accessing device registers, covering devices whose
device tree nodes specify arm,primecell-periphid and therefore bypass
amba_read_periphid().

> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index 543c2eb8cfa81..33ad6c42cd7fb 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c

[ ... ]

> @@ -2725,6 +2727,16 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
>  		goto out_no_ioremap;
>  	}
>  
> +	/* Release the reset before reading registers */
> +	rst = devm_reset_control_get_optional_exclusive(&adev->dev, NULL);

[Severity: High]
Could requesting an exclusive reset control here break platforms where the
PL08x reset line is shared with other peripherals?

In drivers/amba/bus.c, amba_read_periphid() acquires a shared reset control:

	rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);

On StarFive JH7110 (arch/riscv/boot/dts/starfive/jh7110.dtsi), the PL080 node
sdma (dma-controller@16008000) carries arm,primecell-periphid and shares the
JH7110_STGRST_SEC_AHB reset with crypto@16000000 and rng@1600c000. Both
starfive_cryp_probe() in drivers/crypto/starfive/jh7110-cryp.c and
starfive_trng_probe() in drivers/char/hw_random/jh7110-trng.c request that
reset line via devm_reset_control_get_shared().

When pl08x_probe() calls devm_reset_control_get_optional_exclusive(),
__reset_control_get_internal() in drivers/reset/core.c rejects mixing shared
and exclusive controls on the same reset line:

	if (WARN_ON(!rstc->shared || !shared))
		return ERR_PTR(-EBUSY);

Additionally, manually calling reset_control_deassert() without a devm action
leaves the reset deasserted if pl08x_probe() fails later or when the driver
is unbound. Would devm_reset_control_get_optional_shared_deasserted() avoid
both issues?

> +	if (IS_ERR(rst)) {
> +		ret = PTR_ERR(rst);
> +		goto out_no_ioremap;
> +	}
> +	ret = reset_control_deassert(rst);
> +	if (ret)
> +		goto out_no_ioremap;

[Severity: Medium]
Does jumping to out_no_ioremap here leak the pl08x->base MMIO mapping when
getting or deasserting the reset fails (for example, with -EPROBE_DEFER)?

In pl08x_probe(), pl08x->base has already been mapped via ioremap() right
before this block, while the error cleanup labels at the bottom of
pl08x_probe() are ordered as:

out_no_platdata:
	iounmap(pl08x->base);
out_no_ioremap:
	kfree(pl08x);

Should these error paths jump to out_no_platdata instead of out_no_ioremap so
that pl08x->base is unmapped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104154542.3131916-1-mmyangfl@gmail.com?part=1