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: <596b4d23059e14a89c6817123bfc9a020c28d839.camel@linaro.org>
Date: Fri, 09 Aug 2024 09:50:29 +0100
From: André Draszik <andre.draszik@...aro.org>
To: Stephen Boyd <sboyd@...nel.org>, Abel Vesa <abelvesa@...nel.org>, Alim
 Akhtar <alim.akhtar@...sung.com>, Chanwoo Choi <cw00.choi@...sung.com>,
 Fabio Estevam <festevam@...il.com>, Krzysztof Kozlowski <krzk@...nel.org>,
 Michael Turquette <mturquette@...libre.com>, Peng Fan <peng.fan@....com>,
 Pengutronix Kernel Team <kernel@...gutronix.de>, Peter Griffin
 <peter.griffin@...aro.org>, Sam Protsenko <semen.protsenko@...aro.org>,
 Sascha Hauer <s.hauer@...gutronix.de>, Shawn Guo <shawnguo@...nel.org>,
 Sylwester Nawrocki <s.nawrocki@...sung.com>, Tudor Ambarus
 <tudor.ambarus@...aro.org>
Cc: Will McVicker <willmcvicker@...gle.com>, kernel-team@...roid.com, 
	linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org, 
	imx@...ts.linux.dev
Subject: Re: [PATCH v6 01/20] clk: bump stdout clock usage for earlycon

Hi Stephen,

On Thu, 2024-08-08 at 13:14 -0700, Stephen Boyd wrote:
> > +static struct of_clk_stdout_clks {
> > +       bool bump_refs;
> > +
> > +       struct mutex lock;
> > +       bool have_all;
> > +       struct clk **clks;
> > +       size_t n_clks;
> > +} of_clk_stdout_clks = {
> 
> This can be initdata?

With the __ref wrapper you suggested below that's indeed possible. Without,
it's not, and I wasn't aware of __ref. Thanks for the pointer!

> > +       .lock = __MUTEX_INITIALIZER(of_clk_stdout_clks.lock),
> > +};
> > +
> > +static int __init of_clk_bump_stdout_clocks_param(char *str)
> > +{
> > +       of_clk_stdout_clks.bump_refs = true;
> > +       return 0;
> > +}
> > +__setup("earlycon", of_clk_bump_stdout_clocks_param);
> > +__setup_param("earlyprintk", of_clk_keep_stdout_clocks_earlyprintk,
> > +             of_clk_bump_stdout_clocks_param, 0);
> > +
> > +static void of_clk_bump_stdout_clocks(void)
> 
> This can be __init?

dito.

Cheers,
Andre'

> 
> > +{
> > +       size_t n_clks;
> > +
> > +       /*
> > +        * We only need to run this code if required to do so and only ever
> > +        * before late initcalls have run. Otherwise it'd be impossible to know
> > +        * when to drop the extra clock references again.
> > +        *
> > +        * This generally means that this only works if on affected platforms
> > +        * the clock drivers have been built-in (as opposed to being modules).
> > +        */
> > +       if (!of_clk_stdout_clks.bump_refs)
> > +               return;
> > +
> > +       n_clks = of_clk_get_parent_count(of_stdout);
> > +       if (!n_clks || !of_stdout)
> > +               return;
> > +
> > +       mutex_lock(&of_clk_stdout_clks.lock);
> > +
> > +       /*
> > +        * We only need to keep trying if we have not succeeded previously,
> > +        * i.e. if not all required clocks were ready during previous attempts.
> > +        */
> > +       if (of_clk_stdout_clks.have_all)
> > +               goto out_unlock;
> > +
> > +       if (!of_clk_stdout_clks.clks) {
> > +               of_clk_stdout_clks.n_clks = n_clks;
> > +
> > +               of_clk_stdout_clks.clks = kcalloc(of_clk_stdout_clks.n_clks,
> > +                                             sizeof(*of_clk_stdout_clks.clks),
> > +                                             GFP_KERNEL);
> > +               if (!of_clk_stdout_clks.clks)
> > +                       goto out_unlock;
> > +       }
> > +
> > +       /* assume that this time we'll be able to grab all required clocks */
> > +       of_clk_stdout_clks.have_all = true;
> > +       for (size_t i = 0; i < n_clks; ++i) {
> > +               struct clk *clk;
> > +
> > +               /* we might have grabbed this clock in a previous attempt */
> > +               if (of_clk_stdout_clks.clks[i])
> > +                       continue;
> > +
> > +               clk = of_clk_get(of_stdout, i);
> > +               if (IS_ERR(clk)) {
> > +                       /* retry next time if clock has not probed yet */
> > +                       of_clk_stdout_clks.have_all = false;
> > +                       continue;
> > +               }
> > +
> > +               if (clk_prepare_enable(clk)) {
> > +                       clk_put(clk);
> > +                       continue;
> > +               }
> > +               of_clk_stdout_clks.clks[i] = clk;
> > +       }
> > +
> > +out_unlock:
> > +       mutex_unlock(&of_clk_stdout_clks.lock);
> > +}
> > +
> > +static int __init of_clk_drop_stdout_clocks(void)
> > +{
> > +       for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
> > +               clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
> > +               clk_put(of_clk_stdout_clks.clks[i]);
> > +       }
> > +
> > +       kfree(of_clk_stdout_clks.clks);
> > +
> > +       /*
> > +        * Do not try to acquire stdout clocks after late initcalls, e.g.
> > +        * during further module loading, as we then wouldn't have a way to
> > +        * drop the references (and associated allocations) ever again.
> > +        */
> > +       of_clk_stdout_clks.bump_refs = false;
> > +
> > +       return 0;
> > +}
> > +late_initcall_sync(of_clk_drop_stdout_clocks);
> > +
> >  /**
> >   * struct of_clk_provider - Clock provider registration structure
> >   * @link: Entry in global list of clock providers
> > @@ -5031,6 +5156,8 @@ int of_clk_add_provider(struct device_node *np,
> >  
> >         fwnode_dev_initialized(&np->fwnode, true);
> >  
> > +       of_clk_bump_stdout_clocks();
> 
> This can be a wrapper function that isn't marked __init but which calls
> the init function with __ref. That lets us free up as much code as
> possible. We need to set a bool in of_clk_drop_stdout_clocks() that when
> false doesn't call the __init functions that are wrapped though, i.e.
> 'bump_refs'. Here's the structure:
> 
> 	static bool bump_stdout_clks __ro_after_init = true;
> 
> 	static int __init _of_clk_bump_stdout_clks(void)
> 	{
> 		...
> 	}
> 
> 	static int __ref of_clk_bump_stdout_clks(void)
> 	{
> 		if (bump_stdout_clks)
> 			return _of_clk_bump_stdout_clks();
> 
> 		return 0;
> 	}
> 
> 	static int __init of_clk_drop_stdout_clks(void)
> 	{
> 		bump_stdout_clks = false;
> 		...
> 	}
> 	late_initcall_sync(of_clk_drop_stdout_clks);
> 
> > +
> >         return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(of_clk_add_provider);

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