[PATCHv2] dmaengine: st_fdma: simplify allocation

[PATCHv2] dmaengine: st_fdma: simplify allocation

[Rosen Penev]

Use a flexible array member to combine kzalloc and kcalloc to a single
allocation.

Add __counted_by for extra runtime analysis. Assign counting variable
after allocation before any array accesses.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: update description.
 drivers/dma/st_fdma.c | 27 ++++++++-------------------
 drivers/dma/st_fdma.h |  4 ++--
 2 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/dma/st_fdma.c b/drivers/dma/st_fdma.c
index d9547017f3bd..3ec0d6731b8d 100644
--- a/drivers/dma/st_fdma.c
+++ b/drivers/dma/st_fdma.c
@@ -710,16 +710,6 @@ static const struct of_device_id st_fdma_match[] = {
 };
 MODULE_DEVICE_TABLE(of, st_fdma_match);

-static int st_fdma_parse_dt(struct platform_device *pdev,
-			const struct st_fdma_driverdata *drvdata,
-			struct st_fdma_dev *fdev)
-{
-	snprintf(fdev->fw_name, FW_NAME_SIZE, "fdma_%s_%d.elf",
-		drvdata->name, drvdata->id);
-
-	return of_property_read_u32(pdev->dev.of_node, "dma-channels",
-				    &fdev->nr_channels);
-}
 #define FDMA_DMA_BUSWIDTHS	(BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | \
 				 BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | \
 				 BIT(DMA_SLAVE_BUSWIDTH_3_BYTES) | \
@@ -742,27 +732,26 @@ static int st_fdma_probe(struct platform_device *pdev)
 	struct st_fdma_dev *fdev;
 	struct device_node *np = pdev->dev.of_node;
 	const struct st_fdma_driverdata *drvdata;
+	u32 nr_channels;
 	int ret, i;

 	drvdata = device_get_match_data(&pdev->dev);

-	fdev = devm_kzalloc(&pdev->dev, sizeof(*fdev), GFP_KERNEL);
-	if (!fdev)
-		return -ENOMEM;
-
-	ret = st_fdma_parse_dt(pdev, drvdata, fdev);
+	ret = of_property_read_u32(pdev->dev.of_node, "dma-channels", &nr_channels);
 	if (ret) {
 		dev_err(&pdev->dev, "unable to find platform data\n");
-		goto err;
+		return ret;
 	}

-	fdev->chans = devm_kcalloc(&pdev->dev, fdev->nr_channels,
-				   sizeof(struct st_fdma_chan), GFP_KERNEL);
-	if (!fdev->chans)
+	fdev = devm_kzalloc(&pdev->dev, struct_size(fdev, chans, nr_channels), GFP_KERNEL);
+	if (!fdev)
 		return -ENOMEM;

+	fdev->nr_channels = nr_channels;
 	fdev->dev = &pdev->dev;
 	fdev->drvdata = drvdata;
+	snprintf(fdev->fw_name, FW_NAME_SIZE, "fdma_%s_%d.elf", drvdata->name, drvdata->id);
+
 	platform_set_drvdata(pdev, fdev);

 	fdev->irq = platform_get_irq(pdev, 0);
diff --git a/drivers/dma/st_fdma.h b/drivers/dma/st_fdma.h
index f1e746f7bc7d..27ded555879f 100644
--- a/drivers/dma/st_fdma.h
+++ b/drivers/dma/st_fdma.h
@@ -136,13 +136,13 @@ struct st_fdma_dev {

 	int irq;

-	struct st_fdma_chan *chans;
-
 	spinlock_t dreq_lock;
 	unsigned long dreq_mask;

 	u32 nr_channels;
 	char fw_name[FW_NAME_SIZE];
+
+	struct st_fdma_chan chans[] __counted_by(nr_channels);
 };

 /* Peripheral Registers*/
--
2.54.0

Re: [PATCHv2] dmaengine: st_fdma: simplify allocation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] The IRQ handler iterates over the interrupt status register without verifying bounds against `nr_channels`, allowing hardware to trigger out-of-bounds memory accesses.
- [High] The driver requests the IRQ before initializing the virtual DMA channels, leading to a potential crash due to uninitialized spinlock access if a pending interrupt fires immediately.
- [High] The `st_fdma_remove()` function fails to kill the `vchan` tasklets, leading to a potential use-after-free when the `fdev` structure is freed by devres.
- [High] Squashing the negative error code from `platform_get_irq()` to `-EINVAL` breaks deferred probing.
- [High] On the probe error path, `st_fdma_free()` removes channels from the `dma_device.channels` list before the devres unwinder calls `dma_async_device_unregister()`, leaking sysfs devices and percpu memory.
--

