| 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
| ||
|
Message-ID: <d421a6e8-e72c-48d9-8806-09724723b5d8@kernel.org> Date: Tue, 13 May 2025 15:31:30 +0200 From: Krzysztof Kozlowski <krzk@...nel.org> To: Dario Binacchi <dario.binacchi@...rulasolutions.com>, Rob Herring <robh@...nel.org> Cc: linux-kernel@...r.kernel.org, Peng Fan <peng.fan@....com>, Stephen Boyd <sboyd@...nel.org>, Shawn Guo <shawnguo@...nel.org>, linux-amarula@...rulasolutions.com, Abel Vesa <abelvesa@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, Conor Dooley <conor+dt@...nel.org>, Fabio Estevam <festevam@...il.com>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Michael Turquette <mturquette@...libre.com>, Pengutronix Kernel Team <kernel@...gutronix.de>, Sascha Hauer <s.hauer@...gutronix.de>, devicetree@...r.kernel.org, imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org Subject: Re: [PATCH v12 16/19] dt-bindings: clock: imx8m-clock: add PLLs On 09/05/2025 20:22, Dario Binacchi wrote: > On Fri, May 9, 2025 at 6:25 PM Rob Herring <robh@...nel.org> wrote: >> >> On Thu, Apr 24, 2025 at 08:21:46AM +0200, Dario Binacchi wrote: >>> Though adding the PLLs to clocks and clock-names properties will break >>> the ABI, it is required to accurately describe the hardware. Indeed, >>> the Clock Control Module (CCM) receives clocks from the PLLs and >>> oscillators and generates clocks for on-chip peripherals. >>> >>> Signed-off-by: Dario Binacchi <dario.binacchi@...rulasolutions.com> >>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org> >>> >>> --- >>> >>> (no changes since v11) >>> >>> Changes in v11: >>> - Fix conflict while rebasing on master >>> >>> Changes in v7: >>> - Add 'Reviewed-by' tag of Krzysztof Kozlowski >>> >>> Changes in v6: >>> - New >>> >>> .../bindings/clock/imx8m-clock.yaml | 27 ++++++++++++++----- >>> 1 file changed, 21 insertions(+), 6 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml >>> index 4fec55832702..e83f08abd44c 100644 >>> --- a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml >>> +++ b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml >>> @@ -29,12 +29,12 @@ properties: >>> maxItems: 2 >>> >>> clocks: >>> - minItems: 6 >>> - maxItems: 7 >>> + minItems: 7 >> >> Increasing the minimum entries looks like an ABI break to me. The .dts >> files not being in linux-next confirms that (from 0 warnings in >> mainline): >> >> arch/arm64/boot/dts/freescale:859:50 >> 122 clock-controller@...80000 (fsl,imx8mm-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short >> 120 clock-controller@...80000 (fsl,imx8mp-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short >> 61 clock-controller@...60000 (fsl,imx8mm-anatop): 'clocks' is a required property >> 61 clock-controller@...60000 (fsl,imx8mm-anatop): 'clock-names' is a required property >> 60 clock-controller@...60000 (fsl,imx8mp-anatop): 'clocks' is a required property >> 60 clock-controller@...60000 (fsl,imx8mp-anatop): 'clock-names' is a required property >> 36 clock-controller@...80000 (fsl,imx8mp-ccm): clocks: [[35], [36], [37], [38], [39], [40]] is too short >> 36 clock-controller@...80000 (fsl,imx8mm-ccm): clocks: [[24], [25], [26], [27], [28], [29]] is too short >> 32 clock-controller@...80000 (fsl,imx8mp-ccm): clocks: [[34], [35], [36], [37], [38], [39]] is too short >> 28 clock-controller@...80000 (fsl,imx8mm-ccm): clocks: [[22], [23], [24], [25], [26], [27]] is too short >> 26 clock-controller@...80000 (fsl,imx8mn-ccm): clock-names: ['osc_32k', 'osc_24m', 'clk_ext1', 'clk_ext2', 'clk_ext3', 'clk_ext4'] is too short >> 17 clock-controller@...60000 (fsl,imx8mq-anatop): 'clocks' is a required property >> 17 clock-controller@...60000 (fsl,imx8mq-anatop): 'clock-names' is a required property >> 14 clock-controller@...80000 (fsl,imx8mp-ccm): clocks: [[44], [45], [46], [47], [48], [49]] is too short >> 14 clock-controller@...80000 (fsl,imx8mm-ccm): clocks: [[23], [24], [25], [26], [27], [28]] is too short >> 13 clock-controller@...60000 (fsl,imx8mn-anatop): 'clocks' is a required property >> 13 clock-controller@...60000 (fsl,imx8mn-anatop): 'clock-names' is a required property >> 12 clock-controller@...80000 (fsl,imx8mm-ccm): clocks: [[26], [27], [28], [29], [30], [31]] is too short >> 10 clock-controller@...80000 (fsl,imx8mp-ccm): clocks: [[38], [39], [40], [41], [42], [43]] is too short >> 8 clock-controller@...80000 (fsl,imx8mn-ccm): clocks: [[22], [23], [24], [25], [26], [27]] is too short >> 8 clock-controller@...80000 (fsl,imx8mn-ccm): clocks: [[20], [21], [22], [23], [24], [25]] is too short >> 8 clock-controller@...80000 (fsl,imx8mm-ccm): clocks: [[34], [35], [36], [37], [38], [39]] is too short >> 8 clock-controller@...80000 (fsl,imx8mm-ccm): clocks: [[28], [29], [30], [31], [32], [33]] is too short >> 8 bcrmf@1 (brcm,bcm4329-fmac): $nodename:0: 'bcrmf@1' does not match '^wifi(@.*)?$' >> 6 clock-controller@...80000 (fsl,imx8mp-ccm): clocks: [[41], [42], [43], [44], [45], [46]] is too short >> 6 clock-controller@...80000 (fsl,imx8mn-ccm): clocks: [[24], [25], [26], [27], [28], [29]] is too short >> 4 clock-controller@...80000 (fsl,imx8mp-ccm): clocks: [[43], [44], [45], [46], [47], [48]] is too short >> 4 clock-controller@...80000 (fsl,imx8mp-ccm): clocks: [[40], [41], [42], [43], [44], [45]] is too short >> 4 clock-controller@...80000 (fsl,imx8mp-ccm): clocks: [[36], [37], [38], [39], [40], [41]] is too short >> 4 clock-controller@...80000 (fsl,imx8mm-ccm): clocks: [[35], [36], [37], [38], [39], [40]] is too short >> >> Please fix the binding or drop what's been applied so far. > > Abel and Shawn are already aware of the issue: ... and I objected to that ABI break: https://lore.kernel.org/linux-devicetree/gbymcmoya7dfmedq4nkopqpswh63d2ujxl2elc2x7x325b75bu@anp36sdya43v/ to which you replied "correct hardware description". Now I admit, I should not give the review tag, because that's really poor reason to: 1. break ABI 2. break users 3. break boot as it turns out I have impression that at later stage I actually NAKed entire patchset on the basis of being obsolete... but never mind. I am quite picky reviewer but once I cut people slack, some poor explanation about ABI beak is added to commit msg but no one else cares and it bites back. We keep repeating that it is the platform's maintainer decision but apparently still people don't know it. Quite a lesson for me. Anyway, I am dissapointed, regardless of my review, that this patchset could not actually keep backward compatibility. My recommendation to Abel is to drop entire patchset. If there is going to be any re-submits (v13), please kindly drop my Reviewed-by from this and next patch breaking the ABI. Best regards, Krzysztof
Powered by blists - more mailing lists