dmaengine: dw-edma: Enable HDMA 64R/W Channels
Subsystems (2)
PATCH v5 1/1 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:
o Defined HDMA specific macros to reflect the channel count.
o The count of ll_regions and dt_regions in dw_edma_chip and dw_edma_pcie_data shall be in accordance to number of read and write channels.
o In dw_edma_probe() configure the channels as per the channels of the IP used.
o Changed mask types to u64 for higher channel counts.
---
Changes in v4:
o Changed 'mask' variable to a bitmap type as per the review comment.
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.
Changes in v2:
o Fixed the pre-existing bug related to GET_CH_32 interchanging the channel direction and id. This bug was not caused by any version of this patch.
o Fixed the issue when using for_each_set_bit() for mask of u64 type.
Changes in v1:
o On review recommendation of sashiko bot, in the function dw_hdma_v0_core_off(), the loop iterates over registers as per the number of channels enabled and not on total number of channels supported.
o Changed mask types to u64 for higher channel counts.
--- drivers/dma/dw-edma/dw-edma-core.c | 19 +++++++++++----- drivers/dma/dw-edma/dw-edma-core.h | 4 ++-- drivers/dma/dw-edma/dw-edma-pcie.c | 8 +++---- drivers/dma/dw-edma/dw-hdma-v0-core.c | 32 ++++++++++++++++++--------- drivers/dma/dw-edma/dw-hdma-v0-regs.h | 2 +- include/linux/dma/edma.h | 10 +++++---- 6 files changed, 48 insertions(+), 27 deletions(-)
drivers/dma/dw-edma/dw-edma-core.c +13 -6
@@ -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;memcpy (&chan->msi, &irq->msi,sizeof (chan->msi));
@@ -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 ;
drivers/dma/dw-edma/dw-edma-core.h +2 -2
@@ -91,8 +91,8 @@ struct dw_edma_chan {struct dw_edma_irq {struct msi_msg msi; -u32 wr_mask; -u32 rd_mask; +u64 wr_mask; +u64 rd_mask;struct dw_edma *dw; };
drivers/dma/dw-edma/dw-edma-pcie.c +4 -4
@@ -61,11 +61,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 ];/* eDMA memory data location */ -struct dw_edma_block dt_wr[EDMA_MAX_WR_CH ]; -struct dw_edma_block dt_rd[EDMA_MAX_RD_CH ]; +struct dw_edma_block dt_wr[HDMA_MAX_WR_CH ]; +struct dw_edma_block dt_rd[HDMA_MAX_RD_CH ];/* Other */ enum dw_edma_map_format mf;u8 irqs;
drivers/dma/dw-edma/dw-hdma-v0-core.c +22 -10
@@ -53,13 +53,24 @@ __dw_ch_regs(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch)static void dw_hdma_v0_core_off (struct dw_edma *dw) {int id; +enum dw_edma_dir dir; + + dir =EDMA_DIR_WRITE ; +for (id =0 ; id < dw->wr_ch_cnt; id++) { +SET_CH_32 (dw, dir, id, int_setup, +HDMA_V0_STOP_INT_MASK |HDMA_V0_ABORT_INT_MASK ); +SET_CH_32 (dw, dir, id, int_clear, +HDMA_V0_STOP_INT_MASK |HDMA_V0_ABORT_INT_MASK ); +SET_CH_32 (dw, dir, id, ch_en,0 ); + } -for (id =0 ; id <HDMA_V0_MAX_NR_CH ; id++) { -SET_BOTH_CH_32 (dw, id, int_setup, -HDMA_V0_STOP_INT_MASK |HDMA_V0_ABORT_INT_MASK ); -SET_BOTH_CH_32 (dw, id, int_clear, -HDMA_V0_STOP_INT_MASK |HDMA_V0_ABORT_INT_MASK ); -SET_BOTH_CH_32 (dw, id, ch_en,0 ); + dir =EDMA_DIR_READ ; +for (id =0 ; id < dw->rd_ch_cnt; id++) { +SET_CH_32 (dw, dir, id, int_setup, +HDMA_V0_STOP_INT_MASK |HDMA_V0_ABORT_INT_MASK ); +SET_CH_32 (dw, dir, id, int_clear, +HDMA_V0_STOP_INT_MASK |HDMA_V0_ABORT_INT_MASK ); +SET_CH_32 (dw, dir, id, ch_en,0 ); } }
@@ -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);
drivers/dma/dw-edma/dw-hdma-v0-regs.h +1 -1
@@ -11,7 +11,7 @@#include <linux/dmaengine.h> -#define HDMA_V0_MAX_NR_CH 8 +#define HDMA_V0_MAX_NR_CH 64#define HDMA_V0_CH_EN BIT(0)#define HDMA_V0_LOCAL_ABORT_INT_EN BIT(6)#define HDMA_V0_REMOTE_ABORT_INT_EN BIT(5)
include/linux/dma/edma.h +6 -4
@@ -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 64struct dw_edma ;
@@ -89,12 +91,12 @@ struct dw_edma_chip {u16 ll_wr_cnt;u16 ll_rd_cnt;/* link list address */ -struct dw_edma_region ll_region_wr[EDMA_MAX_WR_CH ]; -struct dw_edma_region ll_region_rd[EDMA_MAX_RD_CH ]; +struct dw_edma_region ll_region_wr[HDMA_MAX_WR_CH ]; +struct dw_edma_region ll_region_rd[HDMA_MAX_RD_CH ];/* data region */ -struct dw_edma_region dt_region_wr[EDMA_MAX_WR_CH ]; -struct dw_edma_region dt_region_rd[EDMA_MAX_RD_CH ]; +struct dw_edma_region dt_region_wr[HDMA_MAX_WR_CH ]; +struct dw_edma_region dt_region_rd[HDMA_MAX_RD_CH ];/* interrupt emulation */ int db_irq;
Signature · 1 line
2.43.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
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 <[email protected]>
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.
Quoted 4 lines
drivers/dma/dw-edma/dw-edma-core.c +0
-0
[ ... ]
Quoted 29 lines
drivers/dma/dw-edma/dw-edma-core.c +9
-2@@ -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 ));
drivers/dma/dw-edma/dw-edma-core.c int dw_edma_probe(struct dw_edma_chip *chip) · line 1120
Quoted 10 lines
drivers/dma/dw-edma/dw-edma-core.c +2
-2- 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?
[ ... ]
Quoted 4 lines
drivers/dma/dw-edma/dw-hdma-v0-core.c +0
-0
[ ... ]
Quoted 15 lines
drivers/dma/dw-edma/dw-hdma-v0-core.c +3
-2@@ -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;
drivers/dma/dw-edma/dw-hdma-v0-core.c dw_hdma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir, · line 148
Quoted 10 lines
drivers/dma/dw-edma/dw-hdma-v0-core.c +2
-2off = 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?
[ ... ]
include/linux/dma/edma.h line 21
Quoted 13 lines
include/linux/dma/edma.h +2
-0@@ -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 64struct 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?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Hi Frank, Vinod
Do you have any suggestion about handling of the repeated comments from AI?
On every version of this patch the similar issues have been raised and I am replying with the same answers as many version-times. Please suggest so that multiple replies to the same queries by AI bot can be managed.
regards,
Devendra
Quoted 78 lines · On 26-Jun-26 19:16, [email protected] wrote:
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 becausedw_pcie_edma_find_channelsanddw_pcie_edma_irq_vectorunconditionally enforce the old 8-channel limit.Pre-existing issues:
- [High] The argumentsdirandchare swapped when callingGET_CH_32indw_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 <[email protected]>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.
Quoted 4 lines
drivers/dma/dw-edma/dw-edma-core.c +0
-0[ ... ]
Quoted 29 lines
drivers/dma/dw-edma/dw-edma-core.c +9
-2@@ -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 ));drivers/dma/dw-edma/dw-edma-core.c int dw_edma_probe(struct dw_edma_chip *chip) · line 1120
Quoted 10 lines
drivers/dma/dw-edma/dw-edma-core.c +2
-2- 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 calldw_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 todw_edma_channel_setup():
Quoted 7 lines
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 NULLdw_irq->dwpointer?[ ... ]
As this is pre-existing issue, it shall be taken in a separate patch series.
Quoted 41 lines
Quoted 4 lines
drivers/dma/dw-edma/dw-hdma-v0-core.c +0
-0[ ... ]
Quoted 15 lines
drivers/dma/dw-edma/dw-hdma-v0-core.c +3
-2@@ -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;drivers/dma/dw-edma/dw-hdma-v0-core.c dw_hdma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir, · line 148
Quoted 10 lines
drivers/dma/dw-edma/dw-hdma-v0-core.c +2
-2off = 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 thatdw_hdma_v0_core_ch_status()reads from the wrong hardware register by swapping its arguments?When checking the status, the
GET_CH_32macro expects the direction first, then the channel:GET_CH_32(dw, dir, ch, name)
Quoted 8 lines
However,
dw_hdma_v0_core_ch_status()passeschan->idbeforechan->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?
[ ... ]
This issue shall be taken in a separate patch series.
Quoted 18 lines
include/linux/dma/edma.h line 21
Quoted 13 lines
include/linux/dma/edma.h +2
-0@@ -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 64struct dw_edma ;[Severity: High]
With the expansion to 64 channels, will the DesignWare PCIe controller integration reject valid HDMA configurations?
Quoted 7 lines
In
dw_pcie_edma_find_channels(), the code unconditionally enforces the oldEDMA_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;
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.
Quoted 9 lines
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_CHandHDMA_MAX_RD_CHlimits, to prevent the device probe from completely failing?
This shall not affect the working of IP, with minimum interrupt count of 1 also IP should work. If required, it shall be taken in a separate patch series.
Quoted 8 lines · Devendra
Hi Frank, Vinod
Do you have any suggestion about handling of the repeated comments from AI?
On every version of this patch the similar issues have been raised and I am replying with the same answers as many version-times. Please suggest so that multiple replies to the same queries by AI bot can be managed.
You can omit pre-existing. Only reply once when patch close to land. I hope there are tool, which can help identified comments and pull your previous reply.
On method may help:
After I provided review-by, you can reply you already checked AI's results, so It help vnod offload his checking work.
AI is quite new for us. we are looking for efficent flow to handle it.
Frank
Quoted 178 lines
regards,
DevendraQuoted 71 lines · On 26-Jun-26 19:16, [email protected] wrote:
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 becausedw_pcie_edma_find_channelsanddw_pcie_edma_irq_vectorunconditionally enforce the old 8-channel limit.Pre-existing issues:
- [High] The argumentsdirandchare swapped when callingGET_CH_32indw_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 <[email protected]>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.
Quoted 4 lines
drivers/dma/dw-edma/dw-edma-core.c +0
-0[ ... ]
Quoted 22 lines
drivers/dma/dw-edma/dw-edma-core.c +9
-2@@ -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);drivers/dma/dw-edma/dw-edma-core.c int dw_edma_probe(struct dw_edma_chip *chip) · line 1114
Quoted 10 lines
drivers/dma/dw-edma/dw-edma-core.c +2
-2dw->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 calldw_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 todw_edma_channel_setup():Quoted 7 lines
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 NULLdw_irq->dwpointer?[ ... ]
As this is pre-existing issue, it shall be taken in a separate patch series.
Quoted 38 lines
Quoted 4 lines
drivers/dma/dw-edma/dw-hdma-v0-core.c +0
-0[ ... ]
Quoted 12 lines
drivers/dma/dw-edma/dw-hdma-v0-core.c +3
-2@@ -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);drivers/dma/dw-edma/dw-hdma-v0-core.c dw_hdma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir, · line 145
Quoted 10 lines
drivers/dma/dw-edma/dw-hdma-v0-core.c +2
-2}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 thatdw_hdma_v0_core_ch_status()reads from the wrong hardware register by swapping its arguments?When checking the status, the
GET_CH_32macro expects the direction first, then the channel:GET_CH_32(dw, dir, ch, name)Quoted 8 lines
However,
dw_hdma_v0_core_ch_status()passeschan->idbeforechan->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?
[ ... ]
This issue shall be taken in a separate patch series.
Quoted 15 lines
include/linux/dma/edma.h line 18
Quoted 10 lines
include/linux/dma/edma.h +2
-0@@ -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 64struct dw_edma ;[Severity: High]
With the expansion to 64 channels, will the DesignWare PCIe controller integration reject valid HDMA configurations?Quoted 7 lines
In
dw_pcie_edma_find_channels(), the code unconditionally enforces the oldEDMA_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;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.
Quoted 9 lines
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_CHandHDMA_MAX_RD_CHlimits, to prevent the device probe from completely failing?This shall not affect the working of IP, with minimum interrupt count of 1 also IP should work. If required, it shall be taken in a separate patch series.
Vinod Koul
maintainerQuoted 10 lines · Frank Li
Quoted 8 lines · Devendra
Hi Frank, Vinod
Do you have any suggestion about handling of the repeated comments from AI?
On every version of this patch the similar issues have been raised and I am replying with the same answers as many version-times. Please suggest so that multiple replies to the same queries by AI bot can be managed.
Quoted 3 lines
You can omit pre-existing. Only reply once when patch close to land. I hope there are tool, which can help identified comments and pull your previous reply.
Right, fixing preexisting is indeed optional but since it may impact your device, we are looking forward to fixes on these from you. More important for us is not adding new issues.
Quoted 4 lines
On method may help:
After I provided review-by, you can reply you already checked AI's results, so It help vnod offload his checking work.
Thanks this helps a lot.
Quoted 1 line
AI is quite new for us. we are looking for efficent flow to handle it.
Just like any other tool, it is tool which helps us. People should also run the locally as well.
Signature · 1 line
~Vinod
Quoted 24 lines · Vinod Koul
Quoted 10 lines · Frank Li
Quoted 8 lines · Devendra
Hi Frank, Vinod
Do you have any suggestion about handling of the repeated comments from AI?
On every version of this patch the similar issues have been raised and I am replying with the same answers as many version-times. Please suggest so that multiple replies to the same queries by AI bot can be managed.Quoted 3 lines
You can omit pre-existing. Only reply once when patch close to land. I hope there are tool, which can help identified comments and pull your previous reply.
Right, fixing preexisting is indeed optional but since it may impact your device, we are looking forward to fixes on these from you. More important for us is not adding new issues.
Quoted 4 lines
On method may help:
After I provided review-by, you can reply you already checked AI's results, so It help vnod offload his checking work.
Quoted 7 lines
Thanks this helps a lot.
Quoted 1 line
AI is quite new for us. we are looking for efficent flow to handle it.
Just like any other tool, it is tool which helps us. People should also run the locally as well.
Thank you Vinod and Frank for your suggestion!
The intention to ask was not to complain but to stop flood every version
with the similar issue and comment on top of it.
Your replies helped in understanding the process and thus will take this suggestion and apply it for future reviews. Thank you once again!
-Devendra
Quoted 110 lines · Devendra K Verma
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:
o Defined HDMA specific macros to reflect the channel count.
o The count ofll_regionsanddt_regionsindw_edma_chipanddw_edma_pcie_datashall be in accordance to number of read and write channels.
o Indw_edma_probe()configure the channels as per the channels of the IP used.
o Changed mask types to u64 for higher channel counts.Signed-off-by Devendra K Verma <[email protected]>---
Changes in v4:
o Changed 'mask' variable to a bitmap type as per the review comment.Changes in v3:
o Reverted the FIX for AI reportedGET_CH_32()issue, as per the recommendation of reviewers, need to create separate patch for it.Changes in v2:
o Fixed the pre-existing bug related toGET_CH_32interchanging the channel direction and id. This bug was not caused by any version of this patch.
o Fixed the issue when usingfor_each_set_bit()for mask of u64 type.Changes in v1:
o On review recommendation of sashiko bot, in the functiondw_hdma_v0_core_off(), the loop iterates over registers as per the number of channels enabled and not on total number of channels supported.
o Changed mask types to u64 for higher channel counts.--- drivers/dma/dw-edma/dw-edma-core.c | 19 +++++++++++----- drivers/dma/dw-edma/dw-edma-core.h | 4 ++-- drivers/dma/dw-edma/dw-edma-pcie.c | 8 +++---- drivers/dma/dw-edma/dw-hdma-v0-core.c | 32 ++++++++++++++++++--------- drivers/dma/dw-edma/dw-hdma-v0-regs.h | 2 +- include/linux/dma/edma.h | 10 +++++---- 6 files changed, 48 insertions(+), 27 deletions(-)drivers/dma/dw-edma/dw-edma-core.c +13
-6@@ -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;memcpy (&chan->msi, &irq->msi,sizeof (chan->msi));@@ -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 ;drivers/dma/dw-edma/dw-edma-core.h +0
-0
drivers/dma/dw-edma/dw-edma-core.h struct dw_edma_chan { · line 95
Quoted 7 lines
drivers/dma/dw-edma/dw-edma-core.h +2
-2@@ -91,8 +91,8 @@ struct dw_edma_chan {struct dw_edma_irq {struct msi_msg msi; -u32 wr_mask; -u32 rd_mask; +u64 wr_mask; +u64 rd_mask;
Can you direct use DECLARE_BITMAP(rd_mask, 64) here?
Quoted 7 lines
drivers/dma/dw-edma/dw-edma-core.h +0
-0struct dw_edma *dw; };drivers/dma/dw-edma/dw-edma-pcie.c +0
-0
...
drivers/dma/dw-edma/dw-edma-pcie.c dw_hdma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir, · line 121
Quoted 8 lines
drivers/dma/dw-edma/dw-edma-pcie.c +0
-1} }@@ -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;
after change wr_mask to BITMAP
mask -> *mask
So needn't change this code if support more channel in future.
Frank
Quoted 66 lines
drivers/dma/dw-edma/dw-edma-pcie.c +5
-3+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);drivers/dma/dw-edma/dw-hdma-v0-regs.h +1
-1@@ -11,7 +11,7 @@#include <linux/dmaengine.h> -#define HDMA_V0_MAX_NR_CH 8 +#define HDMA_V0_MAX_NR_CH 64#define HDMA_V0_CH_EN BIT(0)#define HDMA_V0_LOCAL_ABORT_INT_EN BIT(6)#define HDMA_V0_REMOTE_ABORT_INT_EN BIT(5)include/linux/dma/edma.h +6
-5@@ -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 64struct dw_edma ;@@ -89,12 +91,12 @@ struct dw_edma_chip {u16 ll_wr_cnt;u16 ll_rd_cnt;/* link list address */ -struct dw_edma_region ll_region_wr[EDMA_MAX_WR_CH ]; -struct dw_edma_region ll_region_rd[EDMA_MAX_RD_CH ]; +struct dw_edma_region ll_region_wr[HDMA_MAX_WR_CH ]; +struct dw_edma_region ll_region_rd[HDMA_MAX_RD_CH ];/* data region */ -struct dw_edma_region dt_region_wr[EDMA_MAX_WR_CH ]; -struct dw_edma_region dt_region_rd[EDMA_MAX_RD_CH ]; +struct dw_edma_region dt_region_wr[HDMA_MAX_WR_CH ]; +struct dw_edma_region dt_region_rd[HDMA_MAX_RD_CH ];/* interrupt emulation */ int db_irq; --2.43.0
Quoted 140 lines · Frank Li
Quoted 110 lines · Devendra K Verma
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:
o Defined HDMA specific macros to reflect the channel count.
o The count ofll_regionsanddt_regionsindw_edma_chipanddw_edma_pcie_datashall be in accordance to number of read and write channels.
o Indw_edma_probe()configure the channels as per the channels of the IP used.
o Changed mask types to u64 for higher channel counts.Signed-off-by Devendra K Verma <[email protected]>---
Changes in v4:
o Changed 'mask' variable to a bitmap type as per the review comment.Changes in v3:
o Reverted the FIX for AI reportedGET_CH_32()issue, as per the recommendation of reviewers, need to create separate patch for it.Changes in v2:
o Fixed the pre-existing bug related toGET_CH_32interchanging the channel direction and id. This bug was not caused by any version of this patch.
o Fixed the issue when usingfor_each_set_bit()for mask of u64 type.Changes in v1:
o On review recommendation of sashiko bot, in the functiondw_hdma_v0_core_off(), the loop iterates over registers as per the number of channels enabled and not on total number of channels supported.
o Changed mask types to u64 for higher channel counts.--- drivers/dma/dw-edma/dw-edma-core.c | 19 +++++++++++----- drivers/dma/dw-edma/dw-edma-core.h | 4 ++-- drivers/dma/dw-edma/dw-edma-pcie.c | 8 +++---- drivers/dma/dw-edma/dw-hdma-v0-core.c | 32 ++++++++++++++++++--------- drivers/dma/dw-edma/dw-hdma-v0-regs.h | 2 +- include/linux/dma/edma.h | 10 +++++---- 6 files changed, 48 insertions(+), 27 deletions(-)drivers/dma/dw-edma/dw-edma-core.c +13
-6@@ -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;memcpy (&chan->msi, &irq->msi,sizeof (chan->msi));@@ -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 ;drivers/dma/dw-edma/dw-edma-core.h +0
-0drivers/dma/dw-edma/dw-edma-core.h struct dw_edma_chan { · line 95
Quoted 7 lines
drivers/dma/dw-edma/dw-edma-core.h +2
-2@@ -91,8 +91,8 @@ struct dw_edma_chan {struct dw_edma_irq {struct msi_msg msi; -u32 wr_mask; -u32 rd_mask; +u64 wr_mask; +u64 rd_mask;Can you direct use
DECLARE_BITMAP(rd_mask, 64)here?Quoted 7 lines
drivers/dma/dw-edma/dw-edma-core.h +0
-0struct dw_edma *dw; };drivers/dma/dw-edma/dw-edma-pcie.c +0
-0...
drivers/dma/dw-edma/dw-edma-pcie.c dw_hdma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir, · line 121
Quoted 8 lines
} }drivers/dma/dw-edma/dw-edma-pcie.c +0
-1@@ -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;after change
wr_maskto BITMAP
Quoted 6 lines
mask -> *maskSo needn't change this code if support more channel in future.
Frank
It looks good to make this piece of code generic and support for more channel but I did not push that change as the final limitation comes from the HDMA IP which as per the documentation supports upto 64 channels only. As there is no channel increase BITMAP was not implemented for *_mask variable.
-Devendra
Quoted 66 lines
Quoted 66 lines
drivers/dma/dw-edma/dw-edma-pcie.c +5
-3+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);drivers/dma/dw-edma/dw-hdma-v0-regs.h +1
-1@@ -11,7 +11,7 @@#include <linux/dmaengine.h> -#define HDMA_V0_MAX_NR_CH 8 +#define HDMA_V0_MAX_NR_CH 64#define HDMA_V0_CH_EN BIT(0)#define HDMA_V0_LOCAL_ABORT_INT_EN BIT(6)#define HDMA_V0_REMOTE_ABORT_INT_EN BIT(5)include/linux/dma/edma.h +6
-5@@ -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 64struct dw_edma ;@@ -89,12 +91,12 @@ struct dw_edma_chip {u16 ll_wr_cnt;u16 ll_rd_cnt;/* link list address */ -struct dw_edma_region ll_region_wr[EDMA_MAX_WR_CH ]; -struct dw_edma_region ll_region_rd[EDMA_MAX_RD_CH ]; +struct dw_edma_region ll_region_wr[HDMA_MAX_WR_CH ]; +struct dw_edma_region ll_region_rd[HDMA_MAX_RD_CH ];/* data region */ -struct dw_edma_region dt_region_wr[EDMA_MAX_WR_CH ]; -struct dw_edma_region dt_region_rd[EDMA_MAX_RD_CH ]; +struct dw_edma_region dt_region_wr[HDMA_MAX_WR_CH ]; +struct dw_edma_region dt_region_rd[HDMA_MAX_RD_CH ];/* interrupt emulation */ int db_irq; --2.43.0
Quoted 146 lines · Devendra
Quoted 144 lines · Frank Li
Quoted 110 lines · Devendra K Verma
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:
o Defined HDMA specific macros to reflect the channel count.
o The count ofll_regionsanddt_regionsindw_edma_chipanddw_edma_pcie_datashall be in accordance to number of read and write channels.
o Indw_edma_probe()configure the channels as per the channels of the IP used.
o Changed mask types to u64 for higher channel counts.Signed-off-by Devendra K Verma <[email protected]>---
Changes in v4:
o Changed 'mask' variable to a bitmap type as per the review comment.Changes in v3:
o Reverted the FIX for AI reportedGET_CH_32()issue, as per the recommendation of reviewers, need to create separate patch for it.Changes in v2:
o Fixed the pre-existing bug related toGET_CH_32interchanging the channel direction and id. This bug was not caused by any version of this patch.
o Fixed the issue when usingfor_each_set_bit()for mask of u64 type.Changes in v1:
o On review recommendation of sashiko bot, in the functiondw_hdma_v0_core_off(), the loop iterates over registers as per the number of channels enabled and not on total number of channels supported.
o Changed mask types to u64 for higher channel counts.--- drivers/dma/dw-edma/dw-edma-core.c | 19 +++++++++++----- drivers/dma/dw-edma/dw-edma-core.h | 4 ++-- drivers/dma/dw-edma/dw-edma-pcie.c | 8 +++---- drivers/dma/dw-edma/dw-hdma-v0-core.c | 32 ++++++++++++++++++--------- drivers/dma/dw-edma/dw-hdma-v0-regs.h | 2 +- include/linux/dma/edma.h | 10 +++++---- 6 files changed, 48 insertions(+), 27 deletions(-)drivers/dma/dw-edma/dw-edma-core.c +13
-6@@ -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;memcpy (&chan->msi, &irq->msi,sizeof (chan->msi));@@ -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 ;drivers/dma/dw-edma/dw-edma-core.h +0
-0drivers/dma/dw-edma/dw-edma-core.h struct dw_edma_chan { · line 95
Quoted 7 lines
drivers/dma/dw-edma/dw-edma-core.h +2
-2@@ -91,8 +91,8 @@ struct dw_edma_chan {struct dw_edma_irq {struct msi_msg msi; -u32 wr_mask; -u32 rd_mask; +u64 wr_mask; +u64 rd_mask;Can you direct use
DECLARE_BITMAP(rd_mask, 64)here?Quoted 7 lines
drivers/dma/dw-edma/dw-edma-core.h +0
-0struct dw_edma *dw; };drivers/dma/dw-edma/dw-edma-pcie.c +0
-0...
drivers/dma/dw-edma/dw-edma-pcie.c dw_hdma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir, · line 121
Quoted 8 lines
} }drivers/dma/dw-edma/dw-edma-pcie.c +0
-1@@ -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;after change
wr_maskto BITMAPmask -> *maskSo needn't change this code if support more channel in future.
Quoted 8 lines
Quoted 2 lines
Frank
It looks good to make this piece of code generic and support for more channel but I did not push that change as the final limitation comes from the HDMA IP which as per the documentation supports upto 64 channels only. As there is no channel increase BITMAP was not implemented for
*_maskvariable.
DECLARE_BITMAP(rd_mask, 64) the size is the same as u64. needn't call below two bitmap_from_u64().
irq->wr_mask |= BIT_ULL(chan->id), use bitmap_set().
Everything will be simple and better extendable.
Frank
Quoted 70 lines
-Devendra
Quoted 66 lines
Quoted 66 lines
drivers/dma/dw-edma/dw-edma-pcie.c +5
-3+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);drivers/dma/dw-edma/dw-hdma-v0-regs.h +1
-1@@ -11,7 +11,7 @@#include <linux/dmaengine.h> -#define HDMA_V0_MAX_NR_CH 8 +#define HDMA_V0_MAX_NR_CH 64#define HDMA_V0_CH_EN BIT(0)#define HDMA_V0_LOCAL_ABORT_INT_EN BIT(6)#define HDMA_V0_REMOTE_ABORT_INT_EN BIT(5)include/linux/dma/edma.h +6
-5@@ -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 64struct dw_edma ;@@ -89,12 +91,12 @@ struct dw_edma_chip {u16 ll_wr_cnt;u16 ll_rd_cnt;/* link list address */ -struct dw_edma_region ll_region_wr[EDMA_MAX_WR_CH ]; -struct dw_edma_region ll_region_rd[EDMA_MAX_RD_CH ]; +struct dw_edma_region ll_region_wr[HDMA_MAX_WR_CH ]; +struct dw_edma_region ll_region_rd[HDMA_MAX_RD_CH ];/* data region */ -struct dw_edma_region dt_region_wr[EDMA_MAX_WR_CH ]; -struct dw_edma_region dt_region_rd[EDMA_MAX_RD_CH ]; +struct dw_edma_region dt_region_wr[HDMA_MAX_WR_CH ]; +struct dw_edma_region dt_region_rd[HDMA_MAX_RD_CH ];/* interrupt emulation */ int db_irq; --2.43.0
Quoted 156 lines · Frank Li
Quoted 146 lines · Devendra
Quoted 144 lines · Frank Li
Quoted 110 lines · Devendra K Verma
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:
o Defined HDMA specific macros to reflect the channel count.
o The count ofll_regionsanddt_regionsindw_edma_chipanddw_edma_pcie_datashall be in accordance to number of read and write channels.
o Indw_edma_probe()configure the channels as per the channels of the IP used.
o Changed mask types to u64 for higher channel counts.Signed-off-by Devendra K Verma <[email protected]>---
Changes in v4:
o Changed 'mask' variable to a bitmap type as per the review comment.Changes in v3:
o Reverted the FIX for AI reportedGET_CH_32()issue, as per the recommendation of reviewers, need to create separate patch for it.Changes in v2:
o Fixed the pre-existing bug related toGET_CH_32interchanging the channel direction and id. This bug was not caused by any version of this patch.
o Fixed the issue when usingfor_each_set_bit()for mask of u64 type.Changes in v1:
o On review recommendation of sashiko bot, in the functiondw_hdma_v0_core_off(), the loop iterates over registers as per the number of channels enabled and not on total number of channels supported.
o Changed mask types to u64 for higher channel counts.--- drivers/dma/dw-edma/dw-edma-core.c | 19 +++++++++++----- drivers/dma/dw-edma/dw-edma-core.h | 4 ++-- drivers/dma/dw-edma/dw-edma-pcie.c | 8 +++---- drivers/dma/dw-edma/dw-hdma-v0-core.c | 32 ++++++++++++++++++--------- drivers/dma/dw-edma/dw-hdma-v0-regs.h | 2 +- include/linux/dma/edma.h | 10 +++++---- 6 files changed, 48 insertions(+), 27 deletions(-)drivers/dma/dw-edma/dw-edma-core.c +13
-6@@ -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;memcpy (&chan->msi, &irq->msi,sizeof (chan->msi));@@ -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 ;drivers/dma/dw-edma/dw-edma-core.h +0
-0drivers/dma/dw-edma/dw-edma-core.h struct dw_edma_chan { · line 95
Quoted 7 lines
drivers/dma/dw-edma/dw-edma-core.h +2
-2@@ -91,8 +91,8 @@ struct dw_edma_chan {struct dw_edma_irq {struct msi_msg msi; -u32 wr_mask; -u32 rd_mask; +u64 wr_mask; +u64 rd_mask;Can you direct use
DECLARE_BITMAP(rd_mask, 64)here?Quoted 7 lines
drivers/dma/dw-edma/dw-edma-core.h +0
-0struct dw_edma *dw; };drivers/dma/dw-edma/dw-edma-pcie.c +0
-0...
drivers/dma/dw-edma/dw-edma-pcie.c dw_hdma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir, · line 121
Quoted 8 lines
} }drivers/dma/dw-edma/dw-edma-pcie.c +0
-1@@ -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;after change
wr_maskto BITMAPmask -> *maskSo needn't change this code if support more channel in future.
Quoted 8 lines
Quoted 2 lines
Frank
It looks good to make this piece of code generic and support for more channel but I did not push that change as the final limitation comes from the HDMA IP which as per the documentation supports upto 64 channels only. As there is no channel increase BITMAP was not implemented for
*_maskvariable.
Quoted 9 lines
DECLARE_BITMAP(rd_mask, 64)the size is the same as u64. needn't call below twobitmap_from_u64().
irq->wr_mask|=BIT_ULL(chan->id), usebitmap_set().Everything will be simple and better extendable.
Frank
Thank you for the suggestion, Frank!
With this change dw-edma-v0-core.c also require a minor change to work with {wr/rd}_mask as bitmap-type in 'struct dw_edma_irq'.
I will push the changes in next version of the patch.
-Devendra
Quoted 70 lines
Quoted 70 lines
-Devendra
Quoted 66 lines
Quoted 66 lines
drivers/dma/dw-edma/dw-edma-pcie.c +5
-3+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);drivers/dma/dw-edma/dw-hdma-v0-regs.h +1
-1@@ -11,7 +11,7 @@#include <linux/dmaengine.h> -#define HDMA_V0_MAX_NR_CH 8 +#define HDMA_V0_MAX_NR_CH 64#define HDMA_V0_CH_EN BIT(0)#define HDMA_V0_LOCAL_ABORT_INT_EN BIT(6)#define HDMA_V0_REMOTE_ABORT_INT_EN BIT(5)include/linux/dma/edma.h +6
-5@@ -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 64struct dw_edma ;@@ -89,12 +91,12 @@ struct dw_edma_chip {u16 ll_wr_cnt;u16 ll_rd_cnt;/* link list address */ -struct dw_edma_region ll_region_wr[EDMA_MAX_WR_CH ]; -struct dw_edma_region ll_region_rd[EDMA_MAX_RD_CH ]; +struct dw_edma_region ll_region_wr[HDMA_MAX_WR_CH ]; +struct dw_edma_region ll_region_rd[HDMA_MAX_RD_CH ];/* data region */ -struct dw_edma_region dt_region_wr[EDMA_MAX_WR_CH ]; -struct dw_edma_region dt_region_rd[EDMA_MAX_RD_CH ]; +struct dw_edma_region dt_region_wr[HDMA_MAX_WR_CH ]; +struct dw_edma_region dt_region_rd[HDMA_MAX_RD_CH ];/* interrupt emulation */ int db_irq; --2.43.0