[PATCH net-next v3 0/2] net: wwan: qcom_bam_dmux: Add restricted DMA pool support

[PATCH net-next v3 0/2] net: wwan: qcom_bam_dmux: Add restricted DMA pool support

[Vishnu Santhosh]

Some Qualcomm SoCs, such as Shikra, run the modem in a separate security
domain with restricted access to system memory. BAM-DMUX DMA mappings
and BAM descriptor FIFOs must therefore be constrained to a designated
memory region that can be shared with the modem. Accesses outside that
region can trigger an XPU violation.

Add memory-region and qcom,vmid properties to the BAM-DMUX binding, and
a qcom,shikra-bam-dmux compatible that requires them. When qcom,vmid is
present, the driver SCM-assigns the entire restricted DMA pool to the
local and remote execution environments before requesting the DMA
channels.

The restricted pool then contains the BAM-DMUX mappings. When the BAM DMA
controller references the same pool, its descriptor FIFOs are covered by
the assignment as well.

A related binding change allows the BAM DMA controller to use the same
restricted pool for descriptor FIFO allocations:

https://lore.kernel.org/all/20261021-qcom-bam-dma-vmid-ext-v3-1-299355e8dd3b@oss.qualcomm.com/
("dt-bindings: dma: qcom,bam-dma: Allow memory-region property")

Signed-off-by: Vishnu Santhosh <vishnu.santhosh@oss.qualcomm.com>
---
Changes in v3:
- dt-bindings: Add qcom,shikra-bam-dmux, with qcom,bam-dmux as fallback,
  and require memory-region and qcom,vmid for it (Krzysztof)
- dt-bindings: Drop minItems from qcom,vmid (Krzysztof)
- dt-bindings: Drop the example changes (Krzysztof)
- dt-bindings: Drop the statement about the local VMID from the
  qcom,vmid description (Sashiko)
- Reuse remove() on late probe failure, since the PC IRQ may already
  have powered up the RX path (Sashiko)
- Skip the pool reclaim when the remote did not acknowledge the
  power-down (Sashiko)
- Fail probe if the restricted pool is not attached to the device
  (Sashiko)
- Link to v2: https://lore.kernel.org/r/20260920-qcom-bam-dmux-vmid-ext-v2-0-ef5ca2178378@oss.qualcomm.com

Changes in v2:
- Replace the Shikra-specific compatible and coherent RX allocation with
  memory-region and qcom,vmid properties.
- Use a restricted DMA pool for BAM-DMUX mappings and BAM descriptor FIFOs.
- SCM-assign the complete pool before requesting the DMA channels.
- Validate the remote VMID and restricted DMA pool configuration.
- Link to v1: https://lore.kernel.org/r/20260714-qcom-bam-dmux-vmid-ext-v1-0-3f29da7cca76@oss.qualcomm.com

---
Vishnu Santhosh (2):
      dt-bindings: net: qcom,bam-dmux: Add memory-region and qcom,vmid properties
      net: wwan: qcom_bam_dmux: Assign restricted DMA pool to remote processor

 .../devicetree/bindings/net/qcom,bam-dmux.yaml     |  36 ++++++-
 drivers/net/wwan/Kconfig                           |   1 +
 drivers/net/wwan/qcom_bam_dmux.c                   | 119 ++++++++++++++++++++-
 3 files changed, 152 insertions(+), 4 deletions(-)
---
base-commit: 78974bac98eb0581f85aab56ccc0377cdcedb964
change-id: 20260714-qcom-bam-dmux-vmid-ext-d9289db310c1

Best regards,
-- 
Vishnu Santhosh <vishnu.santhosh@oss.qualcomm.com>

[PATCH net-next v3 1/2] dt-bindings: net: qcom,bam-dmux: Add memory-region and qcom,vmid properties

[Vishnu Santhosh]

Some Qualcomm SoCs, such as Shikra, run the modem in a separate security
domain with restricted access to system memory. BAM-DMUX DMA mappings
must therefore be constrained to a designated region that can be shared
with the modem.

Add an optional memory-region property to reference a restricted DMA
pool used for BAM-DMUX mappings, and a qcom,vmid property to identify the
remote execution environment that is granted access to the pool through
SCM.

Require memory-region whenever qcom,vmid is specified.

Add the qcom,shikra-bam-dmux compatible, with qcom,bam-dmux as fallback,
and require both properties for it.

Co-developed-by: Deepak Kumar Singh <deepak.singh@oss.qualcomm.com>
Signed-off-by: Deepak Kumar Singh <deepak.singh@oss.qualcomm.com>
Signed-off-by: Vishnu Santhosh <vishnu.santhosh@oss.qualcomm.com>
---
 .../devicetree/bindings/net/qcom,bam-dmux.yaml     | 36 +++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/qcom,bam-dmux.yaml b/Documentation/devicetree/bindings/net/qcom,bam-dmux.yaml
