Re: [PATCH 2/2] dmaengine: pl08x: Use the channel count from the device tree

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dmaengine: pl08x: shared vendor_data channel count mutated in pl08x_probe()
- [High] dmaengine: pl08x: exclusive reset get in pl08x_probe() breaks shared resets
- [Medium] dmaengine: pl08x: ioremap() leak on reset error paths in pl08x_probe()
--

commit 48a403ca064a8a26673e3514f98d52758e1a7164
Author: David Yang <mmyangfl@gmail.com>

dmaengine: pl08x: Use the channel count from the device tree

This series deasserts the reset control in pl08x_probe() before reading
hardware registers and updates the PL08x DMA driver to honor the
dma-channels property from the Device Tree node.

> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index 543c2eb8cfa81..9bcb4a3cd5a05 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
[ ... ]
> @@ -2725,6 +2728,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]
Does requesting an exclusive reset control here break platforms where the
PL080 DMA controller shares its reset line with other peripherals?

In drivers/amba/bus.c, amba_read_periphid() uses
of_reset_control_array_get_optional_shared() to deassert shared resets. On
StarFive JH7110 (arch/riscv/boot/dts/starfive/jh7110.dtsi), the PL080 node
sdma (which sets arm,primecell-periphid) shares the JH7110_STGRST_SEC_AHB
reset line with the crypto and rng devices:

    sdma: dma-controller@16008000 {
        compatible = "arm,pl080", "arm,primecell";
        arm,primecell-periphid = <0x00041080>;
        ...
        resets = <&stgcrg JH7110_STGRST_SEC_AHB>;

In drivers/char/hw_random/jh7110-trng.c, starfive_trng_probe() acquires this
same reset line via devm_reset_control_get_shared(). When one driver requests
the reset line as exclusive and another requests it as shared,
__reset_control_get_internal() in drivers/reset/core.c triggers a WARN_ON
and fails with -EBUSY:

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

> +	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
devm_reset_control_get_optional_exclusive() or reset_control_deassert()
fails?

pl08x->base is mapped via ioremap() just above this block, whereas the
cleanup labels at the bottom of pl08x_probe() place iounmap(pl08x->base)
under out_no_platdata:

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

> +
>  	if (vd->ftdmac020) {
>  		u32 val;
>  
> @@ -2747,6 +2760,16 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
>  		vd->dualmaster = !!(val & BIT(9));
>  	}
>  
> +	/* Read the channel number from the node if specified */
> +	if (!of_property_read_u32(np, "dma-channels", &channels) && channels) {
> +		if (channels > vd->channels)
> +			dev_warn(&adev->dev,
> +				 "using %u channels, instead of %u from the node\n",
> +				 vd->channels, channels);
> +		else
> +			vd->channels = channels;
> +	}

[Severity: High]
Since vd = id->data points directly to the shared static struct vendor_data
entry in pl08x_ids[] (such as vendor_pl080), doesn't mutating vd->channels
in-place corrupt the channel count across controller instances or across
driver unbind and re-probe?

In pl08x_probe(), each device stores a pointer to the shared structure:

    struct vendor_data *vd = id->data;
    ...
    pl08x->vd = vd;

At runtime, pl08x_get_phy_channel(), pl08x_irq(), and pl08x_debugfs_show()
all read pl08x->vd->channels dynamically:

    for (i = 0; i < pl08x->vd->channels; i++) {
        if ((BIT(i) & err) || (BIT(i) & tc)) {
            /* Locate physical channel */
            struct pl08x_phy_chan *phychan = &pl08x->phy_chans[i];

If a 4-channel controller probes before an 8-channel controller matching the
same AMBA ID, vendor_pl080.channels is overwritten with 4, clamping the
second controller to 4 channels as well. Conversely, if an 8-channel
controller probes first and a 4-channel controller probes second, shrinking
vd->channels from 8 to 4 causes the already-active first controller to stop
allocating physical channels 4..7 in pl08x_get_phy_channel() and to stop
servicing their interrupts in pl08x_irq() after clearing their bits in
PL080_ERR_CLEAR and PL080_TC_CLEAR.

On ftdmac020, where vd->channels is populated from FTDMAC020_FEATURE on each
probe before checking dma-channels, if a first instance overrides
vd->channels to 4 (allocating 4 elements in pl08x->phy_chans) and a second
instance later probes with 8 channels, pl08x->vd->channels on the first
instance becomes 8 and pl08x_get_phy_channel(), pl08x_irq(), and
pl08x_debugfs_show() can index pl08x->phy_chans[] out of bounds.

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