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

["Verma, Devendra" <devverma@amd.com>]

Hi

Please check my response inline.

regards,
Devendra

On 23-Jun-26 17:11, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] The 64-channel HDMA feature is unreachable due to hardcoded legacy limits in the driver bindings.
> 
> Pre-existing issues:
> - [High] Potential NULL pointer dereference if a shared IRQ fires before `dw_irq->dw` is initialized.
> --
> 
> commit ae4f94060e0e6e5d4419cd4f95926541afa41dd0
> Author: Devendra K Verma <devendra.verma@amd.com>
> 
> dmaengine: dw-edma: Enable HDMA 64R/W Channels
> 
> This patch expands the designware eDMA driver to support up to 64 read and
> 64 write channels for the HDMA controller, aligning with the hardware
> databook. It introduces new macros for the HDMA channel counts, expands
> the layout of the channel-related structures, updates channel masks to 64-bit,
> and updates the probe sequence to configure the core based on the controller
> type.
> 
>> 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
>> @@ -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);
>> +			irq->wr_mask |= BIT_ULL(chan->id);
>>   		else
>> -			irq->rd_mask |= BIT(chan->id);
>> +			irq->rd_mask |= BIT_ULL(chan->id);
>>   
>>   		irq->dw = dw;
> 
> [Severity: High]
> This is a pre-existing issue, but could a shared interrupt fire before
> irq->dw is initialized here?
> 

As this is pre-existing issue it shall be taken in a separate patch
series.

> In dw_edma_probe(), dw_edma_irq_request() is called before
> dw_edma_channel_setup(). The IRQs are requested with IRQF_SHARED:
> 
> dw_edma_probe() {
>      ...
>      err = dw_edma_irq_request(dw, &wr_alloc, &rd_alloc);
>      ...
>      err = dw_edma_channel_setup(dw, wr_alloc, rd_alloc);
> }
> 
> If the shared IRQ fires immediately after registration, the interrupt handler
> might dereference a NULL dw_irq->dw:
> 
> dw_edma_interrupt_write()
>     dw_edma_core_handle_int()
>         return dw_irq->dw->core->handle_int(dw_irq, dir, done, abort);
> 
> Should irq->dw be initialized prior to calling request_irq()?
> 
> [ ... ]

As the issue existed before any fix of this patch, it shall be taken
in a separate patch series.

> 
>> 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: Medium]
> Will these expanded 64-channel limits actually be reachable with the current
> driver bindings?
> 
> Looking at drivers/pci/controller/dwc/pcie-designware.c,
> dw_pcie_edma_find_channels() still explicitly enforces the legacy 8-channel
> limit and returns -EINVAL if the configuration exceeds it:
> 
>      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;
> 

This particular function targets non-HDMA platforms which suggests that
increasing the channel limit will not affect anything here.
This code shall be retained as is. It is not affected by increase in
limit for HDMA channels.

> Similarly, in drivers/dma/dw-edma/dw-edma-pcie.c, the Xilinx MDB HDMA data
> statically declares 8 channels and dw_edma_pcie_get_xilinx_dma_data() caps
> the write channels:
> 
>      pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
>                     FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH, val));
> 
> Do the hardcoded legacy limits in the driver bindings prevent the new
> 64-channel feature from being fully utilized?
> 

It depends on the user, how many channels it enables. If user wishes to
enable more that 8 channels, be it read/write, then the current code
limits the channels to a ceiling of 8 channels which is a case of
under-utilization of HDMA channels. This patch enables the code to
handle the max supported HDMA channels which is 64 Read/Write.
Thus, using any number of channels below <= 64 is fine when using this
patch. This patch would not create any issue for the existing use cases.