Re: [PATCH 14/22] clk: Introduce clk_hw_init_rate_request()

[Stephen Boyd <sboyd@kernel.org>]

Quoting Maxime Ripard (2022-04-08 02:10:29)
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 399080456e45..83dd5f1df0b9 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1396,6 +1396,26 @@ static void clk_core_init_rate_req(struct clk_core * const core,
>         }
>  }
>  
> +/**
> + * clk_hw_init_rate_request - Initializes a clk_rate_request
> + * @hw: the clk for which we want to submit a rate request
> + * @req: the clk_rate_request structure we want to initialise
> + * @rate: the rate which is to be requested
> + *
> + * Initializes and fills a clk_rate_request structure to submit to

s/and fills//

> + * __clk_determine_rate or similar functions.

__clk_determine_rate()

> + */
> +void clk_hw_init_rate_request(struct clk_hw * const hw,

I don't get why it isn't 'const struct clk_hw *hw', but it looks to be
following clk_core_init_rate_req() so that can be figured out later.
Please remove the const from here regardless; it's not doing anything.

> +                             struct clk_rate_request *req,
> +                             unsigned long rate)
> +{
> +       if (WARN_ON(!hw || !req))

Why would you call it without those two items? Another copy/paste from
clk_core_init_rate_req()?

> +               return;
> +
> +       clk_core_init_rate_req(hw->core, req, rate);
> +}
> +EXPORT_SYMBOL_GPL(clk_hw_init_rate_request);
> +
>  static bool clk_core_can_round(struct clk_core * const core)
>  {
>         return core->ops->determine_rate || core->ops->round_rate;
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index c10dc4c659e2..39e4ed301ec5 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -42,6 +42,8 @@ struct dentry;
>   * struct clk_rate_request - Structure encoding the clk constraints that
>   * a clock user might require.
>   *
> + * Should be initialized by calling clk_hw_init_rate_request().

How is that enforced?

This has only become a problem after commit 948fb0969eae ("clk: Always
clamp the rounded rate") from what I can tell. I guess we can't skip the
clamp when both min/max are zero though because it may be stack junk?
But I looked at all the call sites and either they zero initialize the
whole struct (qcom) or they copy the req from what is passed into the
determine_rate clk_op (others). So we could simply not clamp if both
values are equal to zero and then qcom would be happy, but that has been
fixed by setting the min/max to 0 and max instead. That leaves the other
users, which already copy what is being passed in, i.e. what is done by
clk_core_init_rate_req().

I guess my question is who is going to use this? And if we can't even
enforce that it is used then it feels like we shouldn't add it. Maybe it
can be useful to cleanup the core request initialization logic because
it's sort of spread out but probably not as a clk_hw API.

> + *
>   * @rate:              Requested clock rate. This field will be adjusted by
>   *                     clock drivers according to hardware capabilities.
>   * @min_rate:          Minimum rate imposed by clk users.