Re: [PATCH v9 2/3] dmaengine: amlogic: Add general DMA driver for A9

On 2026/6/26 13:53, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
> - [High] Use-After-Free during driver removal due to missing IRQ synchronization.
> - [High] Sleeping in atomic context when freeing virtual DMA descriptors.
> - [High] Out-of-bounds write in `aml_dma_prep_slave_sg` when processing zero-length scatterlist elements.
> - [High] Broken pause/resume state machine logic leading to channel stall and state corruption.
> - [Medium] `aml_dma_terminate_all()` fails to reset channel error state, causing permanent stall.
> - [Medium] `aml_dma_tx_status()` improperly handles unissued descriptors, causing false errors and incorrect residues.
> - [Medium] Mismatched DMA address registers used for 64-bit physical addresses.
> --
> 
> commit 7102a7639af273fc6303ccde475117debb548165
> Author: Xianwei Zhao<[email protected]>
> 
> dmaengine: amlogic: Add general DMA driver for A9
> 
> Amlogic A9 SoCs include a general-purpose DMA controller that can be used
> by multiple peripherals, such as I2C PIO and I3C. Each peripheral group
> is associated with a dedicated DMA channel in hardware.
> 
>> diff --git a/drivers/dma/amlogic-dma.c b/drivers/dma/amlogic-dma.c
>> index 0000000000000..bce6932f3a12e
>> --- /dev/null
>> +++ b/drivers/dma/amlogic-dma.c
> [ ... ]
> 
>> +static void aml_dma_free_desc(struct virt_dma_desc *vd)
>> +{
>> +     struct aml_dma_desc *aml_desc = to_aml_dma_desc(vd);
>> +
>> +     dma_free_coherent(aml_desc->dma_device->dev,
>> +                       aml_desc->sg_link_size,
>> +                       aml_desc->sg_link,
>> +                       aml_desc->sg_link_phys);
> [Severity: High]
> Does this code sleep in atomic context?
> 
> When the virtual DMA descriptor is freed, vchan_dma_desc_free_list() calls
> this callback from within vchan_complete(), which runs in tasklet
> (softirq) context. Calling a sleepable function like dma_free_coherent()
> from a tasklet can result in a scheduling while atomic panic.
> 
> Could this driver use dma_pool_alloc/free or kzalloc/kfree for descriptor
> management instead?
> 

Use workqueue to release resources.

>> +     kfree(aml_desc);
>> +}
> [ ... ]
> 
>> +/* DMA transfer state  update how many data reside it */
>> +static enum dma_status aml_dma_tx_status(struct dma_chan *chan,
>> +                                      dma_cookie_t cookie,
>> +                                      struct dma_tx_state *txstate)
>> +{
>> +     struct aml_dma_chan *aml_chan = to_aml_dma_chan(chan);
>> +     struct aml_dma_dev *aml_dma = aml_chan->aml_dma;
>> +     struct aml_dma_desc *aml_desc = NULL;
>> +     struct virt_dma_desc *vd;
>> +     u32 residue = 0, done;
>> +     unsigned long flags;
>> +     enum dma_status ret;
>> +
>> +     ret = dma_cookie_status(chan, cookie, txstate);
>> +     if (ret == DMA_COMPLETE || !txstate)
>> +             return ret;
>> +
>> +     spin_lock_irqsave(&aml_chan->vchan.lock, flags);
>> +     vd = vchan_find_desc(&aml_chan->vchan, cookie);
>> +     if (vd) {
>> +             aml_desc = to_aml_dma_desc(vd);
>> +             residue = aml_desc->data_len;
>> +     } else if (aml_chan->cur_desc && aml_chan->cur_desc->vd.tx.cookie == cookie) {
>> +             aml_desc = aml_chan->cur_desc;
>> +             regmap_read(aml_dma->regmap, aml_chan->reg_offs + RCH_RD_LEN, &done);
>> +             residue = aml_desc->data_len - done;
>> +     } else {
>> +             dev_err(aml_dma->dma_device.dev, "cookie error\n");
> [Severity: Medium]
> Will this incorrectly handle unissued descriptors?
> 
> Looking at aml_dma_tx_status(), it attempts to locate a descriptor using
> vchan_find_desc(), 
> 
> If a client queries the status of a descriptor that was submitted (residing
> in desc_submitted) but not yet issued via issue_pending(), vchan_find_desc()
> will return NULL.
> 
> This causes the driver to log a spurious "cookie error" and return a residue
> of 0, which might incorrectly imply the pending transfer is complete.
> 

If it is not found desc, it indicates that the client's callback has 
been invoked, and then he should not come to query status.

>> +     }
>> +     spin_unlock_irqrestore(&aml_chan->vchan.lock, flags);
> [ ... ]
> 
>> +static struct dma_async_tx_descriptor *aml_dma_prep_slave_sg
>> +             (struct dma_chan *chan, struct scatterlist *sgl,
>> +             unsigned int sg_len, enum dma_transfer_direction direction,
>> +             unsigned long flags, void *context)
>> +{
>> +     struct aml_dma_chan *aml_chan = to_aml_dma_chan(chan);
>> +     struct aml_dma_dev *aml_dma = aml_chan->aml_dma;
>> +     struct aml_dma_desc *aml_desc = NULL;
>> +     struct aml_dma_sg_link *sg_link = NULL;
>> +     struct scatterlist *sg = NULL;
>> +     u64 paddr;
>> +     u32 link_count, avail;
>> +     u32 i;
> [ ... ]
> 
>> +     sg_link = aml_desc->sg_link;
>> +     for_each_sg(sgl, sg, sg_len, i) {
>> +             avail = sg_dma_len(sg);
>> +             paddr = sg->dma_address;
>> +             while (avail > SG_MAX_LEN) {
>> +                     /* set dma address and len to sglink*/
>> +                     sg_link->addr_low = lower_32_bits(paddr);
>> +                     sg_link->addr_high = upper_32_bits(paddr);
>> +                     sg_link->ctl = FIELD_PREP(LINK_LEN, SG_MAX_LEN);
>> +                     paddr = paddr + SG_MAX_LEN;
>> +                     avail = avail - SG_MAX_LEN;
>> +                     sg_link++;
>> +             }
>> +             /* set dma address and len to sglink*/
>> +             sg_link->addr_low = lower_32_bits(paddr);
>> +             sg_link->addr_high = upper_32_bits(paddr);
>> +             sg_link->ctl = FIELD_PREP(LINK_LEN, avail);
>> +
>> +             aml_desc->data_len += sg_dma_len(sg);
>> +             sg_link++;
> [Severity: High]
> Can this write out of bounds for zero-length scatterlist elements?
> 
> Earlier in aml_dma_prep_slave_sg(), sg_nents_for_dma() calculates the
> required number of link descriptors. For an SG element with length 0, it
> computes DIV_ROUND_UP(0, SG_MAX_LEN), adding 0 to the link count.
> 
> However, this for_each_sg loop unconditionally consumes at least one link
> slot per SG element, setting its properties and incrementing sg_link.
> 
> If a client provides an empty SG element, this mismatch might result in an
> out-of-bounds write past the end of the dma_alloc_coherent array, leading
> to memory corruption.

Zero-length processing will be skipped in loop.

>> +     }
> [ ... ]
> 
>> +static int aml_dma_chan_pause(struct dma_chan *chan)
>> +{
>> +     struct aml_dma_chan *aml_chan = to_aml_dma_chan(chan);
>> +     struct aml_dma_dev *aml_dma = aml_chan->aml_dma;
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&aml_chan->vchan.lock, flags);
>> +     regmap_set_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_PAUSE);
>> +     aml_chan->pre_status = aml_chan->status;
>> +     aml_chan->status = DMA_PAUSED;
> [Severity: High]
> Is the pause and resume state tracking prone to corruption?
> 
> If aml_dma_chan_pause() is called twice, it overwrites pre_status with
> DMA_PAUSED, which might brick the channel upon resume.
> 
> Also, if a client submits a new transaction while paused,
> aml_dma_chan_resume() restores DMA_COMPLETE but doesn't call
> aml_dma_start(), which could permanently stall the channel.
> 

NO, I don't think the driver will call pause multiple times.

>> +     spin_unlock_irqrestore(&aml_chan->vchan.lock, flags);
>> +
>> +     return 0;
>> +}
> [ ... ]
> 
>> +static int aml_dma_terminate_all(struct dma_chan *chan)
>> +{
>> +     struct aml_dma_chan *aml_chan = to_aml_dma_chan(chan);
>> +     struct aml_dma_dev *aml_dma = aml_chan->aml_dma;
>> +     int chan_id = aml_chan->chan_id;
>> +     struct virt_dma_desc *cur_vd;
>> +     unsigned long flags;
>> +     LIST_HEAD(head);
>> +
>> +     spin_lock_irqsave(&aml_chan->vchan.lock, flags);
> [Severity: Medium]
> Will the channel stall permanently if terminated after errors?
> 
> When a channel hits 6 hardware errors, aml_dma_start() halts the channel
> and abandons pending processing. To recover, a client driver typically calls
> dmaengine_terminate_all().
> 
> However, aml_dma_terminate_all() doesn't reset aml_chan->status to
> DMA_COMPLETE or aml_chan->err_num to 0. Any subsequent transactions
> submitted to this channel might hang forever because aml_dma_start() will
> immediately return due to the persistent error state.
> 

Will add reset reset aml_chan->status to DMA_COMPLETE.

>> +     regmap_set_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_PAUSE);
> [ ... ]
> 
>> +static void aml_dma_start(struct aml_dma_chan *aml_chan)
>> +{
>> +     struct virt_dma_desc *vd = vchan_next_desc(&aml_chan->vchan);
>> +     struct aml_dma_dev *aml_dma = aml_chan->aml_dma;
>> +     struct aml_dma_desc *aml_desc = NULL;
>> +     int chan_id = aml_chan->chan_id;
> [ ... ]
> 
>> +     if (aml_chan->direction == DMA_MEM_TO_DEV) {
>> +             regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_ADDR,
>> +                          lower_32_bits(aml_desc->sg_link_phys));
>> +             regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_ADDR_HIGH,
>> +                          upper_32_bits(aml_desc->sg_link_phys));
> [Severity: Medium]
> Are the correct DMA address registers being used for 64-bit addresses?
> 
> The hardware registers define RCH_ADDR (0xc), RCH_ADDR_LOW (0x20), and
> RCH_ADDR_HIGH (0x24).
> 
> In aml_dma_start(), the lower 32 bits of the physical address are programmed
> into the legacy 32-bit RCH_ADDR (0xc) rather than RCH_ADDR_LOW (0x20), while
> writing the upper 32 bits to RCH_ADDR_HIGH (0x24).
> 
> If the hardware expects the lower bits of a 64-bit address at 0x20, this
> might result in an invalid address and immediate DMA failures.
> 

