linux-kernel - Re: [PATCH] dt-bindings: serial: 8250: Make clocks and clock-frequency exclusive

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: <20250527-polio-snooze-c05aafc1e270@spud>
Date: Tue, 27 May 2025 16:24:11 +0100
From: Conor Dooley <conor@...nel.org>
To: Yao Zi <ziyao@...root.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Lubomir Rintel <lkundrak@...sk>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-serial@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: serial: 8250: Make clocks and
 clock-frequency exclusive

On Sat, May 24, 2025 at 10:56:02AM +0000, Yao Zi wrote:
> The 8250 binding before converting to json-schema states,
> 
>   - clock-frequency : the input clock frequency for the UART
>   	or
>   - clocks phandle to refer to the clk used as per Documentation/devicetree
> 
> for clock-related properties, where "or" indicates these properties
> shouldn't exist at the same time.
> 
> Additionally, the behavior of Linux's driver is strange when both clocks
> and clock-frequency are specified: it ignores clocks and obtains the
> frequency from clock-frequency, left the specified clocks unclaimed. It
> may even be disabled, which is undesired most of the time.

That sounds like an issue in the driver itself, no? If the clock phandle
is present it sounds like the driver should be claiming the clock
whether a frequency is specified or not. If so, that should be fixed
whether this patch gets applied or not.

> 
> But "anyOf" doesn't prevent these two properties from coexisting, as it
> considers the object valid as long as there's at LEAST one match.
> 
> Let's switch to "oneOf" and disallows the other property if one exists,
> exclusively matching the original binding and avoid future confusion on
> the driver's behavior.

Have you checked whether or not there are devices that have both
in-tree? If there are, can you fix them up as part of the change, rather
than adding new warnings.

> 
> Fixes: e69f5dc623f9 ("dt-bindings: serial: Convert 8250 to json-schema")
> Signed-off-by: Yao Zi <ziyao@...root.org>
> ---
>  Documentation/devicetree/bindings/serial/8250.yaml | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
> index dc0d52920575..4322394f5b8f 100644
> --- a/Documentation/devicetree/bindings/serial/8250.yaml
> +++ b/Documentation/devicetree/bindings/serial/8250.yaml
> @@ -45,9 +45,13 @@ allOf:
>                    - ns16550
>                    - ns16550a
>      then:
> -      anyOf:
> -        - required: [ clock-frequency ]
> -        - required: [ clocks ]
> +      oneOf:
> +        - allOf:

Why is the allOf needed here? Does
oneOf:
  - required: foo
  - required: bar
not work? There's a bunch of bindings doing that, so not sure why it
doesn't work in your case.

Cheers,
Conor.

> +            - required: [ clock-frequency ]
> +            - properties: { clocks: false }
> +        - allOf:
> +            - required: [ clocks ]
> +            - properties: { clock-frequency: false }
>  
>  properties:
>    compatible:
> -- 
> 2.49.0
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)