linux-kernel - Re: [PATCH v2 3/3] mfd: syscon: Allow syscon nodes without a "syscon" compatible

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]

Message-ID: <CAL_JsqKVydDqmr-gyn1wt=1PO_p1NMze+1D+4qXWxGiGVLV9EA@mail.gmail.com>
Date: Fri, 24 Jan 2025 14:03:14 -0600
From: Rob Herring <robh@...nel.org>
To: Vaishnav Achath <vaishnav.a@...com>
Cc: Lee Jones <lee@...nel.org>, Arnd Bergmann <arnd@...db.de>, 
	Pankaj Dubey <pankaj.dubey@...sung.com>, Heiko Stuebner <heiko@...ech.de>, 
	Liviu Dudau <liviu.dudau@....com>, Sudeep Holla <sudeep.holla@....com>, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Peter Griffin <peter.griffin@...aro.org>, 
	Will McVicker <willmcvicker@...gle.com>, John Madieu <john.madieu.xa@...renesas.com>, 
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, Andrew Davis <afd@...com>, 
	Nishanth Menon <nm@...com>, Vignesh Raghavendra <vigneshr@...com>, "Kumar, Udit" <u-kumar1@...com>
Subject: Re: [PATCH v2 3/3] mfd: syscon: Allow syscon nodes without a "syscon" compatible

On Wed, Jan 22, 2025 at 3:44 AM Vaishnav Achath <vaishnav.a@...com> wrote:
>
> Hi Rob,
>
> On 17/12/24 23:41, Rob Herring (Arm) wrote:
> > of_syscon_register_regmap() was added for nodes which need a custom
> > regmap setup. It's not really correct for those nodes to claim they are
> > compatible with "syscon" as the default handling likely doesn't work in
> > those cases. If device_node_get_regmap() happens to be called first,
> > then of_syscon_register() will be called and an incorrect regmap will be
> > created (barring some other error). That may lead to unknown results in
> > the worst case. In the best case, of_syscon_register_regmap() will fail
> > with -EEXIST. This problem remains unless these cases drop "syscon" (an
> > ABI issue) or we exclude them using their specific compatible. ATM,
> > there is only one user: "google,gs101-pmu"
> >
> > There are also cases of adding "syscon" compatible to existing nodes
> > after the fact in order to register the syscon. That presents a
> > potential DT ABI problem. Instead, if there's a kernel change needing a
> > syscon for a node, then it should be possible to allow the kernel to
> > register a syscon without a DT change. That's only possible by using
> > of_syscon_register_regmap() currently, but in the future we may want to
> > support a match list for cases which don't need a custom regmap.
> >
> > With this change, the lookup functions will succeed for any node
> > registered by of_syscon_register_regmap() regardless of whether the node
> > compatible contains "syscon".
> >
> > Signed-off-by: Rob Herring (Arm) <robh@...nel.org>
> > ---
> > v2:
> >   - Fix logic when a syscon is found in list to not return an error
> > ---
> >   drivers/mfd/syscon.c | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > index bfb1f69fcff1d3cd35cf04ccd4c449e7d0395c79..226915ca3c93dcaf47bdd46b58e00e10e155f952 100644
> > --- a/drivers/mfd/syscon.c
> > +++ b/drivers/mfd/syscon.c
> > @@ -171,9 +171,12 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
> >                       break;
> >               }
> >
> > -     if (!syscon)
> > -             syscon = of_syscon_register(np, check_res);
> > -
> > +     if (!syscon) {
> > +             if (of_device_is_compatible(np, "syscon"))
> > +                     syscon = of_syscon_register(np, check_res);
> > +             else
> > +                     syscon = ERR_PTR(-EINVAL);
> > +     }
>
> Earlier the above check was only in syscon_node_to_regmap() , but now
> the users of device_node_to_regmap() also undergoes this check and there
> are few drivers in tree which uses device_node_to_regmap() without
> having "syscon" in compatible, likely those drivers need to be fixed,
> but this patch breaks all those drivers now, atleast the below drivers
> are affected and all TI platforms fail to boot due to this:
>
> drivers/soc/ti/k3-socinfo.c
> drivers/mux/mmio.c ("reg-mux" users)
> drivers/clk/keystone/syscon-clk.c
>
> All the above drivers fail due to the above check for syscon compatible.
>
> What is the recommended solution to fix this?

I've sent a fix[1]. That's not to say I don't agree with fixes to
drivers removing device_node_to_regmap() calls. If you just need a
regmap for within a single driver, then just use regmap APIs directly.
What's in syscon.c is for (surprise!) syscon's, and those have the
characteristic of being shared.

Rob

[1] https://lore.kernel.org/all/20250124191644.2309790-1-robh@kernel.org/