Re: [PATCH v8] 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] The DesignWare PCIe controller frontend fails initialization for platforms attempting to use 64 HDMA channels due to hardcoded legacy channel limits.
- [Medium] The dw-edma-pcie endpoint driver silently caps dynamically detected channel counts to 8.

Pre-existing issues:
- [High] A race condition during initialization causes a guaranteed NULL pointer dereference if a shared or spurious interrupt occurs early.
- [High] Incorrect argument order in dw_hdma_v0_core_ch_status leads to invalid register reads, breaking DMA status reporting.
--

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

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

This patch introduces support for up to 64 read and write channels in the 
DesignWare HDMA controller. It defines new macros for HDMA maximum channel 
counts, updates data structures, and transitions bitmasks to 64-bit bitmaps 
to accommodate the expanded channel limits.

> Changes in v3:
>   o Reverted the FIX for AI reported GET_CH_32() issue, as
>     per the recommendation of reviewers, need to create
>     separate patch for it.

[Severity: High]
This isn't a bug introduced by this patch, but does the incorrect argument 
order in dw_hdma_v0_core_ch_status() still lead to invalid register reads?

When the DMA status is checked, dw_hdma_v0_core_ch_status() calls GET_CH_32() 
with the channel ID and direction swapped:

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

The macro evaluates the numeric channel ID as the direction and uses the 
direction as the channel index, which breaks DMA status reporting.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> @@ -925,9 +925,9 @@ static int dw_edma_channel_setup(struct dw_edma *dw, u32 wr_alloc, u32 rd_alloc)
>  		irq = &dw->irq[pos];
>  
>  		if (chan->dir == EDMA_DIR_WRITE)
> -			irq->wr_mask |= BIT(chan->id);
> +			bitmap_set(irq->wr_mask, chan->id, 1);
>  		else
> -			irq->rd_mask |= BIT(chan->id);
> +			bitmap_set(irq->rd_mask, chan->id, 1);
>  
>  		irq->dw = dw;

[Severity: High]
This is a pre-existing issue, but does this initialization sequence leave a 
race condition that causes a NULL pointer dereference if a shared interrupt 
fires early?

In dw_edma_probe(), dw_edma_irq_request() calls request_irq() with IRQF_SHARED 
while dw->irq[i].dw is still NULL.

If an interrupt fires immediately after registration, 
dw_edma_interrupt_common() executes and dereferences dw_irq->dw to call 
dw->core->handle_int. This will dereference the NULL pointer before 
dw_edma_channel_setup() finishes initialization here.

> diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
> --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
> @@ -62,11 +62,11 @@ struct dw_edma_pcie_data {
>  	/* eDMA registers location */
>  	struct dw_edma_block		rg;
>  	/* eDMA memory linked list location */
> -	struct dw_edma_block		ll_wr[EDMA_MAX_WR_CH];
> -	struct dw_edma_block		ll_rd[EDMA_MAX_RD_CH];
> +	struct dw_edma_block		ll_wr[HDMA_MAX_WR_CH];
> +	struct dw_edma_block		ll_rd[HDMA_MAX_RD_CH];

[Severity: Medium]
Does the dw-edma-pcie endpoint driver silently cap dynamically detected 
channel counts to 8, defeating the purpose of the 64-channel support?

During probe, dw_edma_pcie_get_xilinx_dma_data() reads the channel count from 
the hardware VSEC but caps it using the static template limit:

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

Since pdata is initialized from xilinx_mdb_data which statically defines a 
maximum of 8 channels, the dynamically read channel count is bounded and 
will never exceed 8.

> diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
> --- 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]
Will the DesignWare PCIe controller frontend fail initialization with -EINVAL 
for platforms attempting to use 64 HDMA channels?

When an HDMA platform is probed, dw_pcie_edma_find_channels() in 
drivers/pci/controller/dwc/pcie-designware.c still uses the legacy 
EDMA_MAX_WR_CH limit:

    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;

Additionally, dw_pcie_edma_irq_vector() rejects requested IRQ numbers 
that are greater than the legacy maximums:

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

This artificial cap at 16 seems insufficient for platforms using the new 
64-channel support.

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