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

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

The compatible was requested by the DT maintainers so the schema can
enforce the properties required by the Shikra firmware configuration.
The note about a firmware convention therefore still holds.

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

This is covered by a separate binding change for the BAM DMA controller,
which goes through the dmaengine tree:
https://lore.kernel.org/all/20261021-qcom-bam-dma-vmid-ext-v3-1-299355e8dd3b@oss.qualcomm.com/

Vishnu

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