[PATCH v8 0/3] Add Amlogic general DMA

[PATCH v8 0/3] Add Amlogic general DMA

[Xianwei Zhao via B4 Relay]

Add DMA driver and bindigns for the Amlogic SoCs.

Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
Changes in v8:
- Use kzalloc instead of kmalloc.
- Initialize the temporary variable and fix a spelling mistake.
- Link to v7: https://lore.kernel.org/r/20260324-amlogic-dma-v7-0-f8b91ee192c1@amlogic.com

Changes in v7:
- Take use vchan to support mltiple txns.
- Link to v6: https://lore.kernel.org/r/20260309-amlogic-dma-v6-0-63349d23bd4b@amlogic.com

Changes in v6:
- Some minor modifications according to Frank's suggestion.
- Link to v5: https://lore.kernel.org/r/20260304-amlogic-dma-v5-0-aa453d14fd43@amlogic.com

Changes in v5:
- Rename head file and rename macro definition.
- Rename the subject in [2/3] from "dma" to "dmaengine".
- Link to v4: https://lore.kernel.org/r/20260227-amlogic-dma-v4-0-f25e4614e9b7@amlogic.com

Changes in v4:
- Support split transfer when data len > MAX_LEN.
- When a module fails or exits, perform de-initialization.
- Some other minor modifications.
- Link to v3: https://lore.kernel.org/r/20260206-amlogic-dma-v3-0-56fb9f59ed22@amlogic.com

Changes in v3:
- Adjust the format of binding according to Frank's suggestion.
- Some code format modified according to Frank's suggestion.
- Support one prep_sg and one submit, drop multi prep_sg and one submit.
- Keep pre state when resume from pause status.
- Link to v2: https://lore.kernel.org/r/20260127-amlogic-dma-v2-0-4525d327d74d@amlogic.com

Changes in v2:
- Introduce what the DMA is used for in the A9 SoC.
- Some minor modifications were made according to Krzysztof's suggestions.
- Some modifications were made according to Neil's suggestions.
- Fix a build error.
- Link to v1: https://lore.kernel.org/r/20251216-amlogic-dma-v1-0-e289e57e96a7@amlogic.com

---
Xianwei Zhao (3):
      dt-bindings: dma: Add Amlogic A9 SoC DMA
      dmaengine: amlogic: Add general DMA driver for A9
      MAINTAINERS: Add an entry for Amlogic DMA driver

 .../devicetree/bindings/dma/amlogic,a9-dma.yaml    |  65 ++
 MAINTAINERS                                        |   7 +
 drivers/dma/Kconfig                                |  10 +
 drivers/dma/Makefile                               |   1 +
 drivers/dma/amlogic-dma.c                          | 682 +++++++++++++++++++++
 include/dt-bindings/dma/amlogic,a9-dma.h           |   8 +
 6 files changed, 773 insertions(+)
---
base-commit: 0b1f98df9cf024e9f1a43e0ef9c16d3466d17746
change-id: 20251215-amlogic-dma-79477d5cd264

Best regards,
-- 
Xianwei Zhao <xianwei.zhao@amlogic.com>

[PATCH v8 1/3] dt-bindings: dma: Add Amlogic A9 SoC DMA

[Xianwei Zhao via B4 Relay]

From: Xianwei Zhao <xianwei.zhao@amlogic.com>

Add documentation describing the Amlogic A9 SoC DMA. And add
the properties specific values defines into a new include file.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
 .../devicetree/bindings/dma/amlogic,a9-dma.yaml    | 65 ++++++++++++++++++++++
 include/dt-bindings/dma/amlogic,a9-dma.h           |  8 +++
 2 files changed, 73 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/amlogic,a9-dma.yaml b/Documentation/devicetree/bindings/dma/amlogic,a9-dma.yaml
new file mode 100644
index 000000000000..efd7b2602c33
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/amlogic,a9-dma.yaml
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/amlogic,a9-dma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic general DMA controller
+
+description:
+  This is a general-purpose peripheral DMA controller. It currently supports
+  major peripherals including I2C, I3C, PIO, and CAN-BUS. Transmit and receive
+  for the same peripheral use two separate channels, controlled by different
+  register sets. I2C and I3C transfer data in 1-byte units, while PIO and
+  CAN-BUS transfer data in 4-byte units. From the controller’s perspective,
+  there is no significant difference.
+
+maintainers:
+  - Xianwei Zhao <xianwei.zhao@amlogic.com>
+
+properties:
+  compatible:
+    const: amlogic,a9-dma
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: sys
+
+  '#dma-cells':
+    const: 2
+
+  dma-channels:
+    maximum: 64
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - '#dma-cells'
+  - dma-channels
+
+allOf:
+  - $ref: dma-controller.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    dma-controller@fe400000{
+        compatible = "amlogic,a9-dma";
+        reg = <0xfe400000 0x4000>;
+        interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>;
+        clocks = <&clkc 45>;
+        #dma-cells = <2>;
+        dma-channels = <28>;
+    };
diff --git a/include/dt-bindings/dma/amlogic,a9-dma.h b/include/dt-bindings/dma/amlogic,a9-dma.h
new file mode 100644
index 000000000000..c59c2fd4b956
--- /dev/null
+++ b/include/dt-bindings/dma/amlogic,a9-dma.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+
+#ifndef __DT_BINDINGS_DMA_AMLOGIC_DMA_H__
+#define __DT_BINDINGS_DMA_AMLOGIC_DMA_H__
+
+#define DMA_TX			0
+#define DMA_RX			1
+#endif /* __DT_BINDINGS_DMA_AMLOGIC_DMA_H__ */

-- 
2.52.0

Re: [PATCH v8 1/3] dt-bindings: dma: Add Amlogic A9 SoC DMA

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The macros DMA_TX and DMA_RX are defined in a global header without any vendor or subsystem prefix.
- [Low] The binding specifies `#dma-cells: const: 2` but fails to document the semantics of the two cells.
--

commit 7a02c7e6145050221d77d7a4745ee4d4e651163b
Author: Xianwei Zhao <xianwei.zhao@amlogic.com>

dt-bindings: dma: Add Amlogic A9 SoC DMA

This commit adds device tree binding documentation for the Amlogic A9 SoC
general-purpose DMA controller. It also introduces a new dt-bindings header
file with property values for the DMA transfer directions.

> diff --git a/Documentation/devicetree/bindings/dma/amlogic,a9-dma.yaml b/Documentation/devicetree/bindings/dma/amlogic,a9-dma.yaml
> new file mode 100644
> index 0000000000000..efd7b2602c336
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/amlogic,a9-dma.yaml
[ ... ]
> +properties:
> +  compatible:
> +    const: amlogic,a9-dma
[ ... ]
> +  clock-names:
> +    const: sys
> +
> +  '#dma-cells':
> +    const: 2

[Severity: Low]
Should this include a description explaining what the two cells represent?

Usually bindings with multiple cells document the semantics of each cell
(e.g., first cell for the request line, second for transfer direction) so
clients know how to construct their dmas specifier.

[ ... ]
> diff --git a/include/dt-bindings/dma/amlogic,a9-dma.h b/include/dt-bindings/dma/amlogic,a9-dma.h
> new file mode 100644
> index 0000000000000..c59c2fd4b9561
> --- /dev/null
> +++ b/include/dt-bindings/dma/amlogic,a9-dma.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +
> +#ifndef __DT_BINDINGS_DMA_AMLOGIC_DMA_H__
> +#define __DT_BINDINGS_DMA_AMLOGIC_DMA_H__
> +
> +#define DMA_TX			0
> +#define DMA_RX			1

[Severity: Low]
Would it be safer to add a vendor or device prefix to these macros?

Since this header is globally accessible and can be included in both device
tree sources and driver code, generic names like DMA_TX and DMA_RX might
cause macro redefinition conflicts with other kernel components. Adding a
prefix like AMLOGIC_A9_ would avoid this.

> +#endif /* __DT_BINDINGS_DMA_AMLOGIC_DMA_H__ */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-amlogic-dma-v8-0-86cc2ce94142@amlogic.com?part=1

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

[Xianwei Zhao via B4 Relay]

From: Xianwei Zhao <xianwei.zhao@amlogic.com>

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.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
 drivers/dma/Kconfig       |  10 +
 drivers/dma/Makefile      |   1 +
 drivers/dma/amlogic-dma.c | 682 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 693 insertions(+)

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index e98e3e8c5036..400ae0f11f89 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -85,6 +85,16 @@ config AMCC_PPC440SPE_ADMA
 	help
 	  Enable support for the AMCC PPC440SPe RAID engines.
 
+config AMLOGIC_DMA
+	tristate "Amlogic general DMA support"
+	depends on ARCH_MESON || COMPILE_TEST
+	select DMA_ENGINE
+	select DMA_VIRTUAL_CHANNELS
+	select REGMAP_MMIO
+	help
+	  Enable support for the Amlogic general DMA engines. THis DMA
+	  controller is used some Amlogic SoCs, such as A9.
+
 config APPLE_ADMAC
 	tristate "Apple ADMAC support"
 	depends on ARCH_APPLE || COMPILE_TEST
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index df566c4958b6..8457383281a8 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_DMATEST) += dmatest.o
 obj-$(CONFIG_ALTERA_MSGDMA) += altera-msgdma.o
 obj-$(CONFIG_AMBA_PL08X) += amba-pl08x.o
 obj-$(CONFIG_AMCC_PPC440SPE_ADMA) += ppc4xx/
+obj-$(CONFIG_AMLOGIC_DMA) += amlogic-dma.o
 obj-$(CONFIG_APPLE_ADMAC) += apple-admac.o
 obj-$(CONFIG_ARM_DMA350) += arm-dma350.o
 obj-$(CONFIG_AT_HDMAC) += at_hdmac.o
diff --git a/drivers/dma/amlogic-dma.c b/drivers/dma/amlogic-dma.c
new file mode 100644
index 000000000000..7dae5ba15c7e
--- /dev/null
+++ b/drivers/dma/amlogic-dma.c
@@ -0,0 +1,682 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
+/*
+ * Copyright (C) 2025 Amlogic, Inc. All rights reserved
+ * Author: Xianwei Zhao <xianwei.zhao@amlogic.com>
+ */
+
+#include <dt-bindings/dma/amlogic,a9-dma.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/interrupt.h>
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_dma.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include "virt-dma.h"
+
+#define RCH_REG_BASE		0x0
+#define WCH_REG_BASE		0x2000
+/*
+ * Each rch (read from memory) REG offset  Rch_offset 0x0 each channel total 0x40
+ * rch addr = DMA_base + Rch_offset+ chan_id * 0x40 + reg_offset
+ */
+#define RCH_READY		0x0
+#define RCH_STATUS		0x4
+#define RCH_CFG			0x8
+#define CFG_CLEAR		BIT(25)
+#define CFG_PAUSE		BIT(26)
+#define CFG_ENABLE		BIT(27)
+#define CFG_DONE		BIT(28)
+#define RCH_ADDR		0xc
+#define RCH_LEN			0x10
+#define RCH_RD_LEN		0x14
+#define RCH_PRT			0x18
+#define RCH_SYCN_STAT		0x1c
+#define RCH_ADDR_LOW		0x20
+#define RCH_ADDR_HIGH		0x24
+/* if work on 64, it work with RCH_PRT */
+#define RCH_PTR_HIGH		0x28
+
+/*
+ * Each wch (write to memory) REG offset  Wch_offset 0x2000 each channel total 0x40
+ * wch addr = DMA_base + Wch_offset+ chan_id * 0x40 + reg_offset
+ */
+#define WCH_READY		0x0
+#define WCH_TOTAL_LEN		0x4
+#define WCH_CFG			0x8
+#define WCH_ADDR		0xc
+#define WCH_LEN			0x10
+#define WCH_RD_LEN		0x14
+#define WCH_PRT			0x18
+#define WCH_CMD_CNT		0x1c
+#define WCH_ADDR_LOW		0x20
+#define WCH_ADDR_HIGH		0x24
+/* if work on 64, it work with RCH_PRT */
+#define WCH_PTR_HIGH		0x28
+
+/* DMA controller reg */
+#define RCH_INT_MASK		0x1000
+#define WCH_INT_MASK		0x1004
+#define CLEAR_W_BATCH		0x1014
+#define CLEAR_RCH		0x1024
+#define CLEAR_WCH		0x1028
+#define RCH_ACTIVE		0x1038
+#define WCH_ACTIVE		0x103c
+#define RCH_DONE		0x104c
+#define WCH_DONE		0x1050
+#define RCH_ERR			0x1060
+#define RCH_LEN_ERR		0x1064
+#define WCH_ERR			0x1068
+#define DMA_BATCH_END		0x1078
+#define WCH_EOC_DONE		0x1088
+#define WDMA_RESP_ERR		0x1098
+#define UPT_PKT_SYNC		0x10a8
+#define RCHN_CFG		0x10ac
+#define WCHN_CFG		0x10b0
+#define MEM_PD_CFG		0x10b4
+#define MEM_BUS_CFG		0x10b8
+#define DMA_GMV_CFG		0x10bc
+#define DMA_GMR_CFG		0x10c0
+
+#define MAX_CHAN_ID		32
+#define SG_MAX_LEN		GENMASK(26, 0)
+
+struct aml_dma_sg_link {
+#define LINK_LEN		GENMASK(26, 0)
+#define LINK_IRQ		BIT(27)
+#define LINK_EOC		BIT(28)
+#define LINK_LOOP		BIT(29)
+#define LINK_ERR		BIT(30)
+#define LINK_OWNER		BIT(31)
+	u32 ctl;
+	u64 address;
+	u32 revered;
+} __packed;
+
+/* 1 page for link 256*16 */
+#define DMA_MAX_LINK		256
+/* sizeof(struct aml_dma_sg_link) */
+#define DMA_LINK_SIZE		16
+#define DMA_LINK_MAX_SIZE	(DMA_LINK_SIZE * DMA_MAX_LINK)
+
+struct aml_dma_desc {
+	struct virt_dma_desc		vd;
+	int				idx;
+	int				data_len;
+};
+
+struct aml_dma_chan {
+	struct virt_dma_chan		vchan;
+	struct aml_dma_dev		*aml_dma;
+	struct aml_dma_desc		*cur_desc;
+	struct aml_dma_sg_link		*sg_link;
+	dma_addr_t			sg_link_phys;
+	int				idx_next;
+	enum dma_status			pre_status;
+	enum dma_status			status;
+	enum dma_transfer_direction	direction;
+	int				chan_id;
+	/* reg_base (direction + chan_id) */
+	int				reg_offs;
+};
+
+struct aml_dma_dev {
+	struct dma_device		dma_device;
+	void __iomem			*base;
+	struct regmap			*regmap;
+	struct clk			*clk;
+	int				irq;
+	struct platform_device		*pdev;
+	struct aml_dma_chan		*aml_rch[MAX_CHAN_ID];
+	struct aml_dma_chan		*aml_wch[MAX_CHAN_ID];
+	unsigned int			chan_nr;
+	unsigned int			chan_used;
+	struct aml_dma_chan		aml_chans[]__counted_by(chan_nr);
+};
+
+static inline struct aml_dma_chan *to_aml_dma_chan(struct dma_chan *chan)
+{
+	return container_of(chan, struct aml_dma_chan, vchan.chan);
+}
+
+static inline struct aml_dma_desc *to_aml_dma_desc(struct virt_dma_desc *vd)
+{
+	return container_of(vd, struct aml_dma_desc, vd);
+}
+
+static void aml_dma_free_desc(struct virt_dma_desc *vd)
+{
+	struct aml_dma_desc *aml_desc = to_aml_dma_desc(vd);
+
+	kfree(aml_desc);
+}
+
+static int aml_dma_alloc_chan_resources(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;
+
+	/* offset is the same RCH_CFG and WCH_CFG */
+	regmap_set_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_CLEAR);
+	regmap_clear_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_PAUSE);
+	regmap_clear_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_CLEAR);
+	aml_chan->sg_link = dma_alloc_coherent(aml_dma->dma_device.dev, DMA_LINK_MAX_SIZE,
+					       &aml_chan->sg_link_phys, GFP_KERNEL);
+	if (!aml_chan->sg_link)
+		return -ENOMEM;
+	aml_chan->idx_next = 0;
+	aml_chan->status = DMA_COMPLETE;
+	aml_chan->cur_desc = NULL;
+
+	return 0;
+}
+
+static void aml_dma_free_chan_resources(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;
+	struct virt_dma_desc *cur_vd = NULL;
+	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);
+	regmap_set_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_CLEAR);
+	if (aml_chan->cur_desc)
+		cur_vd = &aml_chan->cur_desc->vd;
+	aml_chan->cur_desc = NULL;
+	spin_unlock_irqrestore(&aml_chan->vchan.lock, flags);
+	if (cur_vd)
+		aml_dma_free_desc(cur_vd);
+
+	vchan_free_chan_resources(&aml_chan->vchan);
+
+	dma_free_coherent(aml_dma->dma_device.dev,
+			  DMA_LINK_MAX_SIZE, aml_chan->sg_link,
+			  aml_chan->sg_link_phys);
+}
+
+/* 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)
+		return ret;
+
+	if (aml_chan->status != DMA_COMPLETE)
+		ret = aml_chan->status;
+	if (!txstate)
+		return ret;
+
+	spin_lock_irqsave(&aml_chan->vchan.lock, flags);
+	vd = vchan_find_desc(&aml_chan->vchan, cookie);
+	if (vd) {
+		list_for_each_entry(vd, &aml_chan->vchan.desc_issued, node) {
+			aml_desc = to_aml_dma_desc(vd);
+			residue += aml_desc->data_len;
+			if (vd->tx.cookie == cookie)
+				break;
+		}
+		aml_desc = aml_chan->cur_desc;
+		if (aml_desc) {
+			regmap_read(aml_dma->regmap, aml_chan->reg_offs + RCH_RD_LEN, &done);
+			residue += aml_desc->data_len - done;
+		}
+	} 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 {
+		residue = 0;
+	}
+	spin_unlock_irqrestore(&aml_chan->vchan.lock, flags);
+	dma_set_residue(txstate, residue);
+
+	return ret;
+}
+
+static int find_dma_chan_link(struct aml_dma_chan *aml_chan, u32 num)
+{
+	int idx;
+	unsigned long flags;
+
+	spin_lock_irqsave(&aml_chan->vchan.lock, flags);
+	if ((aml_chan->idx_next + num) >= DMA_MAX_LINK)
+		idx = 0;
+	else
+		idx = aml_chan->idx_next;
+	aml_chan->idx_next = idx + num;
+	spin_unlock_irqrestore(&aml_chan->vchan.lock, flags);
+
+	return idx;
+}
+
+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;
+
+	if (aml_chan->direction != direction) {
+		dev_err(aml_dma->dma_device.dev, "direction not support\n");
+		return NULL;
+	}
+
+	aml_desc = kzalloc(sizeof(*aml_desc), GFP_KERNEL);
+	if (!aml_desc)
+		return NULL;
+	link_count = sg_nents_for_dma(sgl, sg_len, SG_MAX_LEN);
+	aml_desc->idx = find_dma_chan_link(aml_chan, link_count);
+	sg_link = aml_chan->sg_link + aml_desc->idx;
+	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->address = 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->address = paddr;
+		sg_link->ctl = FIELD_PREP(LINK_LEN, avail);
+
+		aml_desc->data_len += sg_dma_len(sg);
+		sg_link++;
+	}
+
+	/* the last sg set eoc flag */
+	sg_link--;
+	sg_link->ctl |= LINK_EOC;
+
+	return vchan_tx_prep(&aml_chan->vchan, &aml_desc->vd, flags);
+}
+
+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;
+
+	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;
+
+	return 0;
+}
+
+static int aml_dma_chan_resume(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;
+
+	regmap_clear_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_PAUSE);
+	aml_chan->status = aml_chan->pre_status;
+
+	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);
+	regmap_set_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_PAUSE);
+	regmap_set_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_CLEAR);
+
+	if (aml_chan->direction == DMA_MEM_TO_DEV)
+		regmap_set_bits(aml_dma->regmap, RCH_INT_MASK, BIT(chan_id));
+	else if (aml_chan->direction == DMA_DEV_TO_MEM)
+		regmap_set_bits(aml_dma->regmap, WCH_INT_MASK, BIT(chan_id));
+
+	vchan_get_all_descriptors(&aml_chan->vchan, &head);
+	cur_vd = &aml_chan->cur_desc->vd;
+	aml_chan->cur_desc = NULL;
+	spin_unlock_irqrestore(&aml_chan->vchan.lock, flags);
+	if (cur_vd)
+		aml_dma_free_desc(cur_vd);
+
+	vchan_dma_desc_free_list(&aml_chan->vchan, &head);
+
+	return 0;
+}
+
+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 (!vd)
+		return;
+	if (aml_chan->status != DMA_COMPLETE)
+		return;
+
+	list_del(&vd->node);
+	aml_desc = to_aml_dma_desc(vd);
+	aml_chan->cur_desc = aml_desc;
+
+	if (aml_chan->direction == DMA_MEM_TO_DEV) {
+		regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_ADDR,
+			     (aml_chan->sg_link_phys + aml_desc->idx * DMA_LINK_SIZE));
+		regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_LEN, aml_desc->data_len);
+		regmap_clear_bits(aml_dma->regmap, RCH_INT_MASK, BIT(chan_id));
+		/* for rch (tx) need set cfg 0 to trigger start */
+		regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, 0);
+	} else if (aml_chan->direction == DMA_DEV_TO_MEM) {
+		regmap_write(aml_dma->regmap, aml_chan->reg_offs + WCH_ADDR,
+			     (aml_chan->sg_link_phys + aml_desc->idx * DMA_LINK_SIZE));
+		regmap_write(aml_dma->regmap, aml_chan->reg_offs + WCH_LEN, aml_desc->data_len);
+		regmap_clear_bits(aml_dma->regmap, WCH_INT_MASK, BIT(chan_id));
+	}
+}
+
+static void aml_dma_issue_pending(struct dma_chan *chan)
+{
+	struct aml_dma_chan *aml_chan = to_aml_dma_chan(chan);
+	unsigned long flags;
+
+	spin_lock_irqsave(&aml_chan->vchan.lock, flags);
+	if (vchan_issue_pending(&aml_chan->vchan) && !aml_chan->cur_desc)
+		aml_dma_start(aml_chan);
+	spin_unlock_irqrestore(&aml_chan->vchan.lock, flags);
+}
+
+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;
+
+	/* deal with rch normal complete and error */
+	regmap_read(aml_dma->regmap, RCH_DONE, &done);
+	regmap_read(aml_dma->regmap, RCH_ERR, &err);
+	regmap_read(aml_dma->regmap, RCH_LEN_ERR, &err_l);
+	err = err | err_l;
+
+	done = done | err;
+
+	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;
+		}
+		aml_chan->status = (err & (1 << i)) ? DMA_ERROR : DMA_COMPLETE;
+		spin_lock(&aml_chan->vchan.lock);
+		aml_desc = aml_chan->cur_desc;
+		if (aml_chan->status == DMA_ERROR) {
+			aml_desc->vd.tx_result.result = DMA_TRANS_READ_FAILED;
+			regmap_read(aml_dma->regmap, aml_chan->reg_offs + RCH_RD_LEN, &cpl_data);
+			aml_desc->vd.tx_result.residue = aml_desc->data_len - cpl_data;
+		}
+		vchan_cookie_complete(&aml_desc->vd);
+		aml_chan->cur_desc = NULL;
+		aml_dma_start(aml_chan);
+		spin_unlock(&aml_chan->vchan.lock);
+	}
+
+	/* deal with wch normal complete and error */
+	regmap_read(aml_dma->regmap, DMA_BATCH_END, &end);
+	if (end)
+		regmap_write(aml_dma->regmap, CLEAR_W_BATCH, end);
+
+	regmap_read(aml_dma->regmap, WCH_DONE, &done);
+	regmap_read(aml_dma->regmap, WCH_EOC_DONE, &eoc_done);
+	done = done | eoc_done;
+
+	regmap_read(aml_dma->regmap, WCH_ERR, &err);
+	regmap_read(aml_dma->regmap, WDMA_RESP_ERR, &err_l);
+	err = err | err_l;
+
+	done = done | err;
+	i = 0;
+	while (done) {
+		i = ffs(done) - 1;
+		done &= ~BIT(i);
+		regmap_write(aml_dma->regmap, CLEAR_WCH, BIT(i));
+		aml_chan = aml_dma->aml_wch[i];
+		if (!aml_chan) {
+			dev_err(aml_dma->dma_device.dev, "idx %d wch not initialized\n", i);
+			continue;
+		}
+		aml_chan->status = (err & (1 << i)) ? DMA_ERROR : DMA_COMPLETE;
+		spin_lock(&aml_chan->vchan.lock);
+		aml_desc = aml_chan->cur_desc;
+		if (aml_chan->status == DMA_ERROR) {
+			aml_desc->vd.tx_result.result = DMA_TRANS_WRITE_FAILED;
+			regmap_read(aml_dma->regmap, aml_chan->reg_offs + RCH_RD_LEN, &cpl_data);
+			aml_desc->vd.tx_result.residue = aml_desc->data_len - cpl_data;
+		}
+		vchan_cookie_complete(&aml_desc->vd);
+		aml_chan->cur_desc = NULL;
+		aml_dma_start(aml_chan);
+		spin_unlock(&aml_chan->vchan.lock);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static struct dma_chan *aml_of_dma_xlate(struct of_phandle_args *dma_spec, struct of_dma *ofdma)
+{
+	struct aml_dma_dev *aml_dma = (struct aml_dma_dev *)ofdma->of_dma_data;
+	struct aml_dma_chan *aml_chan = NULL;
+	u32 type;
+	u32 phy_chan_id;
+
+	if (dma_spec->args_count != 2)
+		return NULL;
+
+	type = dma_spec->args[0];
+	phy_chan_id = dma_spec->args[1];
+
+	if (phy_chan_id >= MAX_CHAN_ID)
+		return NULL;
+
+	if (type == DMA_TX) {
+		aml_chan = aml_dma->aml_rch[phy_chan_id];
+		if (!aml_chan) {
+			if (aml_dma->chan_used >= aml_dma->chan_nr) {
+				dev_err(aml_dma->dma_device.dev, "some dma clients err used\n");
+				return NULL;
+			}
+			aml_chan = &aml_dma->aml_chans[aml_dma->chan_used];
+			aml_dma->chan_used++;
+			aml_chan->direction = DMA_MEM_TO_DEV;
+			aml_chan->chan_id = phy_chan_id;
+			aml_chan->reg_offs = RCH_REG_BASE + 0x40 * aml_chan->chan_id;
+			aml_dma->aml_rch[phy_chan_id] = aml_chan;
+		}
+	} else if (type == DMA_RX) {
+		aml_chan = aml_dma->aml_wch[phy_chan_id];
+		if (!aml_chan) {
+			if (aml_dma->chan_used >= aml_dma->chan_nr) {
+				dev_err(aml_dma->dma_device.dev, "some dma clients err used\n");
+				return NULL;
+			}
+			aml_chan = &aml_dma->aml_chans[aml_dma->chan_used];
+			aml_dma->chan_used++;
+			aml_chan->direction = DMA_DEV_TO_MEM;
+			aml_chan->chan_id = phy_chan_id;
+			aml_chan->reg_offs = WCH_REG_BASE + 0x40 * aml_chan->chan_id;
+			aml_dma->aml_wch[phy_chan_id] = aml_chan;
+		}
+	} else {
+		dev_err(aml_dma->dma_device.dev, "type %d not supported\n", type);
+		return NULL;
+	}
+
+	return dma_get_slave_channel(&aml_chan->vchan.chan);
+}
+
+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;
+
+	const struct regmap_config aml_regmap_config = {
+		.reg_bits = 32,
+		.val_bits = 32,
+		.reg_stride = 4,
+		.max_register = 0x3000,
+	};
+
+	ret = of_property_read_u32(np, "dma-channels", &chan_nr);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to read dma-channels\n");
+
+	len = sizeof(struct aml_dma_dev) + sizeof(struct aml_dma_chan) * chan_nr;
+	aml_dma = devm_kzalloc(&pdev->dev, len, GFP_KERNEL);
+	if (!aml_dma)
+		return -ENOMEM;
+
+	aml_dma->chan_nr = chan_nr;
+
+	aml_dma->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(aml_dma->base))
+		return PTR_ERR(aml_dma->base);
+
+	aml_dma->regmap = devm_regmap_init_mmio(&pdev->dev, aml_dma->base,
+						&aml_regmap_config);
+	if (IS_ERR_OR_NULL(aml_dma->regmap))
+		return PTR_ERR(aml_dma->regmap);
+
+	aml_dma->clk = devm_clk_get_enabled(&pdev->dev, NULL);
+	if (IS_ERR(aml_dma->clk))
+		return PTR_ERR(aml_dma->clk);
+
+	aml_dma->irq = platform_get_irq(pdev, 0);
+
+	aml_dma->pdev = pdev;
+	aml_dma->dma_device.dev = &pdev->dev;
+
+	dma_dev = &aml_dma->dma_device;
+	INIT_LIST_HEAD(&dma_dev->channels);
+
+	/* Initialize channel parameters */
+	for (i = 0; i < chan_nr; i++) {
+		struct aml_dma_chan *aml_chan = &aml_dma->aml_chans[i];
+
+		aml_chan->aml_dma = aml_dma;
+		aml_chan->vchan.desc_free = aml_dma_free_desc;
+		vchan_init(&aml_chan->vchan, &aml_dma->dma_device);
+	}
+	aml_dma->chan_used = 0;
+
+	dma_set_max_seg_size(dma_dev->dev, SG_MAX_LEN);
+	dma_cap_set(DMA_SLAVE, dma_dev->cap_mask);
+	dma_dev->device_alloc_chan_resources = aml_dma_alloc_chan_resources;
+	dma_dev->device_free_chan_resources = aml_dma_free_chan_resources;
+	dma_dev->device_tx_status = aml_dma_tx_status;
+	dma_dev->device_prep_slave_sg = aml_dma_prep_slave_sg;
+	dma_dev->device_pause = aml_dma_chan_pause;
+	dma_dev->device_resume = aml_dma_chan_resume;
+	dma_dev->device_terminate_all = aml_dma_terminate_all;
+	dma_dev->device_issue_pending = aml_dma_issue_pending;
+	/* PIO 4 bytes and I2C 1 byte */
+	dma_dev->dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) | BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
+	dma_dev->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
+	dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
+
+	ret = dmaenginem_async_device_register(dma_dev);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to register dmaenginem\n");
+
+	ret = of_dma_controller_register(np, aml_of_dma_xlate, aml_dma);
+	if (ret)
+		goto err_dmaenginem_register;
+
+	regmap_write(aml_dma->regmap, RCH_INT_MASK, 0xffffffff);
+	regmap_write(aml_dma->regmap, WCH_INT_MASK, 0xffffffff);
+
+	ret = devm_request_irq(&pdev->dev, aml_dma->irq, aml_dma_interrupt_handler,
+			       IRQF_SHARED, dev_name(&pdev->dev), aml_dma);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request_irq\n");
+		goto err_of_dma_controller;
+	}
+
+	platform_set_drvdata(pdev, aml_dma);
+
+	return 0;
+
+err_of_dma_controller:
+	of_dma_controller_free(np);
+err_dmaenginem_register:
+	dma_async_device_unregister(dma_dev);
+
+	return ret;
+}
+
+static void aml_dma_remove(struct platform_device *pdev)
+{
+	struct aml_dma_dev *aml_dma = platform_get_drvdata(pdev);
+
+	of_dma_controller_free((&pdev->dev)->of_node);
+	dma_async_device_unregister(&aml_dma->dma_device);
+}
+
+static const struct of_device_id aml_dma_ids[] = {
+	{ .compatible = "amlogic,a9-dma", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, aml_dma_ids);
+
+static struct platform_driver aml_dma_driver = {
+	.probe		= aml_dma_probe,
+	.remove		= aml_dma_remove,
+	.driver		= {
+		.name	= "aml-dma",
+		.of_match_table = aml_dma_ids,
+	},
+};
+
+module_platform_driver(aml_dma_driver);
+
+MODULE_DESCRIPTION("GENERAL DMA driver for Amlogic");
+MODULE_AUTHOR("Xianwei Zhao <xianwei.zhao@amlogic.com>");
+MODULE_LICENSE("GPL");

-- 
2.52.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 17 potential issue(s) to consider:
- [Critical] The naive ring buffer allocator overflows on large scatterlists and silently overwrites active in-flight DMA descriptors.
- [High] The DMA engine device is unregistered twice on driver removal and probe error paths.
- [High] The DMA controller is exposed to clients before hardware and interrupts are fully initialized.
- [High] Integer truncation in the calculation of the allocation length for `aml_dma_dev` leads to a heap buffer overflow.
- [High] The driver allocates memory with `GFP_KERNEL` inside a DMA preparation callback, which can sleep in atomic context.
- [High] The driver unconditionally dereferences `cur_desc` in the interrupt handler and `terminate_all`, leading to a kernel panic if the channel is idle.
- [High] A single DMA transfer error permanently deadlocks the channel.
- [High] Calling `aml_dma_prep_slave_sg` with an empty scatterlist results in a buffer underflow write.
- [High] Mismatch between allocated links and written links for zero-length scatterlist elements.
- [High] Missing `tasklet_kill()` during driver removal leads to Use-After-Free.
- [High] For WCH (DEV_TO_MEM) channels, the hardware is never unpaused after `terminate_all()` or `pause()`.
- [High] The 64-bit physical address of the DMA descriptor list is truncated to 32 bits, causing DMA faults.
- [Medium] Incorrect and non-compliant descriptor residue calculation in `aml_dma_tx_status`.
- [Medium] Signed integer overflow due to 32-bit shift in the interrupt handler.
- [Medium] Concurrent modification of `aml_chan->status` without locking causes state corruption.
- [Medium] The shared interrupt handler unconditionally returns `IRQ_HANDLED` even when it processes no events.
- [Medium] Hardware descriptor fields use native endianness and unaligned 64-bit access.
--

commit 6328a62df7a031573dae5a113f1f4a5b08fd7d50
Author: Xianwei Zhao <xianwei.zhao@amlogic.com>
Subject: dmaengine: amlogic: Add general DMA driver for A9

This patch adds a new DMA engine driver for the Amlogic A9 general-purpose
DMA controller. It handles transfer operations using the virt-dma framework
and registers the controller using the dmaengine API.

> diff --git a/drivers/dma/amlogic-dma.c b/drivers/dma/amlogic-dma.c
> new file mode 100644
> index 0000000000000..7dae5ba15c7e7
> --- /dev/null
> +++ b/drivers/dma/amlogic-dma.c
> @@ -0,0 +1,682 @@
[ ... ]
> +struct aml_dma_sg_link {
> +#define LINK_LEN		GENMASK(26, 0)
> +#define LINK_IRQ		BIT(27)
> +#define LINK_EOC		BIT(28)
> +#define LINK_LOOP		BIT(29)
> +#define LINK_ERR		BIT(30)
> +#define LINK_OWNER		BIT(31)
> +	u32 ctl;
> +	u64 address;
> +	u32 revered;
> +} __packed;

[Severity: Medium]
Will this __packed structure cause alignment faults on architectures sensitive
to unaligned memory accesses? The 64-bit address field is forced to an
unaligned 4-byte offset. Are the native types (u32, u64) safe here, or could
they break DMA execution on big-endian kernels?

[ ... ]
> +static enum dma_status aml_dma_tx_status(struct dma_chan *chan,
> +					 dma_cookie_t cookie,
> +					 struct dma_tx_state *txstate)
> +{
[ ... ]
> +	spin_lock_irqsave(&aml_chan->vchan.lock, flags);
> +	vd = vchan_find_desc(&aml_chan->vchan, cookie);
> +	if (vd) {
> +		list_for_each_entry(vd, &aml_chan->vchan.desc_issued, node) {
> +			aml_desc = to_aml_dma_desc(vd);
> +			residue += aml_desc->data_len;
> +			if (vd->tx.cookie == cookie)
> +				break;
> +		}

[Severity: Medium]
Does this incorrectly accumulate the data_len of independent descriptors?
Furthermore, since list_for_each_entry() overwrites the loop cursor 'vd', could
this traverse the entire desc_issued list without breaking if the target
cookie resides in desc_submitted, thereby returning an incorrect total?

[ ... ]
> +static int find_dma_chan_link(struct aml_dma_chan *aml_chan, u32 num)
> +{
> +	int idx;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&aml_chan->vchan.lock, flags);
> +	if ((aml_chan->idx_next + num) >= DMA_MAX_LINK)
> +		idx = 0;
> +	else
> +		idx = aml_chan->idx_next;
> +	aml_chan->idx_next = idx + num;
> +	spin_unlock_irqrestore(&aml_chan->vchan.lock, flags);
> +
> +	return idx;
> +}

[Severity: Critical]
Is it possible for 'num' to be larger than DMA_MAX_LINK (256)? If a scatterlist
needs more than 256 links, could idx_next become out of bounds and cause the
descriptor preparation loop to write past the end of the sg_link array? Could
this also silently overwrite active in-flight descriptors without checking if
they are currently being processed?

[ ... ]
> +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)
> +{
[ ... ]
> +	aml_desc = kzalloc(sizeof(*aml_desc), GFP_KERNEL);

[Severity: High]
Can this allocation sleep in atomic context? Dmaengine prep callbacks are
expected to be callable from atomic contexts (e.g. inside interrupt handlers or
while holding spinlocks). Should GFP_NOWAIT or GFP_ATOMIC be used instead?

> +	if (!aml_desc)
> +		return NULL;
> +	link_count = sg_nents_for_dma(sgl, sg_len, SG_MAX_LEN);
> +	aml_desc->idx = find_dma_chan_link(aml_chan, link_count);
> +	sg_link = aml_chan->sg_link + aml_desc->idx;
> +	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->address = 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->address = paddr;
> +		sg_link->ctl = FIELD_PREP(LINK_LEN, avail);
> +
> +		aml_desc->data_len += sg_dma_len(sg);
> +		sg_link++;
> +	}

[Severity: High]
If a scatterlist element has a length of 0, sg_nents_for_dma() will not allocate
a link slot for it. Will this loop unconditionally increment sg_link++ for that
element anyway, causing the driver to consume more link slots than allocated and
corrupt the ring buffer?

> +
> +	/* the last sg set eoc flag */
> +	sg_link--;
> +	sg_link->ctl |= LINK_EOC;

[Severity: High]
If aml_dma_prep_slave_sg() is called with sg_len == 0, the for_each_sg loop
will not execute. Could this cause sg_link-- to point before the allocated
descriptor index, leading to an out-of-bounds underflow write?

[ ... ]
> +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;
> +
> +	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: Medium]
Are these status updates subject to race conditions? aml_chan->status is
modified here without acquiring aml_chan->vchan.lock, while it is also modified
concurrently by the interrupt handler.

