Re: [PATCH RFC 1/3] dmaengine: Support address bus widths of 32 bytes and above

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unsynchronized capabilities bitmask in `dma_get_slave_caps` breaks consumer capability queries.
- [Medium] The helper `__dma_set_addr_mask` improperly rejects `DMA_SLAVE_BUSWIDTH_UNDEFINED`, breaking driver migration.
--

commit 87eade13a70506a258fd058989e20b6c29344173
Author: Nuno Sá <nuno.sa@analog.com>

dmaengine: Support address bus widths of 32 bytes and above

This commit introduces bitmap-based masks for source and destination address
widths to support widths of 32 bytes and above, alongside the legacy u32
fields. It updates dma_get_slave_caps() to fold legacy u32 capabilities into
the new bitmaps and adds helper functions to migrate controllers and
consumers incrementally.

> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index 9049171df8578..a21bc9c140aa5 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
[ ... ]
> @@ -593,7 +593,25 @@ int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
>  		return -ENXIO;
>  
>  	caps->src_addr_widths = device->src_addr_widths;
> +	if (bitmap_empty(device->src_addr_widths_mask, DMA_SLAVE_BUSWIDTH_MAX)) {
> +		bitmap_zero(caps->src_addr_widths_mask, DMA_SLAVE_BUSWIDTH_MAX);
> +		bitmap_from_arr32(caps->src_addr_widths_mask,
> +				  &device->src_addr_widths, 32);
> +	} else {
> +		bitmap_copy(caps->src_addr_widths_mask,
> +			    device->src_addr_widths_mask, DMA_SLAVE_BUSWIDTH_MAX);
> +	}
> +
>  	caps->dst_addr_widths = device->dst_addr_widths;
> +	if (bitmap_empty(device->dst_addr_widths_mask, DMA_SLAVE_BUSWIDTH_MAX)) {
> +		bitmap_zero(caps->dst_addr_widths_mask, DMA_SLAVE_BUSWIDTH_MAX);
> +		bitmap_from_arr32(caps->dst_addr_widths_mask,
> +				  &device->dst_addr_widths, 32);
> +	} else {
> +		bitmap_copy(caps->dst_addr_widths_mask,
> +			    device->dst_addr_widths_mask, DMA_SLAVE_BUSWIDTH_MAX);
> +	}
> +

[Severity: High]
Will this initial bitmap population get out of sync when the channel-specific
device_caps() callback is invoked later in this function?

Currently, dma_get_slave_caps() copies the capabilities into the new
bitmasks here, but later calls:

        if (device->device_caps)
                device->device_caps(chan, caps);

If a legacy driver (like stm32-dma3) dynamically restricts capabilities by
modifying the u32 fields (e.g., caps->src_addr_widths &= ~BIT(...)), those
changes won't be reflected in caps->src_addr_widths_mask.

Could modernized consumers relying on the bitmap read stale capabilities
and potentially attempt unsupported hardware bus widths?

>  	caps->directions = device->directions;
>  	caps->min_burst = device->min_burst;
>  	caps->max_burst = device->max_burst;
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index b3d251c9734e9..e249158aa4a5f 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
[ ... ]
> @@ -1678,4 +1692,82 @@ static inline struct device *dmaengine_get_dma_device(struct dma_chan *chan)
>  	return chan->device->dev;
>  }
>  
> +static inline enum dma_slave_buswidth
> +__dma_slave_caps_get_width_min(const unsigned long *bitmask)
[ ... ]
> +static inline int __dma_set_addr_mask(unsigned long *bitmask,
> +				      enum dma_slave_buswidth *widths,
> +				      unsigned int n_widths)
> +{
> +	for (unsigned int i = 0; i < n_widths; i++) {
> +		switch (widths[i]) {
> +		case DMA_SLAVE_BUSWIDTH_1_BYTE:
> +		case DMA_SLAVE_BUSWIDTH_2_BYTES:
> +		case DMA_SLAVE_BUSWIDTH_3_BYTES:
> +		case DMA_SLAVE_BUSWIDTH_4_BYTES:
> +		case DMA_SLAVE_BUSWIDTH_8_BYTES:
> +		case DMA_SLAVE_BUSWIDTH_16_BYTES:
> +		case DMA_SLAVE_BUSWIDTH_32_BYTES:
> +		case DMA_SLAVE_BUSWIDTH_64_BYTES:
> +		case DMA_SLAVE_BUSWIDTH_128_BYTES:
> +			break;
> +		default:
> +			return -EINVAL;

[Severity: Medium]
Does this strict validation unintentionally reject
DMA_SLAVE_BUSWIDTH_UNDEFINED?

Since DMA_SLAVE_BUSWIDTH_UNDEFINED is a valid capability bit (BIT(0))
actively advertised by several DMA drivers (such as xilinx_dpdma, dw/core,
and pl330), returning -EINVAL here might prevent these controllers from
migrating to the new bitmap API.

> +		}
> +
> +		__set_bit(widths[i], bitmask);
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-dmaengine-support-wider-dma-masks-v1-0-da23a8dcb756@analog.com?part=1