index 33746c238513d72366bc52359fb10f275475b331..f7388d24c8612b5a99d5d0e66cfbe2de3b1c9e0d 100644
--- a/Documentation/devicetree/bindings/net/qcom,bam-dmux.yaml
+++ b/Documentation/devicetree/bindings/net/qcom,bam-dmux.yaml
@@ -22,7 +22,11 @@ description: |
 
 properties:
   compatible:
-    const: qcom,bam-dmux
+    oneOf:
+      - const: qcom,bam-dmux
+      - items:
+          - const: qcom,shikra-bam-dmux
+          - const: qcom,bam-dmux
 
   interrupts:
     description:
@@ -72,6 +76,22 @@ properties:
       - const: tx
       - const: rx
 
+  memory-region:
+    maxItems: 1
+    description:
+      Phandle to a reserved-memory node compatible with restricted-dma-pool.
+      The BAM-DMUX device uses the pool for DMA mappings.
+
+  qcom,vmid:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    maxItems: 1
+    items:
+      minimum: 1
+      maximum: 63
+    description:
+      VMID of the remote execution environment that is granted access to the
+      restricted DMA pool through SCM.
+
 required:
   - compatible
   - interrupts
@@ -81,6 +101,20 @@ required:
   - dmas
   - dma-names
 
+dependencies:
+  qcom,vmid: [ memory-region ]
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: qcom,shikra-bam-dmux
+    then:
+      required:
+        - memory-region
+        - qcom,vmid
+
 additionalProperties: false
 
 examples:

-- 
2.34.1

[PATCH net-next v3 2/2] net: wwan: qcom_bam_dmux: Assign restricted DMA pool to remote processor

[Vishnu Santhosh]

Some Qualcomm SoCs, such as Shikra, run the modem in a separate security
domain with restricted access to system memory. BAM-DMUX DMA mappings
and BAM descriptor FIFOs must therefore be constrained to a designated
memory region that can be shared with the modem. Accesses outside that
region can trigger an XPU violation.

When qcom,vmid is present, require a restricted DMA pool and assign the
entire pool to both the local and remote execution environments before
requesting the DMA channels.

This ensures that BAM-DMUX mappings are within the assigned region. When
the BAM DMA controller references the same pool, its descriptor FIFOs
are covered by the assignment as well.

Track the assigned execution environments explicitly and reclaim the
pool for the local environment after DMA channels and mappings are
released during remove or probe error cleanup. Skip the reclaim and
leave the pool assigned if the remote side did not acknowledge the
power-down, since it may still access the pool.

Fail probing when restricted DMA pool support is unavailable, when the
VMID property is malformed, when memory-region does not reference a
restricted DMA pool, or when the pool is not attached to the device.

Co-developed-by: Deepak Kumar Singh <deepak.singh@oss.qualcomm.com>
Signed-off-by: Deepak Kumar Singh <deepak.singh@oss.qualcomm.com>
Signed-off-by: Vishnu Santhosh <vishnu.santhosh@oss.qualcomm.com>
---
 drivers/net/wwan/Kconfig         |   1 +
 drivers/net/wwan/qcom_bam_dmux.c | 119 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 117 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
index 958dbc7347fa84ee869439bf8b503037faab8bef..1b133c56231615269698140187ca3141dfe48dbf 100644
--- a/drivers/net/wwan/Kconfig
+++ b/drivers/net/wwan/Kconfig
@@ -65,6 +65,7 @@ config MHI_WWAN_MBIM
 config QCOM_BAM_DMUX
 	tristate "Qualcomm BAM-DMUX WWAN network driver"
 	depends on (DMA_ENGINE && PM && QCOM_SMEM_STATE) || COMPILE_TEST
+	select QCOM_SCM
 	help
 	  The BAM Data Multiplexer provides access to the network data channels
 	  of modems integrated into many older Qualcomm SoCs, e.g. Qualcomm
diff --git a/drivers/net/wwan/qcom_bam_dmux.c b/drivers/net/wwan/qcom_bam_dmux.c
index cc6ace8d64371eb8d00c638a39b234ee540b83c9..fa3621224a56e34086635e1abe1220c3ae40e899 100644
--- a/drivers/net/wwan/qcom_bam_dmux.c
+++ b/drivers/net/wwan/qcom_bam_dmux.c
@@ -9,14 +9,17 @@
 #include <linux/completion.h>
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
+#include <linux/firmware/qcom/qcom_scm.h>
 #include <linux/if_arp.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/soc/qcom/smem_state.h>
 #include <linux/spinlock.h>
+#include <linux/swiotlb.h>
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 #include <net/pkt_sched.h>
@@ -75,6 +78,9 @@ struct bam_dmux {
 	struct completion pc_ack_completion;
 
 	struct dma_chan *rx, *tx;
+	phys_addr_t dma_pool_base;
+	size_t dma_pool_size;
+	u64 dma_pool_perms;
 	struct bam_dmux_skb_dma rx_skbs[BAM_DMUX_NUM_SKB];
 	struct bam_dmux_skb_dma tx_skbs[BAM_DMUX_NUM_SKB];
 	spinlock_t tx_lock; /* Protect tx_skbs, tx_next_skb */
@@ -762,6 +768,96 @@ static int __maybe_unused bam_dmux_runtime_resume(struct device *dev)
 	return 0;
 }
 
+static int bam_dmux_assign_dma_pool(struct bam_dmux *dmux)
+{
+	struct device *dev = dmux->dev;
+	struct device_node *rmem_np;
+	struct reserved_mem *rmem;
+	struct qcom_scm_vmperm dst[2];
+	u64 src = BIT_ULL(QCOM_SCM_VMID_HLOS);
+	u32 vmid;
+	int ret;
+
+	if (!of_property_present(dev->of_node, "qcom,vmid"))
+		return 0;
+
+	ret = of_property_read_u32(dev->of_node, "qcom,vmid", &vmid);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to read qcom,vmid\n");
+	if (vmid == QCOM_SCM_VMID_HLOS || vmid >= BITS_PER_TYPE(u64))
+		return dev_err_probe(dev, -EINVAL, "Invalid qcom,vmid %u\n", vmid);
+
+	if (!IS_ENABLED(CONFIG_DMA_RESTRICTED_POOL))
+		return dev_err_probe(dev, -EOPNOTSUPP,
+				     "qcom,vmid requires DMA_RESTRICTED_POOL\n");
+
+	rmem_np = of_parse_phandle(dev->of_node, "memory-region", 0);
+	if (!rmem_np)
+		return dev_err_probe(dev, -EINVAL,
+					     "qcom,vmid requires memory-region\n");
+
+	if (!of_device_is_compatible(rmem_np, "restricted-dma-pool")) {
+		of_node_put(rmem_np);
+		return dev_err_probe(dev, -EINVAL,
+				     "memory-region must reference a restricted DMA pool\n");
+	}
+
+	rmem = of_reserved_mem_lookup(rmem_np);
+	of_node_put(rmem_np);
+	if (!rmem)
+		return dev_err_probe(dev, -EINVAL,
+					     "Failed to look up restricted DMA pool\n");
+
+	if (!is_swiotlb_for_alloc(dev))
+		return dev_err_probe(dev, -EINVAL,
+				     "Restricted DMA pool not attached\n");
+
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
+	dst[0].vmid = QCOM_SCM_VMID_HLOS;
+	dst[0].perm = QCOM_SCM_PERM_RW;
+	dst[1].vmid = vmid;
+	dst[1].perm = QCOM_SCM_PERM_RW;
+
+	ret = qcom_scm_assign_mem(rmem->base, rmem->size, &src, dst,
+				  ARRAY_SIZE(dst));
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "SCM assign restricted DMA pool failed\n");
+
+	dmux->dma_pool_base = rmem->base;
+	dmux->dma_pool_size = rmem->size;
+	/* Track the destination VMIDs explicitly for the reclaim operation. */
+	dmux->dma_pool_perms = BIT_ULL(QCOM_SCM_VMID_HLOS) | BIT_ULL(vmid);
+
+	return 0;
+}
+
+static void bam_dmux_reclaim_dma_pool(struct bam_dmux *dmux)
+{
+	struct qcom_scm_vmperm hlos = {
+		.vmid = QCOM_SCM_VMID_HLOS,
+		.perm = QCOM_SCM_PERM_RW,
+	};
+	u64 src = dmux->dma_pool_perms;
+	int ret;
+
+	if (!dmux->dma_pool_perms)
+		return;
+
+	ret = qcom_scm_assign_mem(dmux->dma_pool_base, dmux->dma_pool_size, &src,
+				  &hlos, 1);
+	if (ret) {
+		dev_err(dmux->dev, "SCM reclaim restricted DMA pool failed: %d\n", ret);
+		return;
+	}
+
+	dmux->dma_pool_perms = 0;
+}
+
+static void bam_dmux_remove(struct platform_device *pdev);
+
 static int bam_dmux_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -796,6 +892,10 @@ static int bam_dmux_probe(struct platform_device *pdev)
 				     "Failed to get pc-ack state\n");
 	dmux->pc_ack_mask = BIT(bit);
 
+	ret = bam_dmux_assign_dma_pool(dmux);
+	if (ret)
+		return ret;
+
 	init_waitqueue_head(&dmux->pc_wait);
 	init_completion(&dmux->pc_ack_completion);
 	complete_all(&dmux->pc_ack_completion);
@@ -829,8 +929,11 @@ static int bam_dmux_probe(struct platform_device *pdev)
 
 	ret = irq_get_irqchip_state(dmux->pc_irq, IRQCHIP_STATE_LINE_LEVEL,
 				    &dmux->pc_state);
-	if (ret)
-		goto err_disable_pm;
+	if (ret) {
+		/* The PC IRQ may already have powered up the RX path */
+		bam_dmux_remove(pdev);
+		return ret;
+	}
 
 	/* Check if remote finished initialization before us */
 	if (dmux->pc_state) {
@@ -845,6 +948,7 @@ static int bam_dmux_probe(struct platform_device *pdev)
 err_disable_pm:
 	pm_runtime_disable(dev);
 	pm_runtime_dont_use_autosuspend(dev);
+	bam_dmux_reclaim_dma_pool(dmux);
 	return ret;
 }
 
@@ -853,6 +957,7 @@ static void bam_dmux_remove(struct platform_device *pdev)
 	struct bam_dmux *dmux = platform_get_drvdata(pdev);
 	struct device *dev = dmux->dev;
 	LIST_HEAD(list);
+	bool remote_suspended;
 	int i;
 
 	/* Unregister network interfaces */
@@ -872,13 +977,21 @@ static void bam_dmux_remove(struct platform_device *pdev)
 	pm_runtime_set_suspended(dev);
 
 	/* Try to wait for remote side to drop power vote */
-	if (!wait_event_timeout(dmux->pc_wait, !dmux->rx, BAM_DMUX_REMOTE_TIMEOUT))
+	remote_suspended = wait_event_timeout(dmux->pc_wait, !dmux->rx,
+					      BAM_DMUX_REMOTE_TIMEOUT);
+	if (!remote_suspended)
 		dev_err(dev, "Timed out waiting for remote side to suspend\n");
 
 	/* Make sure everything is cleaned up before we return */
 	disable_irq(dmux->pc_irq);
 	bam_dmux_power_off(dmux);
 	bam_dmux_free_skbs(dmux->tx_skbs, DMA_TO_DEVICE);
+
+	/* The remote side may still access the pool if it did not suspend */
+	if (remote_suspended)
+		bam_dmux_reclaim_dma_pool(dmux);
+	else if (dmux->dma_pool_perms)
+		dev_warn(dev, "Leaving restricted DMA pool assigned to remote\n");
 }
 
 static const struct dev_pm_ops bam_dmux_pm_ops = {

-- 
2.34.1

Re: [PATCH net-next v3 0/2] net: wwan: qcom_bam_dmux: Add restricted DMA pool support

[Jakub Kicinski]

On Wed, 21 Oct 2026 12:09:42 +0530 Vishnu Santhosh wrote:
> Some Qualcomm SoCs, such as Shikra, run the modem in a separate security
> domain with restricted access to system memory. BAM-DMUX DMA mappings
> and BAM descriptor FIFOs must therefore be constrained to a designated
> memory region that can be shared with the modem. Accesses outside that
> region can trigger an XPU violation.

## Form letter - net-next-closed

The merge window for Linux v7.4 has begun, and the net-next
tree is now closed for new drivers, features, code refactoring
and optimizations. We are currently accepting bug fixes only.

Please repost when net-next reopens. There is some uncertainty
about how long this merge window will last, so please check
https://netdev.bots.linux.dev/net-next.html
before reposting. net-next _may_ reopen on Nov 2nd.

See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle
-- 
pw-bot: defer
pv-bot: closed

Re: [PATCH net-next v3 1/2] dt-bindings: net: qcom,bam-dmux: Add memory-region and qcom,vmid properties

[Krzysztof Kozlowski]

On Wed, Oct 21, 2026 at 12:09:43PM +0530, Vishnu Santhosh wrote:
> Some Qualcomm SoCs, such as Shikra, run the modem in a separate security
> domain with restricted access to system memory. BAM-DMUX DMA mappings
> must therefore be constrained to a designated region that can be shared
> with the modem.
> 
> Add an optional memory-region property to reference a restricted DMA
> pool used for BAM-DMUX mappings, and a qcom,vmid property to identify the
> remote execution environment that is granted access to the pool through
> SCM.
> 
> Require memory-region whenever qcom,vmid is specified.
> 
> Add the qcom,shikra-bam-dmux compatible, with qcom,bam-dmux as fallback,
> and require both properties for it.
> 
> Co-developed-by: Deepak Kumar Singh <deepak.singh@oss.qualcomm.com>
> Signed-off-by: Deepak Kumar Singh <deepak.singh@oss.qualcomm.com>
> Signed-off-by: Vishnu Santhosh <vishnu.santhosh@oss.qualcomm.com>
> ---
>  .../devicetree/bindings/net/qcom,bam-dmux.yaml     | 36 +++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

Re: [PATCH net-next v3 1/2] dt-bindings: net: qcom,bam-dmux: Add memory-region and qcom,vmid properties

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new memory-region/qcom,vmid properties only cover the bam-
  dmux client node.
- [Low] Documentation/devicetree/bindings/net/qcom,bam-dmux.yaml still
  says (lines 18-21): "Note that this schema does not directly describe…
- [Low] Baseline decision: false (resolved later in the series).

--

dt-bindings: net: qcom,bam-dmux: Add memory-region and qcom,vmid properties

This adds two optional properties to the qcom,bam-dmux binding,
memory-region and qcom,vmid. They confine BAM-DMUX DMA mappings to a
restricted DMA pool that is shared with the modem through SCM. It also
adds a qcom,shikra-bam-dmux compatible, with qcom,bam-dmux as fallback,
which requires both properties.

> diff --git a/Documentation/devicetree/bindings/net/qcom,bam-dmux.yaml b/Documentation/devicetree/bindings/net/qcom,bam-dmux.yaml
> index 33746c238513d..f7388d24c8612 100644
> --- a/Documentation/devicetree/bindings/net/qcom,bam-dmux.yaml
> +++ b/Documentation/devicetree/bindings/net/qcom,bam-dmux.yaml
> @@ -22,7 +22,11 @@ description: |
>  
>  properties:
>    compatible:
> -    const: qcom,bam-dmux
> +    oneOf:
> +      - const: qcom,bam-dmux
> +      - items:
> +          - const: qcom,shikra-bam-dmux
> +          - const: qcom,bam-dmux

[Severity: Low]
The description at the top of this binding still says:

  Note that this schema does not directly describe a hardware block but rather
  a firmware convention that combines several other hardware blocks (such as the
  DMA engine). As such it is specific to a firmware version, not a particular
  SoC or hardware version.

This patch adds the first SoC-specific compatible. Should that text be
updated too?

The commit message also doesn't explain why the no-SoC-compatible design
is being dropped. The only effect of qcom,shikra-bam-dmux is the allOf/if
block that makes memory-region and qcom,vmid required. The driver's
of_match table only lists the generic "qcom,bam-dmux".

Separately, the subject line only says "Add memory-region and qcom,vmid
properties". Could it also mention the new compatible, since that adds to
the DT ABI?

[ ... ]

> @@ -72,6 +76,22 @@ properties:
>        - const: tx
>        - const: rx
>  
> +  memory-region:
> +    maxItems: 1
> +    description:
> +      Phandle to a reserved-memory node compatible with restricted-dma-pool.
> +      The BAM-DMUX device uses the pool for DMA mappings.

[Severity: Medium]
Does a pool on the bam-dmux node cover everything the BAM accesses on
Shikra?

The pool only affects dma_map_single(dmux->dev, ...) in
bam_dmux_skb_dma_map(). The BAM descriptor FIFOs are allocated on the BAM
controller's own device in drivers/dma/qcom/bam_dma.c:

bam_alloc_chan() {
    ...
	bchan->fifo_virt = dma_alloc_wc(bdev->dev, BAM_DESC_FIFO_SIZE,
					&bchan->fifo_phys, GFP_KERNEL);
    ...
}

bam_chan_init_hw() then programs that address into the hardware:

	writel_relaxed(ALIGN(bchan->fifo_phys, sizeof(struct bam_desc_hw)),
			bam_addr(bdev, bchan->id, BAM_P_DESC_FIFO_ADDR));

The companion patch "net: wwan: qcom_bam_dmux: Assign restricted DMA pool
to remote processor" says that "BAM-DMUX DMA mappings and BAM descriptor
FIFOs must therefore be constrained to a designated memory region", and
that accesses outside that region "can trigger an XPU violation". It also
says the FIFOs are covered only "When the BAM DMA controller references
the same pool".

However, Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml defines
no memory-region property and has additionalProperties: false. Nothing in
the series changes that.

So a schema-valid Shikra DT would leave the descriptor FIFOs outside the
shared pool, and a DT that works would fail dtbs_check. Should
qcom,bam-dma.yaml get a memory-region property as part of this series?

> +
> +  qcom,vmid:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    maxItems: 1
> +    items:
> +      minimum: 1
> +      maximum: 63
> +    description:
> +      VMID of the remote execution environment that is granted access to the
> +      restricted DMA pool through SCM.

[Severity: Low]
Nothing reads qcom,vmid or does the SCM assignment at this point in the
series. A Shikra DT that binds through the qcom,bam-dmux fallback would
bounce buffers into a pool the modem has not been given access to. Is
that gap between this patch and the next one acceptable?

The next patch in the series, "net: wwan: qcom_bam_dmux: Assign
restricted DMA pool to remote processor", appears to resolve it. It adds
bam_dmux_assign_dma_pool() to bam_dmux_probe(). That function reads
qcom,vmid and calls qcom_scm_assign_mem() with HLOS and the remote VMID
before the DMA channels are requested. The patch also adds
bam_dmux_reclaim_dma_pool(), which runs on remove and on probe error.

The driver matches the fallback compatible and handles the property
whenever it is present. So the fallback semantics look correct by the
end of the series, and this is noted only for completeness.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-qcom-bam-dmux-vmid-ext-v3-0-cedd1f681102%40oss.qualcomm.com

Re: [PATCH net-next v3 2/2] net: wwan: qcom_bam_dmux: Assign restricted DMA pool to remote processor

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 4 (1) · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says BAM descriptor FIFOs "must therefore
  be constrained" to the shared region, and that they are covered "when…
- [Medium] bam_dmux_assign_dma_pool() always passes `u64 src =
  BIT_ULL(QCOM_SCM_VMID_HLOS)` (line 777) to qcom_scm_assign_mem(), so…
- [Medium] In bam_dmux_remove(), `remote_suspended` decides whether the
  pool goes back to HLOS-only, and it can be wrong in two ways.
- [Medium] Once the restricted pool is assigned RW to the modem VMID,
  every RX buffer mapping copies uninitialized HLOS memory into memory…
- [Low] The patch changes the probe error path in bam_dmux_probe() for an
  irq_get_irqchip_state() failure.
- [Low] The binding and the driver disagree on which qcom,vmid values are
  valid.
- [Low] The driver and binding accept qcom,vmid values up to 63, and
  bam_dmux_reclaim_dma_pool() passes `src = BIT_ULL(HLOS) |…

Pre-existing issues:
- [High] bam_dmux_remove() calls
  cancel_work_sync(&dmux->register_netdev_work) at line 964, before RX…
- [Medium] This problem wasn't introduced by this patch, but net_devices
  that registered successfully are unregistered and never freed.

--

net: wwan: qcom_bam_dmux: Assign restricted DMA pool to remote processor

When qcom,vmid is present in DT, this patch requires a restricted DMA pool
and uses SCM to assign the whole pool to both HLOS and the remote VMID. The
pool is reclaimed for HLOS on remove or on probe error cleanup, unless the
remote side did not drop its power vote. It also changes the
irq_get_irqchip_state() failure path in probe to call bam_dmux_remove().

> diff --git a/drivers/net/wwan/qcom_bam_dmux.c b/drivers/net/wwan/qcom_bam_dmux.c
> index cc6ace8d64371..fa3621224a56e 100644
> --- a/drivers/net/wwan/qcom_bam_dmux.c
> +++ b/drivers/net/wwan/qcom_bam_dmux.c

[ ... ]

> @@ -762,6 +768,96 @@ static int __maybe_unused bam_dmux_runtime_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static int bam_dmux_assign_dma_pool(struct bam_dmux *dmux)
> +{
> +	struct device *dev = dmux->dev;
> +	struct device_node *rmem_np;
> +	struct reserved_mem *rmem;
> +	struct qcom_scm_vmperm dst[2];
> +	u64 src = BIT_ULL(QCOM_SCM_VMID_HLOS);

[Severity: Medium]

Is it safe to assume on every probe that HLOS is the only current owner?
qcom_scm_assign_mem() documents @srcvm as the "vmid for current set of
owners".

Two paths can leave the pool owned by {HLOS, vmid} after the driver
instance is gone. The first is bam_dmux_remove(), which skips the reclaim
on a remote timeout:

	else if (dmux->dma_pool_perms)
		dev_warn(dev, "Leaving restricted DMA pool assigned to remote\n");

The second is bam_dmux_reclaim_dma_pool(), which only logs when the SCM
call fails.

In both cases the only record of the assignment is dmux->dma_pool_perms.
That field lives in devm_kzalloc() memory, which is freed right after
remove or after the failed probe.

On the next probe (module reload, or unbind/bind of the remoteproc
parent), the zeroed dmux would ask TZ to move the pool from {HLOS} while
{HLOS, vmid} actually owns it. Would the assign then fail with "SCM assign
restricted DMA pool failed" until reboot?

The new probe error path calls bam_dmux_remove() right after the PC IRQ
may have powered up RX, so it seems likely to hit the timeout case.

> +	u32 vmid;
> +	int ret;
> +
> +	if (!of_property_present(dev->of_node, "qcom,vmid"))
> +		return 0;
> +
> +	ret = of_property_read_u32(dev->of_node, "qcom,vmid", &vmid);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to read qcom,vmid\n");
> +	if (vmid == QCOM_SCM_VMID_HLOS || vmid >= BITS_PER_TYPE(u64))
> +		return dev_err_probe(dev, -EINVAL, "Invalid qcom,vmid %u\n", vmid);

[Severity: Low]

Should the binding and this check agree on the valid range? The
qcom,bam-dmux.yaml binding in this series allows:

    items:
      minimum: 1
      maximum: 63

This check rejects 3 (QCOM_SCM_VMID_HLOS) but accepts 0. So a DT with
qcom,vmid = <3> passes dtbs_check and then fails probe with -EINVAL. In
the other direction, vmid 0 is rejected by the schema but is passed to
SCM here.

[Severity: Low]

On 32-bit kernels, can a vmid between 32 and 63 be reclaimed correctly?
bam_dmux_reclaim_dma_pool() passes src = BIT_ULL(HLOS) | BIT_ULL(vmid).
However, qcom_scm_assign_mem() builds the source list with the unsigned
long BIT():

	for (b = 0; b < BITS_PER_TYPE(u64); b++) {
		if (srcvm_bits & BIT(b))
			src[i++] = cpu_to_le32(b);
	}

On 32-bit this shift is undefined for b >= 32, so the remote VMID entry
would not be written. src_sz is still computed with hweight64(), so the
SCM call would see an uninitialized trailing source slot. The helper's
int next_vm |= BIT(newvm->vmid) has a similar limit.

The root cause is in the existing SCM helper. However, this patch adds a
caller and accepts a range the helper cannot encode on 32-bit, and
QCOM_BAM_DMUX is used on 32-bit SoCs such as MSM8974. Should the range be
tightened here, or should the helper switch to BIT_ULL()?

[ ... ]

> +	rmem = of_reserved_mem_lookup(rmem_np);
> +	of_node_put(rmem_np);
> +	if (!rmem)
> +		return dev_err_probe(dev, -EINVAL,
> +					     "Failed to look up restricted DMA pool\n");
> +
> +	if (!is_swiotlb_for_alloc(dev))
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Restricted DMA pool not attached\n");

[Severity: Medium]

The commit message says:

    BAM-DMUX DMA mappings and BAM descriptor FIFOs must therefore be
    constrained to a designated memory region ...

    When the BAM DMA controller references the same pool, its descriptor
    FIFOs are covered by the assignment as well.

This check only looks at the bam-dmux device. The descriptor FIFOs are
allocated on the BAM controller's own device in bam_alloc_chan():

	bchan->fifo_virt = dma_alloc_wc(bdev->dev, BAM_DESC_FIFO_SIZE,
					&bchan->fifo_phys, GFP_KERNEL);

Their address is then programmed into BAM_P_DESC_FIFO_ADDR in
bam_chan_init_hw().

Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml has no
memory-region property and sets additionalProperties: false. This series
only updates the bam-dmux binding.

With a schema-valid DT, wouldn't the FIFOs stay in ordinary memory that
the modem VMID cannot reach? That would leave the XPU violation possible
for FIFO accesses.

Should the bam-dma binding gain a memory-region property? Should the
driver also verify, or at least warn, that the DMA controller behind the
rx/tx channels uses the same pool?

> +
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
> +	dst[0].vmid = QCOM_SCM_VMID_HLOS;
> +	dst[0].perm = QCOM_SCM_PERM_RW;
> +	dst[1].vmid = vmid;
> +	dst[1].perm = QCOM_SCM_PERM_RW;
> +
> +	ret = qcom_scm_assign_mem(rmem->base, rmem->size, &src, dst,
> +				  ARRAY_SIZE(dst));

[Severity: Medium]

Once the whole pool is RW for the modem VMID, does every RX mapping copy
uninitialized kernel memory into it?

bam_dmux_skb_dma_queue_rx() allocates the RX skb with
__netdev_alloc_skb() and calls skb_put(skb, BAM_DMUX_BUFFER_SIZE) without
zeroing. It then maps the buffer DMA_FROM_DEVICE.

With a restricted-dma-pool, swiotlb_tbl_map_single() copies the original
buffer into the pool slot before the transfer, whatever the direction:

	swiotlb_bounce(dev, tlb_addr, mapping_size, DMA_TO_DEVICE, pool);

That would put up to 2 KiB of stale kernel data per buffer into memory
shared with the modem. This happens for all 32 RX buffers at power-on and
again on each refill in bam_dmux_rx_callback().

Should the RX buffers be zeroed before mapping when qcom,vmid is set?

[ ... ]

> @@ -829,8 +929,11 @@ static int bam_dmux_probe(struct platform_device *pdev)
>  
>  	ret = irq_get_irqchip_state(dmux->pc_irq, IRQCHIP_STATE_LINE_LEVEL,
>  				    &dmux->pc_state);
> -	if (ret)
> -		goto err_disable_pm;
> +	if (ret) {
> +		/* The PC IRQ may already have powered up the RX path */
> +		bam_dmux_remove(pdev);
> +		return ret;
> +	}

[Severity: Low]

This changes the irq_get_irqchip_state() failure path from only disabling
runtime PM to a full bam_dmux_remove(). That seems to fix an existing bug.
The threaded PC IRQ is already registered at this point and may have run
bam_dmux_power_on(), which requests the RX channel and maps and submits
the RX skbs. The old err_disable_pm path leaked all of that while devm
freed dmux.

Could this be split into a separate patch with a Fixes: tag so it reaches
stable, and mentioned in the commit message?

The change also alters probe behaviour. The failure path can now block in
wait_event_timeout() for up to BAM_DMUX_REMOTE_TIMEOUT, it sends a
power-down vote through bam_dmux_runtime_suspend(), and it calls
disable_irq() on devm-managed IRQs.

[ ... ]

> @@ -853,6 +957,7 @@ static void bam_dmux_remove(struct platform_device *pdev)
>  	struct bam_dmux *dmux = platform_get_drvdata(pdev);
>  	struct device *dev = dmux->dev;
>  	LIST_HEAD(list);
> +	bool remote_suspended;
>  	int i;
>  
>  	/* Unregister network interfaces */

[Severity: High]

This isn't a bug introduced by this patch, but the new probe error path
relies on bam_dmux_remove() to tear down an RX path that may already be
active. remove cancels register_netdev_work before RX is stopped:

	cancel_work_sync(&dmux->register_netdev_work);
	...
	remote_suspended = wait_event_timeout(dmux->pc_wait, !dmux->rx,
	...
	disable_irq(dmux->pc_irq);
	bam_dmux_power_off(dmux);

During the wait, an RX completion can carry a BAM_DMUX_CMD_OPEN command
for a channel that has no netdev. That goes through
bam_dmux_rx_callback()->bam_dmux_cmd_open(), which calls
schedule_work(&dmux->register_netdev_work) again. Nothing cancels the work
after that.

Can the work then run after devres frees dmux, or register netdevs that
point at the freed dmux and are never unregistered?

During probe, netdevs[] is all NULL, so after a power-up ack every open
command from the modem would schedule the work.

[Severity: Medium]

This isn't a bug introduced by this patch, but are the registered
net_devices ever freed?

bam_dmux_register_netdev_work() calls free_netdev() only when
register_netdev() fails. bam_dmux_netdev_setup() does not set
needs_free_netdev, and this path only calls unregister_netdevice_queue()
and unregister_netdevice_many(). netdev_run_todo() only frees the device
when needs_free_netdev is set.

So each registered netdev seems to leak on remove. With this patch it can
also leak in the new probe error cleanup, if interfaces were already
created.

> @@ -872,13 +977,21 @@ static void bam_dmux_remove(struct platform_device *pdev)
>  	pm_runtime_set_suspended(dev);
>  
>  	/* Try to wait for remote side to drop power vote */
> -	if (!wait_event_timeout(dmux->pc_wait, !dmux->rx, BAM_DMUX_REMOTE_TIMEOUT))
> +	remote_suspended = wait_event_timeout(dmux->pc_wait, !dmux->rx,
> +					      BAM_DMUX_REMOTE_TIMEOUT);
> +	if (!remote_suspended)
>  		dev_err(dev, "Timed out waiting for remote side to suspend\n");
>  
>  	/* Make sure everything is cleaned up before we return */
>  	disable_irq(dmux->pc_irq);
>  	bam_dmux_power_off(dmux);
>  	bam_dmux_free_skbs(dmux->tx_skbs, DMA_TO_DEVICE);
> +
> +	/* The remote side may still access the pool if it did not suspend */
> +	if (remote_suspended)
> +		bam_dmux_reclaim_dma_pool(dmux);

[Severity: Medium]

Can remote_suspended be stale or wrong by the time it gates the reclaim?

It is sampled while the PC IRQ is still enabled, and disable_irq() only
runs afterwards. In between, the modem can raise its power vote again,
and bam_dmux_pc_irq() would run:

	if (new_state) {
		if (bam_dmux_power_on(dmux))
			bam_dmux_pc_ack(dmux);

That requests RX, maps the RX skbs into the pool and acks the modem.
disable_irq() only waits for the handler to finish. The earlier true value
then reclaims the pool while the modem believes the link is up.

Also, dmux->rx == NULL does not necessarily mean the remote powered down.
If bam_dmux_power_on() fails in the PC IRQ handler or in probe,
bam_dmux_power_off() clears rx while pc_state is set to true. The wait
then returns immediately and the pool is reclaimed while the remote vote
is still asserted.

The commit message says:

    Skip the reclaim and leave the pool assigned if the remote side did not
    acknowledge the power-down, since it may still access the pool.

Would it be more robust to make this decision after disable_irq(), based
on the remote PC state (for example !dmux->pc_state)?

> +	else if (dmux->dma_pool_perms)
> +		dev_warn(dev, "Leaving restricted DMA pool assigned to remote\n");
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-qcom-bam-dmux-vmid-ext-v3-0-cedd1f681102%40oss.qualcomm.com