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