[ ... ]
> +static void aml_dma_start(struct aml_dma_chan *aml_chan)
> +{
[ ... ]
> +	if (aml_chan->direction == DMA_MEM_TO_DEV) {
> +		regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_ADDR,
> +			     (aml_chan->sg_link_phys + aml_desc->idx * DMA_LINK_SIZE));

[Severity: High]
Does passing a 64-bit dma_addr_t (sg_link_phys) to regmap_write() silently
truncate the upper 32 bits of the address? If memory is allocated above 4GB,
will this lead to incorrect memory accesses by the DMA controller?

> +		regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_LEN, aml_desc->data_len);
> +		regmap_clear_bits(aml_dma->regmap, RCH_INT_MASK, BIT(chan_id));
> +		/* for rch (tx) need set cfg 0 to trigger start */
> +		regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, 0);
> +	} else if (aml_chan->direction == DMA_DEV_TO_MEM) {
> +		regmap_write(aml_dma->regmap, aml_chan->reg_offs + WCH_ADDR,
> +			     (aml_chan->sg_link_phys + aml_desc->idx * DMA_LINK_SIZE));
> +		regmap_write(aml_dma->regmap, aml_chan->reg_offs + WCH_LEN, aml_desc->data_len);
> +		regmap_clear_bits(aml_dma->regmap, WCH_INT_MASK, BIT(chan_id));
> +	}

[Severity: High]
For WCH (DEV_TO_MEM) channels, aml_dma_start() neglects to write to WCH_CFG.
If aml_dma_terminate_all() or aml_dma_chan_pause() previously set the CFG_PAUSE
bit, will WCH channels remain indefinitely paused?

> +}
[ ... ]
> +static irqreturn_t aml_dma_interrupt_handler(int irq, void *dev_id)
> +{
[ ... ]
> +	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;
> +		}
> +		aml_chan->status = (err & (1 << i)) ? DMA_ERROR : DMA_COMPLETE;

[Severity: Medium]
Could this shift result in a signed integer overflow? If 'i' reaches 31
(since MAX_CHAN_ID is 32), shifting the signed 32-bit literal '1' causes
undefined behavior. Should this use 1U << i or BIT(i) instead?

[Severity: High]
If a hardware error occurs and aml_chan->status is set to DMA_ERROR, is there
any mechanism to reset the status back to DMA_COMPLETE? Without a reset path,
will aml_dma_start() simply return immediately for all future transfers,
permanently deadlocking the channel?

> +		spin_lock(&aml_chan->vchan.lock);
> +		aml_desc = aml_chan->cur_desc;
> +		if (aml_chan->status == DMA_ERROR) {
> +			aml_desc->vd.tx_result.result = DMA_TRANS_READ_FAILED;
> +			regmap_read(aml_dma->regmap, aml_chan->reg_offs + RCH_RD_LEN, &cpl_data);
> +			aml_desc->vd.tx_result.residue = aml_desc->data_len - cpl_data;
> +		}

[Severity: High]
If a spurious interrupt fires, or if it fires concurrently with
aml_dma_terminate_all() (which sets cur_desc to NULL), will aml_desc be NULL
here? This appears to unconditionally dereference aml_desc, potentially causing
a NULL pointer dereference.

[ ... ]
> +	}
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
Because this driver requests a shared interrupt line (IRQF_SHARED), shouldn't
the handler return IRQ_NONE if it processed no events (e.g., done and end
were both 0)? Returning IRQ_HANDLED unconditionally could break shared interrupt
semantics and cause issues with the kernel's spurious interrupt detection.

[ ... ]
> +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 = of_property_read_u32(np, "dma-channels", &chan_nr);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to read dma-channels\n");
> +
> +	len = sizeof(struct aml_dma_dev) + sizeof(struct aml_dma_chan) * chan_nr;
> +	aml_dma = devm_kzalloc(&pdev->dev, len, GFP_KERNEL);

[Severity: High]
Is the calculation of 'len' susceptible to an integer overflow? 'chan_nr' is
read directly from the device tree and isn't bounds-checked before being used
in the calculation of the signed integer 'len'. Could a large 'chan_nr' cause
a heap buffer overflow later during initialization?

[ ... ]
> +	ret = dmaenginem_async_device_register(dma_dev);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to register dmaenginem\n");
> +
> +	ret = of_dma_controller_register(np, aml_of_dma_xlate, aml_dma);
> +	if (ret)
> +		goto err_dmaenginem_register;
> +
> +	regmap_write(aml_dma->regmap, RCH_INT_MASK, 0xffffffff);
> +	regmap_write(aml_dma->regmap, WCH_INT_MASK, 0xffffffff);
> +
> +	ret = devm_request_irq(&pdev->dev, aml_dma->irq, aml_dma_interrupt_handler,
> +			       IRQF_SHARED, dev_name(&pdev->dev), aml_dma);

