Re: [PATCH] dmaengine: xilinx: Treat "xlnx,flush-fsync" as a flag

[sashiko-bot@kernel.org]

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

New issues:
- [High] Switching to of_property_read_bool() limits flush_on_fsync to a boolean, breaking selective per-channel flushing and leaving dead code in the driver.

Pre-existing issues:
- [High] The property 'xlnx,irq-delay' is incorrectly read using of_property_read_u8() despite being defined as a 32-bit integer in the device tree bindings.
- [High] Missing bounds check on the number of DMA channels allows an out-of-bounds array write to xdev->chan.
--

commit 509281d674522c9a7b063c96ad8a6bee204fa2c0
Author: Rob Herring (Arm) <robh@kernel.org>

dmaengine: xilinx: Treat "xlnx,flush-fsync" as a flag

This commit updates the Xilinx DMA driver to treat the xlnx,flush-fsync
property as a boolean flag rather than an integer. This aligns the driver
behavior with the device tree binding documentation. It also explicitly leaves
xlnx,irq-delay as an 8-bit read.

> Use the boolean helper so the driver follows the binding. Leave
> "xlnx,irq-delay" as an 8-bit property read because the hardware field
> is 8 bits wide.

[Severity: High]
This is a pre-existing issue, but since it's mentioned here: is it safe to
read a 32-bit device tree cell with of_property_read_u8()?

Device tree properties are 32-bit big-endian values. When reading a 32-bit
property with of_property_read_u8() in xilinx_dma_chan_probe(), only the first
byte in memory is read. For values less than 256, the most significant byte is
0, meaning chan->irq_delay will always evaluate to 0 and silently disable the
interrupt delay timer. Does the hardware register width affect the DT cell
encoding width?

> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 404235c173538..cbb23fd6e0960 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
[ ... ]
> @@ -3262,11 +3262,8 @@ static int xilinx_dma_probe(struct platform_device *pdev)
>  			goto disable_clks;
>  		}
>  
> -		err = of_property_read_u32(node, "xlnx,flush-fsync",
> -					   &xdev->flush_on_fsync);
> -		if (err < 0)
> -			dev_warn(xdev->dev,
> -				 "missing xlnx,flush-fsync property\n");
> +		xdev->flush_on_fsync =
> +			of_property_read_bool(node, "xlnx,flush-fsync");

[Severity: High]
Does this change break backwards compatibility for device trees that rely on
selective flushing?

While this matches the boolean type in the DT binding schema, it ignores the
driver's legacy behavior where it checked for integer values:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_chan_probe() {
    ...
    if (xdev->flush_on_fsync == XILINX_DMA_FLUSH_BOTH ||
        xdev->flush_on_fsync == XILINX_DMA_FLUSH_MM2S)
        chan->flush_on_fsync = true;
    ...
}

Because of_property_read_bool() in xilinx_dma_probe() returns a boolean,
xdev->flush_on_fsync will evaluate to 1 if the property exists, treating all
legacy configurations like <2> (XILINX_DMA_FLUSH_MM2S) or <3>
(XILINX_DMA_FLUSH_S2MM) as <1>.

Does this also leave the explicit conditionals checking for
XILINX_DMA_FLUSH_MM2S and XILINX_DMA_FLUSH_S2MM in xilinx_dma_chan_probe()
as unreachable dead code?

>  	}
>  
>  	err = of_property_read_u32(node, "xlnx,addrwidth", &addr_width);

[Severity: High]
This isn't a bug introduced by this patch, but does the driver lack bounds
checking when parsing the dma-channels property?

In drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_child_probe(), the dma-channels
property dictates the loop limit for calling xilinx_dma_chan_probe():

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_child_probe() {
    ...
    for (i = 0; i < nr_channels; i++) {
        ret = xilinx_dma_chan_probe(xdev, node);
    ...
}

Inside xilinx_dma_chan_probe(), chan->id is incremented and used as an index:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_chan_probe() {
    ...
    xdev->chan[chan->id] = chan;
    ...
}

Since the xdev->chan array is fixed to a size of 32
(XILINX_MCDMA_MAX_CHANS_PER_DEVICE), could a device tree specifying a
larger number of channels result in an out-of-bounds write and memory
corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612215233.1887921-1-robh@kernel.org?part=1