These two registers are equivalent.

>> +             regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_LEN, aml_desc->data_len);
> [ ... ]
> 
>> +static irqreturn_t aml_dma_interrupt_handler(int irq, void *dev_id)
>> +{
>> +     struct aml_dma_dev *aml_dma = dev_id;
>> +     struct aml_dma_chan *aml_chan;
>> +     struct aml_dma_desc *aml_desc;
>> +     u32 done, eoc_done, err, err_l, end;
>> +     u32 cpl_data;
>> +     int i = 0;
> [ ... ]
> 
>> +     while (done) {
>> +             i = ffs(done) - 1;
>> +             regmap_write(aml_dma->regmap, CLEAR_RCH, BIT(i));
>> +             done &= ~BIT(i);
>> +             aml_chan = aml_dma->aml_rch[i];
>> +             if (!aml_chan) {
>> +                     dev_err(aml_dma->dma_device.dev, "idx %d rch not initialized\n", i);
>> +                     continue;
>> +             }
>> +             spin_lock(&aml_chan->vchan.lock);
>> +             aml_chan->status = (err & BIT(i)) ? DMA_ERROR : DMA_COMPLETE;
> [Severity: High]
> Can an interrupt overwrite the paused state?
> 
> If an interrupt fires while the channel is paused, this handler unconditionally
> overwrites status to DMA_COMPLETE and calls aml_dma_start(). Could this unpause
> the hardware and break the pause semantics?
> 

When paused, DMA will not work and no interruption will occur. So this 
situation will not occur.

>> +             aml_desc = aml_chan->cur_desc;
> [ ... ]
> 
>> +static int aml_dma_probe(struct platform_device *pdev)
>> +{
>> +     struct device_node *np = pdev->dev.of_node;
>> +     struct dma_device *dma_dev;
>> +     struct aml_dma_dev *aml_dma;
>> +     int ret, i, len;
>> +     u32 chan_nr;
> [ ... ]
> 
>> +     ret = devm_request_irq(&pdev->dev, aml_dma->irq, aml_dma_interrupt_handler,
>> +                            0, dev_name(&pdev->dev), aml_dma);
> [Severity: High]
> Can this cause a use-after-free during driver removal?
> 
> Since devm_request_irq() is used in aml_dma_probe(), the IRQ remains active
> until after aml_dma_remove() completes.
> 
> In aml_dma_remove(), the driver loops and calls tasklet_kill(&aml_chan->vchan.task):
> 

NO, when module remove, mask all irq.

>> +static void aml_dma_remove(struct platform_device *pdev)
>> +{
>> +     struct aml_dma_dev *aml_dma = platform_get_drvdata(pdev);
>> +     struct aml_dma_chan *aml_chan = NULL;
>> +     int i;
>> +
>> +     of_dma_controller_free((&pdev->dev)->of_node);
>> +
>> +     regmap_write(aml_dma->regmap, RCH_INT_MASK, 0xffffffff);
>> +     regmap_write(aml_dma->regmap, WCH_INT_MASK, 0xffffffff);
>> +
>> +     for (i = 0; i < MAX_CHAN_ID; i++) {
>> +             aml_chan = aml_dma->aml_rch[i];
>> +             if (aml_chan)
>> +                     tasklet_kill(&aml_chan->vchan.task);
> But an executing interrupt handler on another CPU can subsequently call
> vchan_cookie_complete() and reschedule the tasklet.
> 
> Once remove() returns, devres unregisters the DMA engine, frees the IRQ, and
> frees the driver struct via devm_kfree. The tasklet will then run and access
> freed memory, causing a use-after-free panic.
> 
> Should the driver explicitly free or disable the IRQ using devm_free_irq()
> before calling tasklet_kill()?

Yes it had  mask all irq before call tasklet_kill.

> 
>> +             aml_chan = aml_dma->aml_wch[i];
>> +             if (aml_chan)
>> +                     tasklet_kill(&aml_chan->vchan.task);
>> +     }
>> +}
> --
> Sashiko AI review ·https://sashiko.dev/#/patchset/[email protected]?part=2

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic