linux-kernel - Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers

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: <CAMhs-H9BkQNwNyYQQgSig9vkW-_+i0+x8CLogXNgkRyi9Un6xw@mail.gmail.com>
Date:   Mon, 13 Feb 2023 09:59:35 +0100
From:   Sergio Paracuellos <sergio.paracuellos@...il.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Arınç ÜNAL <arinc.unal@...nc9.com>,
        linux-watchdog@...r.kernel.org, wim@...ux-watchdog.org,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        matthias.bgg@...il.com, tsbogend@...ha.franken.de,
        p.zabel@...gutronix.de, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-mips@...r.kernel.org
Subject: Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to
 access system controller registers

On Sun, Feb 12, 2023 at 4:27 PM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On 2/12/23 00:13, Sergio Paracuellos wrote:
> > On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@...aro.org> wrote:
> >>
> >> On 11/02/2023 12:01, Sergio Paracuellos wrote:
> >>> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@...nc9.com> wrote:
> >>>>
> >>>> On 11.02.2023 13:41, Sergio Paracuellos wrote:
> >>>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@...nc9.com> wrote:
> >>>>>>
> >>>>>> Is this mediatek,sysctl property required after your changes on the
> >>>>>> watchdog code?
> >>>>>
> >>>>> I don't really understand the question :-) Yes, it is. Since we have
> >>>>> introduced a new phandle in the watchdog node to be able to access the
> >>>>> reset status register through the 'sysc' syscon node.
> >>>>> We need the bindings to be aligned with the mt7621.dtsi file and we
> >>>>> are getting the syscon regmap handler via
> >>>>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç.
> >>>>
> >>>> I believe you need to put mediatek,sysctl under "required:".
> >>>
> >>> Ah, I understood your question now :-). You meant 'required' property.
> >>> I need more coffee, I guess :-). I am not sure if you can add
> >>> properties as required after bindings are already mainlined for
> >>> compatibility issues. The problem with this SoC is that drivers become
> >>> mainlined before the device tree was so if things are properly fixed
> >>> now this kind of issues appear.  Let's see Krzysztof and Rob comments
> >>> for this.
> >>
> >> If your driver fails to probe without mediatek,sysctl, you already made
> >> it required (thus broke the ABI) regardless what dt-binding is saying.
> >> In such case you should update dt-binding to reflect reality.
> >>
> >> Now ABI break is different case. Usually you should not break it without
> >> valid reasons (e.g. it was never working before). Your commit msg
> >> suggests that you only improve the code, thus ABI break is not really
> >> justified. In such case - binding is correct, driver should be reworked
> >> to accept DTS without the new property.
> >
> > Thanks for clarification, Krzysztof. Ok, so if this is the case I need
> > to add this property required (as Arinc was properly pointing out in
> > previous mail) since without it the driver is going to fail on probe
> > (PATCH 5 of the series). I understand the "it was never working
> > before" argument reason for ABI breaks. What happens if the old driver
> > code was not ideal and totally dependent on architecture specific
> > operations when this could be totally avoided and properly make arch
> > independent agnostic drivers? This driver was added in 2016 [0]. There
> > was not a device tree file in the kernel for this SoC mainlined until
> > 2022 [1]. I also personally migrated this watchdog binding in 2022
> > from text to YAML and maintained it without changes [2]. When this was
> > mainlined not all drivers were properly reviewed and the current code
> > was just maintained as it is. Most users of this SoC are in the
> > openWRT community where the dtsi of the mainline is not used yet and
> > they maintain their own mt7621.dtsi files. Also, when a new version of
> > the openWRT selected kernel is added they also modify and align with
> > its mt7621.dtsi file without maintaining previous dtb's. If "make the
> > driver arch independent to be able to be compile tested" and this kind
> > of arguments are not valid at all I need to know because I have
> > started to review driver code for this SoC and other drivers also have
> > the same arch dependency that ideally should be avoided in the same
> > way. This at the end means to break the ABI again in the future for
> > those drivers / bindings. So I can just let them be as it is and not
> > provide any change at all and continue without being compile tested
> > and other beneficial features to detect future driver breakage.
> >
>
> Problem is that there are (presumably) shipped systems out there with
> the old devicetree file. The watchdog driver would no longer instantiate
> on those systems.

Ok, I will maintain only the PATCH that changes the driver to not use
globals and send v5.

>
> Guenter
>

Thanks,
    Sergio Paracuellos