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

[Vishnu Santhosh <vishnu.santhosh@oss.qualcomm.com>]

On 25-10-2026 03:53 pm, netdev-bot+sashiko@kernel.org wrote:
> 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?

Yes. This is fail-safe, since the pool is static reserved memory that is
never returned to the page allocator. This matches the existing
qcom_scm_assign_mem() users, which also only log reclaim failures.

>
> 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.

This path is not reachable with the SMSM irqchip, as
smsm_get_irqchip_state() only fails for states other than
IRQCHIP_STATE_LINE_LEVEL.

>
>> +	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?

This is covered by a separate binding change for the BAM DMA controller:
https://lore.kernel.org/all/20261021-qcom-bam-dma-vmid-ext-v3-1-299355e8dd3b@oss.qualcomm.com/
With memory-region on the BAM DMA node, dma_alloc_wc() allocates the
descriptor FIFOs from the pool, without BAM DMA driver changes.

Using the same pool for both nodes is a DT requirement. The DMA channels
are only requested at modem power-up, so a check in BAM-DMUX could not
fail probe and would only warn after the fact.

>
>> +
>> +	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?

This is generic swiotlb behaviour for all restricted-dma-pool users: the
original buffer is bounced for every direction, so that bytes the device
does not write are preserved. Without a restricted pool, as on the
existing BAM-DMUX platforms, the BAM accesses the skb directly, including
its stale contents, so the pool does not widen the exposure.

>
> [ ... ]
>
>> @@ -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.

This is a pre-existing issue in bam_dmux_remove(), present since the
driver was added. This is out of scope for this series.

>
> [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.

This is a pre-existing issue in bam_dmux_remove(), present since the
driver was added. This is out of scope for this series.

>
>> @@ -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)?

Agreed. In the next revision, the reclaim can be decided after disable_irq()
based on dmux->pc_state, so a failed power-on with the remote vote still
asserted also leaves the pool assigned.

Vishnu

pw-bot: cr