[PATCH 0/2] dmaengine: pl08x: Support the Hi3798MV100 PL080 variant

[PATCH 0/2] dmaengine: pl08x: Support the Hi3798MV100 PL080 variant

[David Yang]

Hi3798MV100 (HiSilicon HiSTB) carries a PL080-class DMAC. The block
reports 0x0a141080 -- the ID reserved for Samsung's PL080S -- while its
register map is plain PL080 and the instance has 4 channels instead of
the 8 the plain PL080 entry claims.

Set the DT node to arm,primecell-periphid = <0x00041080> and
dma-channels = <4>, and apply the two patches to add that support.

Tested on Hi3798MV100 hardware.

David Yang (2):
  dmaengine: pl08x: Release the reset before reading registers
  dmaengine: pl08x: Use the channel count from the device tree

 drivers/dma/amba-pl08x.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

-- 
2.53.0

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

[David Yang]

amba_read_periphid() happens to release the reset. A node that carries
arm,primecell-periphid never reaches it. Release the reset ourselves.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/dma/amba-pl08x.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 543c2eb8cfa8..33ad6c42cd7f 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -75,6 +75,7 @@
 #include <linux/of.h>
 #include <linux/of_dma.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/amba/pl080.h>
@@ -2695,6 +2696,7 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 	struct pl08x_driver_data *pl08x;
 	struct vendor_data *vd = id->data;
 	struct device_node *np = adev->dev.of_node;
+	struct reset_control *rst;
 	u32 tsfr_size;
 	int ret = 0;
 	int i;
@@ -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);
+	if (IS_ERR(rst)) {
+		ret = PTR_ERR(rst);
+		goto out_no_ioremap;
+	}
+	ret = reset_control_deassert(rst);
+	if (ret)
+		goto out_no_ioremap;
+
 	if (vd->ftdmac020) {
 		u32 val;
 
-- 
2.53.0

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

[David Yang]

The Hi3798MV100 DMAC is a plain PL080 except for the channel count: 4
instead of 8. The part ID does not say, and the driver takes the count
from the matched vendor_data entry. dma-common.yaml already defines
dma-channels for a controller node. Honor it.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/dma/amba-pl08x.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 33ad6c42cd7f..9bcb4a3cd5a0 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -2698,6 +2698,7 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 	struct device_node *np = adev->dev.of_node;
 	struct reset_control *rst;
 	u32 tsfr_size;
+	u32 channels;
 	int ret = 0;
 	int i;
 
@@ -2759,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;
+	}
+
 	/* Initialize memcpy engine */
 	dma_cap_set(DMA_MEMCPY, pl08x->memcpy.cap_mask);
 	pl08x->memcpy.dev = &adev->dev;
-- 
2.53.0

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

[sashiko-bot]

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

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

[sashiko-bot]

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