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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] SERIES-RESOLVED (retained for policy combination only): the
  concern that the newly documented `qcom,vmid`/`memory-region`…
- [Low] The `qcom,vmid` schema contradicts its own description: the
  description states "The local execution environment is retained…

--

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

This adds an optional memory-region property pointing at a
restricted-dma-pool reserved-memory node for BAM-DMUX DMA mappings, plus a
qcom,vmid property naming the remote execution environment that is granted
access to that pool via SCM. A dependency entry makes memory-region
mandatory whenever qcom,vmid is present.

> diff --git a/Documentation/devicetree/bindings/net/qcom,bam-dmux.yaml b/Documentation/devicetree/bindings/net/qcom,bam-dmux.yaml
> index 33746c238513..5351269b0ab7 100644
> --- a/Documentation/devicetree/bindings/net/qcom,bam-dmux.yaml
> +++ b/Documentation/devicetree/bindings/net/qcom,bam-dmux.yaml
> @@ -72,6 +72,24 @@ 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: Low]
This isn't a bug, just a note for the record on an earlier concern that
these two properties have no in-tree consumer.

The companion patch in this series, "net: wwan: qcom_bam_dmux: Assign
restricted DMA pool to remote processor", adds bam_dmux_assign_dma_pool()
in drivers/net/wwan/qcom_bam_dmux.c, which reads qcom,vmid, resolves
memory-region through of_parse_phandle() and of_reserved_mem_lookup(), and
then performs the grant:

    ret = qcom_scm_assign_mem(rmem->base, rmem->size, &src, dst,
                              ARRAY_SIZE(dst));

The pool attachment itself is handled generically by
of_dma_set_restricted_buffer() from of_dma_configure_id(), so the
"uses the pool for DMA mappings" wording is backed by existing code too.
Nothing further appears to be needed here.

> +  qcom,vmid:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 1
> +    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. The local execution environment is
> +      retained automatically and must not be specified.
> +

[Severity: Low]
Does the numeric constraint on qcom,vmid contradict its own description?

The description says the local execution environment "must not be
specified", but the only constraints are minimum 1 and maximum 63, and
include/dt-bindings/firmware/qcom,scm.h has:

    #define QCOM_SCM_VMID_HLOS          0x3

So qcom,vmid = <QCOM_SCM_VMID_HLOS> passes dt_binding_check even though the
binding text declares it invalid. The 1..63 range looks like it encodes the
width of the SCM VMID bitmask used for BIT_ULL(vmid) in a u64, not the set
of values meaningful for this device.

The driver side in this series does enforce the rule, in
bam_dmux_assign_dma_pool():

    if (vmid == QCOM_SCM_VMID_HLOS || vmid >= BITS_PER_TYPE(u64))
            return dev_err_probe(dev, -EINVAL, "Invalid qcom,vmid %u\n", vmid);

so the outcome of a bad value is a probe failure rather than anything worse.
Would it make sense to encode the same restriction in the schema, for
example with a not/const 3 clause or an enum of the supported remote
execution environments, or otherwise drop the "must not be specified"
sentence so the prose and the schema agree?

For context, neither qcom,rmtfs-mem.yaml nor qcom,fastrpc.yaml places value
constraints on their qcom,vmid / qcom,vmids properties, so an enum here
would be a new convention rather than an existing one; the narrow point is
just the internal inconsistency in this file.

[ ... ]

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