linux-kernel - Re: [PATCH 4/6] clk: renesas: rzv2h: Add fixed-factor module clocks with status reporting

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: <CAMuHMdWsA4mC+D8ftx74_XeuBjpv-9EQN0rgVLPsxjmrO3+rWg@mail.gmail.com>
Date: Thu, 26 Jun 2025 15:22:10 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Prabhakar <prabhakar.csengg@...il.com>
Cc: Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Magnus Damm <magnus.damm@...il.com>, linux-renesas-soc@...r.kernel.org, 
	linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH 4/6] clk: renesas: rzv2h: Add fixed-factor module clocks
 with status reporting

Hi Prabhakar,

On Tue, 24 Jun 2025 at 19:30, Prabhakar <prabhakar.csengg@...il.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
>
> Add support for fixed-factor module clocks that can report their enable
> status through the module status monitor. Introduce a new clock type,
> CLK_TYPE_FF_MOD_STATUS, and define the associated structure,
> rzv2h_ff_mod_status_clk, to manage these clocks.
>
> Implement the .is_enabled callback by reading the module status register
> using monitor index and bit definitions. Provide a helper macro,
> DEF_FIXED_MOD_STATUS, to simplify the definition of such clocks.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>

Thanks for your patch!

One early review comment below...

> --- a/drivers/clk/renesas/rzv2h-cpg.c
> +++ b/drivers/clk/renesas/rzv2h-cpg.c

> +static struct clk_ops rzv2h_clk_ff_mod_status_ops;

This is an empty block of 200 bytes, consuming memory even when running
on a different platform.

> +static struct clk * __init
> +rzv2h_cpg_fixed_mod_status_clk_register(const struct cpg_core_clk *core,
> +                                       struct rzv2h_cpg_priv *priv)
> +{
> +       struct rzv2h_ff_mod_status_clk *clk_hw_data;
> +       struct clk_init_data init = { };
> +       struct clk_fixed_factor *fix;
> +       const struct clk *parent;
> +       const char *parent_name;
> +       int ret;
> +
> +       WARN_DEBUG(core->parent >= priv->num_core_clks);
> +       parent = priv->clks[core->parent];
> +       if (IS_ERR(parent))
> +               return ERR_CAST(parent);
> +
> +       parent_name = __clk_get_name(parent);
> +       parent = priv->clks[core->parent];
> +       if (IS_ERR(parent))
> +               return ERR_CAST(parent);
> +
> +       clk_hw_data = devm_kzalloc(priv->dev, sizeof(*clk_hw_data), GFP_KERNEL);
> +       if (!clk_hw_data)
> +               return ERR_PTR(-ENOMEM);
> +
> +       clk_hw_data->priv = priv;
> +       clk_hw_data->conf = core->cfg.fixed_mod;
> +
> +       rzv2h_clk_ff_mod_status_ops = clk_fixed_factor_ops;

This overwrites rzv2h_clk_ff_mod_status_ops in every call (currently
there is only one).

> +       rzv2h_clk_ff_mod_status_ops.is_enabled = rzv2h_clk_ff_mod_status_is_enabled;

If there would be multiple calls, there is a short time window where
rzv2h_clk_ff_mod_status_ops.is_enabled is NULL, possibly affecting
already-registered clocks of the same type.

Hence I think you better store rzv2h_clk_ff_mod_status_ops inside
rzv2h_cpg_priv (so it is allocated dynamically), and initialize it from
rzv2h_cpg_probe (so it is initialized once).

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds