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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The DesignWare PCIe controller integration rejects eDMA initialization for HDMA platforms with more than 8 channels because `dw_pcie_edma_find_channels` and `dw_pcie_edma_irq_vector` unconditionally enforce the old 8-channel limit.

Pre-existing issues:
- [High] The arguments `dir` and `ch` are swapped when calling `GET_CH_32` in `dw_hdma_v0_core_ch_status`, causing it to read the wrong hardware registers.
- [High] The driver requests shared IRQs before the interrupt context data is fully initialized, exposing a NULL pointer dereference window if an interrupt fires early.
--

commit cc43ddee72586cbf9f93d80ae20f2b0a9e539a31
Author: Devendra K Verma <devendra.verma@amd.com>

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

This commit updates the DesignWare eDMA driver to support up to 64 read and
write channels for HDMA. It introduces HDMA-specific macros, adjusts channel 
counts in device structures, and switches bitmasks to 64-bit to accommodate 
the higher channel counts.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index c2feb3adc79fa..adf1b3939f96c 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> @@ -1079,6 +1079,8 @@ int dw_edma_probe(struct dw_edma_chip *chip)
>  	struct dw_edma *dw;
>  	u32 wr_alloc = 0;
>  	u32 rd_alloc = 0;
> +	u16 max_wr_cnt;
> +	u16 max_rd_cnt;
>  	int i, err;
>  
>  	if (!chip)
> @@ -1094,20 +1096,25 @@ int dw_edma_probe(struct dw_edma_chip *chip)
>  
>  	dw->chip = chip;
>  
> -	if (dw->chip->mf == EDMA_MF_HDMA_NATIVE)
> +	if (dw->chip->mf == EDMA_MF_HDMA_NATIVE) {
>  		dw_hdma_v0_core_register(dw);
> -	else
> +		max_wr_cnt = HDMA_MAX_WR_CH;
> +		max_rd_cnt = HDMA_MAX_RD_CH;
> +	} else {
>  		dw_edma_v0_core_register(dw);
> +		max_wr_cnt = EDMA_MAX_WR_CH;
> +		max_rd_cnt = EDMA_MAX_RD_CH;
> +	}
>  
>  	raw_spin_lock_init(&dw->lock);
>  
>  	dw->wr_ch_cnt = min_t(u16, chip->ll_wr_cnt,
>  			      dw_edma_core_ch_count(dw, EDMA_DIR_WRITE));
> -	dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt, EDMA_MAX_WR_CH);
> +	dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt, max_wr_cnt);
>  
>  	dw->rd_ch_cnt = min_t(u16, chip->ll_rd_cnt,
>  			      dw_edma_core_ch_count(dw, EDMA_DIR_READ));
> -	dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt, EDMA_MAX_RD_CH);
> +	dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt, max_rd_cnt);
>  
>  	if (!dw->wr_ch_cnt && !dw->rd_ch_cnt)
>  		return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does the current probe sequence expose a
window for a NULL pointer dereference if an early interrupt fires?

In dw_edma_probe(), we call dw_edma_irq_request() which registers a shared
IRQ handler:

    err = request_irq(irq, dw_edma_interrupt_common,
                      IRQF_SHARED, dw->name, &dw->irq[0]);

However, the interrupt context data (irq->dw) isn't populated until the later
call to dw_edma_channel_setup():

    irq->dw = dw;

If another device sharing the same IRQ line triggers an interrupt before
dw_edma_channel_setup() completes, could the handler dereference a NULL
dw_irq->dw pointer?

[ ... ]
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 632abb8b481cf..61064de293b85 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
[ ... ]
> @@ -118,19 +129,20 @@ dw_hdma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir,
>  	unsigned long total, pos, val;
>  	irqreturn_t ret = IRQ_NONE;
>  	struct dw_edma_chan *chan;
> -	unsigned long off, mask;
> +	DECLARE_BITMAP(mask, 64);
> +	unsigned long off;
>  
>  	if (dir == EDMA_DIR_WRITE) {
>  		total = dw->wr_ch_cnt;
>  		off = 0;
> -		mask = dw_irq->wr_mask;
> +		bitmap_from_u64(mask, dw_irq->wr_mask);
>  	} else {
>  		total = dw->rd_ch_cnt;
>  		off = dw->wr_ch_cnt;
> -		mask = dw_irq->rd_mask;
> +		bitmap_from_u64(mask, dw_irq->rd_mask);
>  	}
>  
> -	for_each_set_bit(pos, &mask, total) {
> +	for_each_set_bit(pos, mask, total) {
>  		chan = &dw->chan[pos + off];
>  
>  		val = dw_hdma_v0_core_status_int(chan);

[Severity: High]
This is a pre-existing issue, but is it possible that 
dw_hdma_v0_core_ch_status() reads from the wrong hardware register by
swapping its arguments?

When checking the status, the GET_CH_32 macro expects the direction first, 
then the channel:

    GET_CH_32(dw, dir, ch, name)

However, dw_hdma_v0_core_ch_status() passes chan->id before chan->dir:

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

Could this lead to incorrect channel status checks?

[ ... ]
> 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
>  
>  struct dw_edma;
>  

[Severity: High]
With the expansion to 64 channels, will the DesignWare PCIe controller
integration reject valid HDMA configurations?

In dw_pcie_edma_find_channels(), the code unconditionally enforces the old
EDMA_MAX_WR_CH (8) limits:

    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;

Similarly, dw_pcie_edma_irq_vector() enforces a maximum of 16 vectors:

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

Do these bounding checks in the PCIe controller driver also need to be
updated to handle the new HDMA_MAX_WR_CH and HDMA_MAX_RD_CH limits, to
prevent the device probe from completely failing?

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