[Severity: High]
Does exposing the DMA controller to clients before the interrupt is requested
leave a window where a client could queue a transfer and the hardware could
generate an unhandled interrupt? Should the interrupt be initialized before
dmaenginem_async_device_register() and of_dma_controller_register() are called?

> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to request_irq\n");
> +		goto err_of_dma_controller;
> +	}
> +
> +	platform_set_drvdata(pdev, aml_dma);
> +
> +	return 0;
> +
> +err_of_dma_controller:
> +	of_dma_controller_free(np);
> +err_dmaenginem_register:
> +	dma_async_device_unregister(dma_dev);

[Severity: High]
Because dmaenginem_async_device_register() already registers a devres action
to unregister the device upon failure, does calling dma_async_device_unregister()
here manually result in a double-free or list corruption?

> +
> +	return ret;
> +}
> +
> +static void aml_dma_remove(struct platform_device *pdev)
> +{
> +	struct aml_dma_dev *aml_dma = platform_get_drvdata(pdev);
> +
> +	of_dma_controller_free((&pdev->dev)->of_node);
> +	dma_async_device_unregister(&aml_dma->dma_device);

[Severity: High]
Similarly, does calling dma_async_device_unregister() here cause a double-free
since dmaenginem_async_device_register() was used?

[Severity: High]
Is a call to tasklet_kill() needed here during removal? Since the virt-dma
framework schedules tasklets for DMA completions, failing to kill them could
lead to a use-after-free if a tasklet executes after the driver has been unloaded.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-amlogic-dma-v8-0-86cc2ce94142@amlogic.com?part=2

[PATCH v8 3/3] MAINTAINERS: Add an entry for Amlogic DMA driver

[Xianwei Zhao via B4 Relay]

From: Xianwei Zhao <xianwei.zhao@amlogic.com>

Add Amlogic DMA controller entry to MAINTAINERS to clarify
the maintainers.

Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index b38452804a2d..e7530b1e7152 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1316,6 +1316,13 @@ F:	Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml
 F:	drivers/perf/amlogic/
 F:	include/soc/amlogic/
 
+AMLOGIC DMA DRIVER
+M:	Xianwei Zhao <xianwei.zhao@amlogic.com>
+L:	linux-amlogic@lists.infradead.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/dma/amlogic,a9-dma.yaml
+F:	drivers/dma/amlogic-dma.c
+
 AMLOGIC ISP DRIVER
 M:	Keke Li <keke.li@amlogic.com>
 L:	linux-media@vger.kernel.org

-- 
2.52.0