Re: [PATCH v3 5/9] dmaengine: stm32-dma3: Use bus width capability helpers

[Nuno Sá]

On Mon, Aug 31, 2026 at 04:01:24PM +0200, Amelie Delaunay wrote:
> Hi,
> 
> Thanks for the updates!
> 
> On 8/31/26 13:46, Nuno Sá wrote:
> > Advertise the controller-wide bus width capabilities through the new
> > dma_bus_width_set_many() helper and clear the per-channel unsupported
> > widths through dma_bus_width_clear().
> > 
> > While at it, validate the requested transfer bus widths against the new
> > capability mask instead of the legacy u32 one. Besides not depending on
> > the legacy field anymore, this also stops BIT() from being fed a bus
> > width coming from the device tree without any bound check.
> > 
> > Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
> > Signed-off-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> > ---
> >   drivers/dma/stm32/stm32-dma3.c | 34 +++++++++++++++++++++-------------
> >   1 file changed, 21 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/dma/stm32/stm32-dma3.c b/drivers/dma/stm32/stm32-dma3.c
> > index 4724e7fa0008..0689aeb7cff5 100644
> > --- a/drivers/dma/stm32/stm32-dma3.c
> > +++ b/drivers/dma/stm32/stm32-dma3.c
> > @@ -9,6 +9,7 @@
> >   #include <linux/bitfield.h>
> >   #include <linux/clk.h>
> >   #include <linux/dma-mapping.h>
> > +#include <linux/dma/engine/widthmask.h>
> >   #include <linux/dmaengine.h>
> >   #include <linux/dmapool.h>
> >   #include <linux/init.h>
> > @@ -588,7 +589,8 @@ static int stm32_dma3_chan_prep_hw(struct stm32_dma3_chan *chan, enum dma_transf
> >   	dbl_max = chan->dma_config.dst_maxburst ? : 1;
> >   	/* Following conditions would raise User Setting Error interrupt */
> > -	if (!(dma_device.src_addr_widths & BIT(sdw)) || !(dma_device.dst_addr_widths & BIT(ddw))) {
> > +	if (!dma_bus_width_test(dma_device.src_bus_widths, sdw) ||
> > +	    !dma_bus_width_test(dma_device.dst_bus_widths, ddw)) {
> 
> Just to come back to one point you mentioned in your cover letter, "this
> stops BIT() from being fed an unvalidated device tree bus width". Here, sdw
> and ddw are set either by dma_config.[src|dst]_addr_width, or forced to
> DMA_SLAVE_BUSWIDTH_4_BYTES or DMA_SLAVE_BUSWIDTH_8_BYTES depending on the
> channel capabilities. So, unless I'm missing something, they do not come
> from an "unvalidated *device tree* bus width".

Yeah, nowadays I let commits and covers be written mainly by AI (I do
review it though) and I guess that slipped. I mean, it's not wrong but I
guess it should be not in the cover as it's very focused on the ADI dmac
controller (and in a legacy path which used to read DT).

I'll probably drop that from the cover.

> Anyway, it's great to see another use of the new API.
> 
> 
> >   		dev_err(chan2dev(chan), "Bus width (src=%u, dst=%u) not supported\n", sdw, ddw);
> >   		return -EINVAL;
> >   	}
> > @@ -1469,14 +1471,14 @@ static void stm32_dma3_caps(struct dma_chan *c, struct dma_slave_caps *caps)
> >   	if (!chan->fifo_size) {
> >   		caps->max_burst = 0;
> > -		caps->src_addr_widths &= ~BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> > -		caps->dst_addr_widths &= ~BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> > +		dma_bus_width_clear(caps->src_bus_widths, DMA_SLAVE_BUSWIDTH_8_BYTES);
> > +		dma_bus_width_clear(caps->dst_bus_widths, DMA_SLAVE_BUSWIDTH_8_BYTES);
> >   	} else {
> >   		/* Burst transfer should not exceed half of the fifo size */
> >   		caps->max_burst = chan->max_burst;
> >   		if (caps->max_burst < DMA_SLAVE_BUSWIDTH_8_BYTES) {
> > -			caps->src_addr_widths &= ~BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> > -			caps->dst_addr_widths &= ~BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> > +			dma_bus_width_clear(caps->src_bus_widths, DMA_SLAVE_BUSWIDTH_8_BYTES);
> > +			dma_bus_width_clear(caps->dst_bus_widths, DMA_SLAVE_BUSWIDTH_8_BYTES);
> >   		}
> >   	}
> >   }
> > @@ -1734,6 +1736,12 @@ static int stm32_dma3_probe(struct platform_device *pdev)
> >   	struct reset_control *reset;
> >   	struct stm32_dma3_chan *chan;
> >   	struct dma_device *dma_dev;
> > +	enum dma_slave_buswidth buswidths[] = {
> > +		DMA_SLAVE_BUSWIDTH_1_BYTE,
> > +		DMA_SLAVE_BUSWIDTH_2_BYTES,
> > +		DMA_SLAVE_BUSWIDTH_4_BYTES,
> > +		DMA_SLAVE_BUSWIDTH_8_BYTES,
> > +	};
> >   	u32 master_ports, chan_reserved, i, verr;
> >   	u64 hwcfgr;
> >   	int ret;
> > @@ -1775,14 +1783,14 @@ static int stm32_dma3_probe(struct platform_device *pdev)
> >   	 * channel, and can only access address at even boundaries, multiple of the buswidth.
> >   	 */
> >   	dma_dev->copy_align = DMAENGINE_ALIGN_8_BYTES;
> > -	dma_dev->src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> > -				   BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> > -				   BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> > -				   BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> > -	dma_dev->dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> > -				   BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> > -				   BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> > -				   BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> > +	ret = dma_bus_width_set_many(dma_dev->src_bus_widths, buswidths, ARRAY_SIZE(buswidths));
> > +	if (ret)
> > +		goto err_clk_disable;
> > +
> > +	ret = dma_bus_width_set_many(dma_dev->dst_bus_widths, buswidths, ARRAY_SIZE(buswidths));
> > +	if (ret)
> > +		goto err_clk_disable;
> > +
> >   	dma_dev->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV) | BIT(DMA_MEM_TO_MEM);
> >   	dma_dev->descriptor_reuse = true;
> > 
> 
> It looks nicer this way. Thanks!
> 
> Reviewed-by: Amelie Delaunay <amelie.delaunay@xxxxxxxxxxx>

Thx!

- Nuno Sá

> Regards,
> Amelie