[v4,3/5] dmaengine: sun6i-dma: Add num_channels_per_reg for flexible interrupt mapping
Commit Message
The `sun6i-dma` driver previously assumed a fixed number of channels
per interrupt register. For example, `sun6i_kill_tasklet` was hardcoded
to disable interrupts only for registers 0 and 1. `DMA_MAX_CHANNELS` was
also not in used previously, and the old SoCs never has more than 16
channels.
The A733 has a different interrupt structure where the number of
channels per register may differ. Add `num_channels_per_reg` to `struct
sun6i_dma_config` to make the interrupt handling logic handware-agnostic
Update `sun6i_dma_interrupt`, `sun6i_dma_start_desc`, and
`sun6i_kill_tasklet` to use this value.
Additionally, set `DMA_MAX_CHANNELS` to 16 to ensure loops over
interrupts are correctly bounded, aligning with the hardware
specifications.
Signed-off-by: Yuanshen Cao <alex.caoys@gmail.com>
---
drivers/dma/sun6i-dma.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
Comments
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] Integer division truncation in `sun6i_dma_interrupt` loop bound silently drops interrupts for higher-numbered channels.
- [High] Incorrect physical channel indexing in `sun6i_dma_interrupt` misroutes interrupts for higher channels.
- [High] TOCTOU race in `sun6i_dma_interrupt` leads to a NULL pointer dereference on channel teardown.
--
commit 0015717509538d0aa3c622981170b196ad6d4d64
Author: Yuanshen Cao <alex.caoys@gmail.com>
dmaengine: sun6i-dma: Add num_channels_per_reg for flexible interrupt mapping
The `sun6i-dma` driver previously assumed a fixed number of channels per
interrupt register. The A733 has a different interrupt structure where the
number of channels per register may differ, so add `num_channels_per_reg` to
`struct sun6i_dma_config` to make the interrupt handling logic hardware-agnostic.
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 53c54161e32a9..b96d7d90f6b53 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
[ ... ]
> @@ -585,7 +587,7 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
> int i, j, ret = IRQ_NONE;
> u32 status;
>
> - for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) {
> + for (i = 0; i < sdev->num_pchans / sdev->cfg->num_channels_per_reg; i++) {
[Severity: High]
This isn't a bug introduced by this patch, but does this loop silently ignore
higher-numbered channels in sun6i_dma_interrupt() due to integer division
truncation?
If the total number of channels is not perfectly divisible by the channels per
register (e.g. Allwinner H3 has 12 channels, 8 per register), this calculation
truncates the remainder. For H3, 12 / 8 truncates to 1, causing the loop to
only process i = 0 (channels 0-7) and skip i = 1 (channels 8-11), stalling
those higher channels.
The patch correctly uses DIV_ROUND_UP() in sun6i_dma_dump_com_regs(), should
it be used here as well?
> status = sun6i_read_irq_stat(sdev, i);
> if (!status)
> continue;
> @@ -595,7 +597,7 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
>
> sun6i_write_irq_stat(sdev, i, status);
>
> - for (j = 0; (j < DMA_IRQ_CHAN_NR) && status; j++) {
> + for (j = 0; (j < sdev->cfg->num_channels_per_reg) && status; j++) {
> pchan = sdev->pchans + j;
[Severity: High]
This is a pre-existing issue, but does this array indexing misroute interrupts
for channels in subsequent registers?
The index calculation in sun6i_dma_interrupt() completely ignores the register
index `i`. For instance, if an interrupt fires on channel 8 (which is in
register i=1, offset j=0), this will retrieve sdev->pchans[0] instead of
sdev->pchans[8], erroneously completing channel 0's active transfer.
Shouldn't this incorporate i * sdev->cfg->num_channels_per_reg?
> vchan = pchan->vchan;
> if (vchan && (status & vchan->irq_type)) {
[Severity: High]
This is a pre-existing issue, but does this lockless check introduce a TOCTOU
race leading to a NULL pointer dereference on channel teardown?
If a client driver concurrently terminates the channel (e.g., during device
removal) and sun6i_dma_terminate_all() sets pchan->desc to NULL, this code
can evaluate the following block where pchan->desc->vd is dereferenced:
if (vchan->cyclic) {
vchan_cyclic_callback(&pchan->desc->vd);
} else {
spin_lock(&vchan->vc.lock);
vchan_cookie_complete(&pchan->desc->vd);
Since the cyclic path takes no locks and the non-cyclic path doesn't re-verify
pchan->desc after taking the lock, could this crash when the pointer becomes
NULL mid-execution?
@@ -35,14 +35,13 @@
#define DMA_IRQ_PKG BIT(1)
#define DMA_IRQ_QUEUE BIT(2)
-#define DMA_IRQ_CHAN_NR 8
#define DMA_IRQ_CHAN_WIDTH 4
#define DMA_STAT 0x30
/* Offset between DMA_IRQ_EN and DMA_IRQ_STAT limits number of channels */
-#define DMA_MAX_CHANNELS (DMA_IRQ_CHAN_NR * 0x10 / 4)
+#define DMA_MAX_CHANNELS 16
/*
* sun8i specific registers
@@ -61,6 +60,8 @@
#define DMA_IRQ_EN_OFFSET_A31 0x00
#define DMA_IRQ_STAT_OFFSET_A31 0x10
+#define DMA_IRQ_CHAN_NR_A31 8
+
/*
* Channels specific registers
*/
@@ -157,6 +158,7 @@ struct sun6i_dma_config {
u32 irq_stride;
u32 irq_en_offset;
u32 irq_stat_offset;
+ u32 num_channels_per_reg;
};
/*
@@ -271,7 +273,7 @@ static inline void sun6i_dma_dump_com_regs(struct sun6i_dma_dev *sdev)
{
int i;
- for (i = 0; i < 2; i++) {
+ for (i = 0; i < DIV_ROUND_UP(sdev->num_pchans, sdev->cfg->num_channels_per_reg); i++) {
dev_dbg(sdev->slave.dev, "Common register:\n"
"chan num %d\n"
"\tmask(%04x): 0x%08x\n"
@@ -492,8 +494,8 @@ static int sun6i_dma_start_desc(struct sun6i_vchan *vchan)
sun6i_dma_dump_lli(vchan, pchan->desc->v_lli, pchan->desc->p_lli);
- irq_reg = pchan->idx / DMA_IRQ_CHAN_NR;
- irq_offset = pchan->idx % DMA_IRQ_CHAN_NR;
+ irq_reg = pchan->idx / sdev->cfg->num_channels_per_reg;
+ irq_offset = pchan->idx % sdev->cfg->num_channels_per_reg;
vchan->irq_type = vchan->cyclic ? DMA_IRQ_PKG : DMA_IRQ_QUEUE;
@@ -585,7 +587,7 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
int i, j, ret = IRQ_NONE;
u32 status;
- for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) {
+ for (i = 0; i < sdev->num_pchans / sdev->cfg->num_channels_per_reg; i++) {
status = sun6i_read_irq_stat(sdev, i);
if (!status)
continue;
@@ -595,7 +597,7 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
sun6i_write_irq_stat(sdev, i, status);
- for (j = 0; (j < DMA_IRQ_CHAN_NR) && status; j++) {
+ for (j = 0; (j < sdev->cfg->num_channels_per_reg) && status; j++) {
pchan = sdev->pchans + j;
vchan = pchan->vchan;
if (vchan && (status & vchan->irq_type)) {
@@ -1113,7 +1115,7 @@ static inline void sun6i_kill_tasklet(struct sun6i_dma_dev *sdev)
int i;
/* Disable all interrupts from DMA */
- for (i = 0; i < 2; i++)
+ for (i = 0; i < DMA_MAX_CHANNELS / sdev->cfg->num_channels_per_reg; i++)
sun6i_write_irq_en(sdev, i, 0);
/* Prevent spurious interrupts from scheduling the tasklet */
@@ -1141,7 +1143,8 @@ static inline void sun6i_dma_free(struct sun6i_dma_dev *sdev)
#define SUN6I_DMA_IRQ_A31_COMMON_CFG \
.irq_stride = DMA_IRQ_STRIDE_A31, \
.irq_en_offset = DMA_IRQ_EN_OFFSET_A31, \
- .irq_stat_offset = DMA_IRQ_STAT_OFFSET_A31,
+ .irq_stat_offset = DMA_IRQ_STAT_OFFSET_A31, \
+ .num_channels_per_reg = DMA_IRQ_CHAN_NR_A31,
/*
* For A31: