[PATCH v9 2/3] dmaengine: amlogic: Add general DMA driver for A9
Xianwei Zhao
xianwei.zhao at amlogic.com
Thu Jul 9 01:11:10 PDT 2026
On 2026/6/26 13:53, sashiko-bot at kernel.org 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<xianwei.zhao at amlogic.com>
>
> 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/20260626-amlogic-dma-v9-0-558d672c4a95@amlogic.com?part=2
More information about the linux-amlogic
mailing list