commit ce37cbca5ef5b425ece0170e4264843d1f67946c
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: st_fdma: simplify allocation

Use a flexible array member to combine kzalloc and kcalloc to a single
allocation. Add __counted_by for extra runtime analysis. Assign counting
variable after allocation before any array accesses.

> diff --git a/drivers/dma/st_fdma.c b/drivers/dma/st_fdma.c
> index d9547017f3bd6..3ec0d6731b8dc 100644
> --- a/drivers/dma/st_fdma.c
> +++ b/drivers/dma/st_fdma.c
[ ... ]
> @@ -742,27 +732,26 @@ static int st_fdma_probe(struct platform_device *pdev)
>  	struct st_fdma_dev *fdev;
>  	struct device_node *np = pdev->dev.of_node;
>  	const struct st_fdma_driverdata *drvdata;
> +	u32 nr_channels;
>  	int ret, i;
>  
>  	drvdata = device_get_match_data(&pdev->dev);
>  
> -	fdev = devm_kzalloc(&pdev->dev, sizeof(*fdev), GFP_KERNEL);
> -	if (!fdev)
> -		return -ENOMEM;
> -
> -	ret = st_fdma_parse_dt(pdev, drvdata, fdev);
> +	ret = of_property_read_u32(pdev->dev.of_node, "dma-channels", &nr_channels);
>  	if (ret) {
>  		dev_err(&pdev->dev, "unable to find platform data\n");
> -		goto err;
> +		return ret;
>  	}
>  
> -	fdev->chans = devm_kcalloc(&pdev->dev, fdev->nr_channels,
> -				   sizeof(struct st_fdma_chan), GFP_KERNEL);
> -	if (!fdev->chans)
> +	fdev = devm_kzalloc(&pdev->dev, struct_size(fdev, chans, nr_channels), GFP_KERNEL);
> +	if (!fdev)
>  		return -ENOMEM;
>  
> +	fdev->nr_channels = nr_channels;
>  	fdev->dev = &pdev->dev;
>  	fdev->drvdata = drvdata;
> +	snprintf(fdev->fw_name, FW_NAME_SIZE, "fdma_%s_%d.elf", drvdata->name, drvdata->id);
> +
>  	platform_set_drvdata(pdev, fdev);
>  
>  	fdev->irq = platform_get_irq(pdev, 0);

[Severity: High]
This isn't a bug introduced by this patch, but squashing the negative error
code from platform_get_irq() to -EINVAL breaks deferred probing.

platform_get_irq() can return -EPROBE_DEFER if the interrupt controller is
not yet ready. By overwriting the error code, the driver prevents the
subsystem from retrying the probe later:

	fdev->irq = platform_get_irq(pdev, 0);
	if (fdev->irq < 0)
		return -EINVAL;

Does this code leave the device non-functional on systems dependent on
specific probe ordering?

[Severity: High]
This is a pre-existing issue, but does the driver request the IRQ before
initializing the virtual DMA channels?

In st_fdma_probe(), devm_request_irq() is called before the loop that
invokes vchan_init():

	ret = devm_request_irq(&pdev->dev, fdev->irq, st_fdma_irq_handler, 0,
			       dev_name(&pdev->dev), fdev);
	...
	for (i = 0; i < fdev->nr_channels; i++) {
		...
		vchan_init(&fchan->vchan, &fdev->dma_device);
	}

If an interrupt is already pending from a previous system state, could
st_fdma_irq_handler() run immediately and attempt to acquire
fchan->vchan.lock before vchan_init() has initialized it, leading to an
uninitialized spinlock access?

[Severity: High]
This isn't a bug introduced by this patch, but does the IRQ handler iterate
over the interrupt status register without verifying bounds against
nr_channels?

drivers/dma/st_fdma.c:st_fdma_irq_handler() {
	...
	for (; int_sta != 0 ; int_sta >>= 2, fchan++) {
	...

If the hardware sets interrupt status bits for channels equal to or greater
than the configured nr_channels, could this increment the fchan pointer
past the end of the fdev->chans flexible array and cause out-of-bounds
memory accesses?

[Severity: High]
This is a pre-existing issue, but does st_fdma_remove() fail to explicitly
kill the vchan tasklets?

drivers/dma/st_fdma.c:st_fdma_remove() {
	struct st_fdma_dev *fdev = platform_get_drvdata(pdev);

	devm_free_irq(&pdev->dev, fdev->irq, fdev);
	...

The device removal callback does not call tasklet_kill() for the channels.
When st_fdma_remove() returns, devres automatically frees the fdev
structure. If a tasklet was scheduled right before the IRQ was freed and
runs after the struct is freed, could it access the vchan struct embedded
in fdev and cause a use-after-free?

[Severity: High]
This isn't a bug introduced by this patch, but does st_fdma_free() remove
channels from the dma_device.channels list before the devres unwinder calls
dma_async_device_unregister()?

During the probe error path, st_fdma_free() is called manually:

	err_rproc:
		st_fdma_free(fdev);

This function executes list_del(&fchan->vchan.chan.device_node), removing
channels from the device list:

drivers/dma/st_fdma.c:st_fdma_free() {
	...
		list_del(&fchan->vchan.chan.device_node);
	...

When the devres unwinder subsequently calls dma_async_device_unregister(),
does it iterate over an empty list and skip the channel unregistration,
leaking the allocated percpu memory and sysfs nodes?

> diff --git a/drivers/dma/st_fdma.h b/drivers/dma/st_fdma.h
> index f1e746f7bc7dc..27ded555879fb 100644
> --- a/drivers/dma/st_fdma.h
> +++ b/drivers/dma/st_fdma.h
[ ... ]
> @@ -136,13 +136,13 @@ struct st_fdma_dev {
>  
>  	int irq;
>  
> -	struct st_fdma_chan *chans;
> -
>  	spinlock_t dreq_lock;
>  	unsigned long dreq_mask;
>  
>  	u32 nr_channels;
>  	char fw_name[FW_NAME_SIZE];
> +
> +	struct st_fdma_chan chans[] __counted_by(nr_channels);
>  };

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

Re: [PATCHv2] dmaengine: st_fdma: simplify allocation

[Frank Li]

On Sun, Jun 07, 2026 at 10:18:29PM -0700, Rosen Penev wrote:

Nit: dmaengine: st_fdma: simplify allocation by using flexible array

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> Use a flexible array member to combine kzalloc and kcalloc to a single
> allocation.
>
> Add __counted_by for extra runtime analysis. Assign counting variable
> after allocation before any array accesses.
>
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>

Re: [PATCHv2] dmaengine: st_fdma: simplify allocation

[Rosen Penev]

On Mon, Jun 8, 2026 at 9:25 AM Frank Li <Frank.li@oss.nxp.com> wrote:
>
> On Sun, Jun 07, 2026 at 10:18:29PM -0700, Rosen Penev wrote:
>
> Nit: dmaengine: st_fdma: simplify allocation by using flexible array
that's in the description. Did it that was to not have it as long,

flexible array member is the proper terminology.
>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> > Use a flexible array member to combine kzalloc and kcalloc to a single
> > allocation.
> >
> > Add __counted_by for extra runtime analysis. Assign counting variable
> > after allocation before any array accesses.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >

Re: [PATCHv2] dmaengine: st_fdma: simplify allocation

[Frank Li]

On Mon, Jun 08, 2026 at 01:52:09PM -0700, Rosen Penev wrote:
> On Mon, Jun 8, 2026 at 9:25 AM Frank Li <Frank.li@oss.nxp.com> wrote:
> >
> > On Sun, Jun 07, 2026 at 10:18:29PM -0700, Rosen Penev wrote:
> >
> > Nit: dmaengine: st_fdma: simplify allocation by using flexible array
> that's in the description. Did it that was to not have it as long,
>
> flexible array member is the proper terminology.

subject should provide most important information and summary what you did.
prefer pattern is

	do what for ...

	It is too general (simplify allocation\fix wraning\....)


Frank

> >
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> >
> > > Use a flexible array member to combine kzalloc and kcalloc to a single
> > > allocation.
> > >
> > > Add __counted_by for extra runtime analysis. Assign counting variable
> > > after allocation before any array accesses.
> > >
> > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > ---
> > >