Re: [PATCH 1/4] dmaengine: dw: Add peripheral bus width verification

[Andy Shevchenko]

On Tue, Apr 16, 2024 at 07:28:55PM +0300, Serge Semin wrote:
> Currently the src_addr_width and dst_addr_width fields of the
> dma_slave_config structure are mapped to the CTLx.SRC_TR_WIDTH and
> CTLx.DST_TR_WIDTH fields of the peripheral bus side in order to have the
> properly aligned data passed to the target device. It's done just by
> converting the passed peripheral bus width to the encoded value using the
> __ffs() function. This implementation has several problematic sides:
> 
> 1. __ffs() is undefined if no bit exist in the passed value. Thus if the
> specified addr-width is DMA_SLAVE_BUSWIDTH_UNDEFINED, __ffs() may return
> unexpected value depending on the platform-specific implementation.
> 
> 2. DW AHB DMA-engine permits having the power-of-2 transfer width limited
> by the DMAH_Mk_HDATA_WIDTH IP-core synthesize parameter. Specifying
> bus-width out of that constraints scope will definitely cause unexpected
> result since the destination reg will be only partly touched than the
> client driver implied.
> 
> Let's fix all of that by adding the peripheral bus width verification
> method which would make sure that the passed source or destination address
> width is valid and if undefined then the driver will just fallback to the
> 1-byte width transfer.

Please, add a word that you apply the check in the dwc_config() which is
supposed to be called before preparing any transfer?

..

> +static int dwc_verify_p_buswidth(struct dma_chan *chan)
> +{
> +	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
> +	struct dw_dma *dw = to_dw_dma(chan->device);
> +	u32 reg_width, max_width;
> +
> +	if (dwc->dma_sconfig.direction == DMA_MEM_TO_DEV)
> +		reg_width = dwc->dma_sconfig.dst_addr_width;
> +	else if (dwc->dma_sconfig.direction == DMA_DEV_TO_MEM)
> +		reg_width = dwc->dma_sconfig.src_addr_width;

> +	else /* DMA_MEM_TO_MEM */

Actually not only this direction, but TBH I do not see value in these comments.

> +		return 0;
> +
> +	max_width = dw->pdata->data_width[dwc->dws.p_master];
> +
> +	/* Fall-back to 1byte transfer width if undefined */

1-byte
(as you even used in the commit message correctly)

> +	if (reg_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
> +		reg_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> +	else if (!is_power_of_2(reg_width) || reg_width > max_width)
> +		return -EINVAL;
> +	else /* bus width is valid */
> +		return 0;
> +
> +	/* Update undefined addr width value */
> +	if (dwc->dma_sconfig.direction == DMA_MEM_TO_DEV)
> +		dwc->dma_sconfig.dst_addr_width = reg_width;
> +	else /* DMA_DEV_TO_MEM */
> +		dwc->dma_sconfig.src_addr_width = reg_width;

So, can't you simply call clamp() for both fields in dwc_config()?

> +	return 0;
> +}

..

> +	int err;

Hmm... we have two functions one of which is using different name for this.
Can we have a patch to convert to err the other one?

-- 
With Best Regards,
Andy Shevchenko