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

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

On 21-09-2026 09:18 am, netdev-bot+sashiko@kernel.org wrote:
> 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?

I plan to drop the "must not be specified" sentence in the next
revision, so the prose and the schema agree.

Thanks,
Vishnu

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