linux-kernel - Aw: Re: BUG: [PATCH v2] isoc: mediatek: Check for error clk pointer

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] [day] [month] [year] [list]

Date:   Wed, 2 Feb 2022 09:09:03 +0100
From:   Frank Wunderlich <frank-w@...lic-files.de>
To:     Daniel Golle <daniel@...rotopia.org>
Cc:     Jiasheng Jiang <jiasheng@...as.ac.cn>, justinkb@...il.com,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        lgirdwood@...il.com, broonie@...nel.org,
        linux-mediatek@...ts.infradead.org, matthias.bgg@...il.com,
        linux-arm-kernel@...ts.infradead.org
Subject: Aw: Re: BUG: [PATCH v2] isoc: mediatek: Check for error clk pointer

> Gesendet: Dienstag, 01. Februar 2022 um 23:19 Uhr
> Von: "Daniel Golle" <daniel@...rotopia.org>
> An: "Jiasheng Jiang" <jiasheng@...as.ac.cn>
> Cc: justinkb@...il.com, gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org, lgirdwood@...il.com, broonie@...nel.org, linux-mediatek@...ts.infradead.org, matthias.bgg@...il.com, linux-arm-kernel@...ts.infradead.org
> Betreff: Re: BUG: [PATCH v2] isoc: mediatek: Check for error clk pointer
>
> On Sun, Jan 30, 2022 at 10:43:35AM +0800, Jiasheng Jiang wrote:
> > On Fri, Jan 28, 2022 at 10:26:51PM +0800, Paul Mulders wrote:
> > > I guess this breaks all MT7622 SoCs since it'll prematurely exit
> > > init_clks (and subsequently init_scp) completely once devm_clk_get
> > > fails to get a reference to the mm clock producer (which happens to be
> > > the first one tried). This is because MT7623 has a GPU (so no mm
> > > clock) and MT7622 doesn't, and as a result the other clock producer
> > > pointers never get initialized (and other stuff in init_scp after
> > > returning from the error never happens).
> > >
> > > The patch seems fundamentally flawed, I guess it was either not tested
> > > at all, or only tested on a MT7623. The initialization functions seem
> > > designed with the idea that it's ok if some clocks aren't present, so
> > > stopping the initialization when one of them isn't present seems
> > > wrong. (For example, there is also a MT7622B variant of the MT7622
> > > which probably also lacks some clocks MT7622(A) does have).
> >
> > I don't think the patch for init_clks() is flawed.
> > At most it is incompleted.
> > What it did is like fixing a potential error in the tool platform
> > providing service for the upper application, like what you said,
> > MT7623 and MT7622.
> > We should not keep the error in the platform because of the upper
> > application.
> > And it seems like it is MT7622 that is flawed.
> > The better way is to fix both the bug in init_clks() and its caller,
> > MT7622.
>
> I agree that further cleaning is needed here.
> Yet the commit in it's current form very obviously breaks at least the
> MT7622 platform if no further fixes are applied.
>
> Imho the whole approach of this driver to hard-code the names of all
> clocks it could try to grab in a string-array accompanied by an enum
> is flawed.
>
> The correct approach would likely be to add the clocks actually present
> to the device tree of each SoC and then grab only those.
>
> I can see that mt2701.dtsi and mt7623.dtsi both have some clocks
> defined for scpsys, mt7622.dtsi however lacks them.
> I didn't check other MediaTek SoCs which also use scpsys.

re-add me to discussion :)

have not yet tested mt7623 without the revert i applied for mt7622. will do this later

regards Frank