Message-ID: <3364c2b6d3c1f22e1310464ae2a6f9a4.sboyd@kernel.org>
Date: Wed, 26 Feb 2025 15:43:22 -0800
From: Stephen Boyd <sboyd@...nel.org>
To: Claudiu Beznea <claudiu.beznea@...on.dev>, Michael Turquette <mturquette@...libre.com>
Cc: linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org, Bjorn Andersson <bjorn.andersson@...aro.org>, Doug Anderson <dianders@...omium.org>
Subject: Re: [PATCH 9/9] clk: Overwrite clk_hw::init with NULL during clk_register()

Quoting Claudiu Beznea (2025-02-15 05:55:51)
> Hi, Stephen,
> 
> On 31.07.2019 22:35, Stephen Boyd wrote:
> > We don't want clk provider drivers to use the init structure after clk
> > registration time, but we leave a dangling reference to it by means of
> > clk_hw::init. Let's overwrite the member with NULL during clk_register()
> > so that this can't be used anymore after registration time.
> > 
> > Cc: Bjorn Andersson <bjorn.andersson@...aro.org>
> > Cc: Doug Anderson <dianders@...omium.org>
> > Signed-off-by: Stephen Boyd <sboyd@...nel.org>
> > ---
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index c0990703ce54..efac620264a2 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3566,6 +3566,14 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
> >  {
> >       int ret;
> >       struct clk_core *core;
> > +     const struct clk_init_data *init = hw->init;
> > +
> > +     /*
> > +      * The init data is not supposed to be used outside of registration path.
> > +      * Set it to NULL so that provider drivers can't use it either and so that
> > +      * we catch use of hw->init early on in the core.
> > +      */
> > +     hw->init = NULL;
> 
> I found that this line impact the drivers/clk/clk-versaclock3.c driver when
> doing unbind/bind. The driver provides statically initialized hw.init
> object for registration, the hw.init pointers are set to NULL after clock
> registrations then the next registration (when re-binding) fails.

That's nice </sarcasm>. Yeah I suspect a lot of drivers would fail if
they're unbound and rebound right now, for various reasons including
what you mention. I think Doug or Bjorn was concerned that this pointer
was left dangling when it could potentially lead to some driver
accessing the stack. That's what started the whole series.

> diff --git a/drivers/clk/clk-versaclock3.c b/drivers/clk/clk-versaclock3.c
> index 9fe27dace111..c1776c313e9b 100644
> --- a/drivers/clk/clk-versaclock3.c
> +++ b/drivers/clk/clk-versaclock3.c
> @@ -1068,7 +1069,7 @@ static int vc3_probe(struct i2c_client *client)
>                 ret = devm_clk_hw_register(dev, &clk_div[i].hw);
>                 if (ret)
>                         return dev_err_probe(dev, ret, "%s failed\n",
> -                                            clk_div[i].hw.init->name);
> +                                            "test");
> //clk_div[i].hw.init->name);
>         }
> 
>         /* Register clk muxes */
> @@ -1082,7 +1083,7 @@ static int vc3_probe(struct i2c_client *client)
>                 ret = devm_clk_hw_register(dev, &clk_mux[i].hw);
>                 if (ret)
>                         return dev_err_probe(dev, ret, "%s failed\n",
> -                                            clk_mux[i].hw.init->name);
> +
> "test");//clk_mux[i].hw.init->name);
>         }
> 
>         /* Register clk outputs */
> 
> Can you please let me know if this is a wrong pattern? I noticed there are
> other drivers following similar approach, e.g.:
> - drivers/clk/clk-axm5516.c
> - drivers/clk/qcom/camcc-sm6350.c
> - drivers/clk/meson/c3-pll.c

Maybe we should only overwrite it to NULL when it succeeds. Or
inversely, restore the pointer when it fails so that drivers can print
the init->name. Of course, that doesn't fix the problem that a driver
may be stashing the init pointer in there anonymously, like the qcom
driver does.

Actually, I think what we want to do is pull that error message into
devm_clk_hw_register() and get rid of it everywhere else. Drivers will
still have to restore the pointer before registering if the driver is
unbound and bound. It isn't good to keep a pointer to the stack so I'd
like to keep the logic that knocks out the pointer.

---8<---
From: Stephen Boyd <sboyd@...nel.org>
Subject: [PATCH] clk: Print an error when clk registration fails

We have a lot of driver code that prints an error message when
registering a clk fails. Do that in the core function instead to
consolidate code. This also helps drivers avoid the anti-pattern of
accessing the struct clk_hw::init pointer after registration.

Signed-off-by: Stephen Boyd <sboyd@...nel.org>
---
 drivers/clk/clk.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 50faafbf5dda..88edeae7a1f4 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4397,6 +4397,13 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 fail_name:
 	kref_put(&core->ref, __clk_release);
 fail_out:
+	if (dev) {
+		dev_err_probe(dev, ret, "failed to register clk '%s' (%pS)\n",
+			      init->name, hw);
+	} else {
+		pr_err("%pOF: error %pe: failed to register clk '%s' (%pS)\n",
+		       np, &ret, init->name, hw);
+	}
 	return ERR_PTR(ret);
 }
 
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git

Powered by blists - more mailing lists