linux-kernel - Re: [PATCH v8 2/6] dt-bindings: remoteproc: qcom: document hexagon based WCSS secure PIL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251220-amphibian-nippy-firefly-d4fac0@quoll>
Date: Sat, 20 Dec 2025 09:39:54 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Varadarajan Narayanan <varadarajan.narayanan@....qualcomm.com>
Cc: andersson@...nel.org, mathieu.poirier@...aro.org, robh@...nel.org, 
	krzk+dt@...nel.org, conor+dt@...nel.org, konradybcio@...nel.org, 
	quic_mmanikan@...cinc.com, linux-arm-msm@...r.kernel.org, linux-remoteproc@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Gokul Sriram Palanisamy <quic_gokulsri@...cinc.com>, George Moussalem <george.moussalem@...look.com>
Subject: Re: [PATCH v8 2/6] dt-bindings: remoteproc: qcom: document hexagon
 based WCSS secure PIL

On Fri, Dec 19, 2025 at 08:40:06AM +0530, Varadarajan Narayanan wrote:
> From: Manikanta Mylavarapu <quic_mmanikan@...cinc.com>
> 
> Add new binding document for hexagon based WCSS secure PIL remoteproc.
> IPQ5018, IPQ5332 and IPQ9574 follow secure PIL remoteproc.
> 
> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@...cinc.com>
> Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@...cinc.com>
> Signed-off-by: George Moussalem <george.moussalem@...look.com>
> [ Dropped ipq5424 support ]
> Signed-off-by: Varadarajan Narayanan <varadarajan.narayanan@....qualcomm.com>
> ---
> v8: Dropped Krzysztof's 'Reviewed-by' as the bindings file has changed significantly
>     Drop ipq5424 support
>     Update example to ipq9574 instead of ipq5424
>     Change 'mboxes' description

I do not see any "significant" differences in the binding. You ONLY
dropped one compatible (no problem, why would we care?) and renamed one
description in mboxs. No other changes, nothing, so I do not understand
what was the significant difference here.

Dropping reviews at v8 is really unexpected and to me it feels like you
rewrite everything, which should not happen at v8.


> ---
>  .../remoteproc/qcom,wcss-sec-pil.yaml         | 172 ++++++++++++++++++
>  1 file changed, 172 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
> new file mode 100644
> index 000000000000..0fe04e0a4ca5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml


Well, since you ask for re-review and I really cannot find the
difference, then let's start nitpicking from scratch:

Please use one of the compatibles, e.g. the oldest device, as filename.

> @@ -0,0 +1,172 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/qcom,wcss-sec-pil.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm WCSS Secure Peripheral Image Loader
> +
> +maintainers:
> +  - Manikanta Mylavarapu <quic_mmanikan@...cinc.com>
> +
> +description:
> +  Wireless Connectivity Subsystem (WCSS) Secure Peripheral Image Loader loads
> +  firmware and power up QDSP6 remoteproc on the Qualcomm IPQ series SoC.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,ipq5018-wcss-sec-pil
> +      - qcom,ipq5332-wcss-sec-pil
> +      - qcom,ipq9574-wcss-sec-pil
> +
> +  reg:
> +    maxItems: 1
> +
> +  firmware-name:
> +    maxItems: 1
> +    description: Firmware name for the Hexagon core
> +
> +  interrupts:
> +    items:
> +      - description: Watchdog interrupt
> +      - description: Fatal interrupt
> +      - description: Ready interrupt
> +      - description: Handover interrupt
> +      - description: Stop acknowledge interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: wdog
> +      - const: fatal
> +      - const: ready
> +      - const: handover
> +      - const: stop-ack
> +
> +  clocks:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array

What is this? How did it ever appear here? Sorry, but this is not a
syntax present anywhere.

> +
> +  clock-names:
> +    $ref: /schemas/types.yaml#/definitions/string-array

Neither this. Look, I have never reviewed something like this:

https://lore.kernel.org/all/20240829134021.1452711-2-quic_gokulsri@quicinc.com/

NAK, that's not a binding style at all. Please do not invent completely
different style.

What's weird, this change did not happen at v8, so you still kept my
review tag even after inventing this weird code.

> +
> +  mboxes:
> +    items:
> +      - description: TMECom mailbox driver
> +
> +  qcom,smem-states:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: States used by the AP to signal the remote processor
> +    items:
> +      - description: Stop Q6
> +      - description: Shutdown Q6
> +
> +  qcom,smem-state-names:
> +    description:
> +      Names of the states used by the AP to signal the remote processor
> +    items:
> +      - const: stop
> +      - const: shutdown
> +
> +  memory-region:
> +    items:
> +      - description: Q6 reserved region
> +
> +  glink-edge:
> +    $ref: /schemas/remoteproc/qcom,glink-edge.yaml#
> +    description:
> +      Qualcomm G-Link subnode which represents communication edge, channels
> +      and devices related to the Modem.
> +    unevaluatedProperties: false

Best regards,
Krzysztof