Re: [PATCH v7] dmaengine: dw-edma: Enable HDMA 64R/W Channels

[sashiko-bot@kernel.org]

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

New issues:
- [High] Designware PCIe core bounds checks reject HDMA configurations with more than 8 channels.
- [High] Standalone PCIe eDMA driver statically limits hardware to 8 channels.

Pre-existing issues:
- [Critical] Missing register offset when setting the eDMA/HDMA base register pointer.
- [High] Incorrect arguments passed to GET_CH_32 macro causing wrong channel registers to be accessed.
--

--- Patch [1]: [PATCH v7] dmaengine: dw-edma: Enable HDMA 64R/W Channels ---
commit 4834a312ef82b7f9b290aedb3a1f566db2548955
Author: Devendra K Verma <devendra.verma@amd.com>

dmaengine: dw-edma: Enable HDMA 64R/W Channels

As per 'Designware Cores PCI Express Controller Databook',
Section 7.1 - Overview, HDMA supports 64 Read and 64 Write
channels. Current controller driver supports up to 8 read and
write channels only. In order to utilize all the channels the
controller driver need to have the channel related structs
and variables as per the number of channels supported by IP.
Following changes are made to enable 64 Read / 64 Write
channel support:

> diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
> index 1fafd5b0e3153..da7a5cc93ad43 100644
> --- a/include/linux/dma/edma.h
> +++ b/include/linux/dma/edma.h
> @@ -14,6 +14,8 @@
>  
>  #define EDMA_MAX_WR_CH                                  8
>  #define EDMA_MAX_RD_CH                                  8
> +#define HDMA_MAX_WR_CH                                  64
> +#define HDMA_MAX_RD_CH                                  64

[Severity: High]
This patch introduces 64-channel support for HDMA, but does this break
when used with the Designware PCIe host driver?

Looking at drivers/pci/controller/dwc/pcie-designware.c:dw_pcie_edma_find_channels():

	/* Sanity check the channels count if the mapping was incorrect */
	if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > EDMA_MAX_WR_CH ||
	    !pci->edma.ll_rd_cnt || pci->edma.ll_rd_cnt > EDMA_MAX_RD_CH)
		return -EINVAL;

And in dw_pcie_edma_irq_vector():

	if (nr >= EDMA_MAX_WR_CH + EDMA_MAX_RD_CH)
		return -EINVAL;

Will these sanity checks reject HDMA configurations with more than 8 channels
because they strictly enforce the EDMA_MAX_WR_CH (8) limit?

[Severity: High]
Does the standalone PCIe eDMA driver statically limit the hardware to
8 channels?

Looking at drivers/dma/dw-edma/dw-edma-pcie.c:

static const struct dw_edma_pcie_data xilinx_mdb_data = {
...
	.wr_ch_cnt			= 8,
	.rd_ch_cnt			= 8,
};

And later in dw_edma_pcie_get_xilinx_dma_data():

	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH, val));

Doesn't the min() operation cap the dynamically detected hardware channel
count to the static limit of 8, negating the new 64-channel HDMA support
on Xilinx MDB controllers?

[Severity: Critical]
This is a pre-existing issue, but is there a missing register offset when
setting the eDMA/HDMA base register pointer?

In drivers/dma/dw-edma/dw-edma-pcie.c:dw_edma_pcie_probe():

	chip->reg_base = pcim_iomap_table(pdev)[vsec_data->rg.bar];
	if (!chip->reg_base)
		return -ENOMEM;

Should this map the whole PCIe BAR but also add vsec_data->rg.off to
chip->reg_base? Because all register accesses via __dw_regs(dw) directly
dereference chip->reg_base, will all reads and writes hit the beginning
of the BAR instead of the actual hardware registers on hardware where
rg.off is non-zero?

[Severity: High]
This is a pre-existing issue, but are the arguments to GET_CH_32
swapped here?

In drivers/dma/dw-edma/dw-hdma-v0-core.c:dw_hdma_v0_core_ch_status():

static enum dma_status dw_hdma_v0_core_ch_status(struct dw_edma_chan *chan)
{
	struct dw_edma *dw = chan->dw;
	u32 tmp;

	tmp = FIELD_GET(HDMA_V0_CH_STATUS_MASK,
			GET_CH_32(dw, chan->id, chan->dir, ch_stat));

The GET_CH_32(dw, dir, ch, name) macro expects the direction as the second
argument and the channel ID as the third. Here it is invoked with
chan->id as the direction and chan->dir as the channel ID.

Since chan->id is evaluated as the direction, won't any channel ID > 0 be
treated as a READ direction? And won't chan->dir restrict the accessed
channel index to 0 or 1, meaning it will always read the status of channel
0 or 1 instead of the intended channel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708134343.3806759-1-devendra.verma@amd.com?part=1