Re: [PATCH RESEND 1/2] clk: Implement protected-clocks for all OF clock providers

[Maxime Ripard]

On Wed, Sep 02, 2020 at 11:00:14PM -0500, Samuel Holland wrote:
> This is a generic implementation of the "protected-clocks" property from
> the common clock binding. It allows firmware to inform the OS about
> clocks that must not be disabled while the OS is running.
> 
> This implementation comes with some caveats:
> 
> 1) Clocks that have CLK_IS_CRITICAL in their init data are prepared/
> enabled before they are attached to the clock tree. protected-clocks are
> only protected once the clock provider is added, which is generally
> after all of the clocks it provides have been registered. This leaves a
> window of opportunity where something could disable or modify the clock,
> such as a driver running on another CPU, or the clock core itself. There
> is a comment to this effect in __clk_core_init():
> 
>   /*
>    * Enable CLK_IS_CRITICAL clocks so newly added critical clocks
>    * don't get accidentally disabled when walking the orphan tree and
>    * reparenting clocks
>    */
> 
> Similarly, these clocks will be enabled after they are first reparented,
> unlike other CLK_IS_CRITICAL clocks. See the comment in
> clk_core_reparent_orphans_nolock():
> 
>   /*
>    * We need to use __clk_set_parent_before() and _after() to
>    * to properly migrate any prepare/enable count of the orphan
>    * clock. This is important for CLK_IS_CRITICAL clocks, which
>    * are enabled during init but might not have a parent yet.
>    */
> 
> Ideally we could detect protected clocks before they are reparented, but
> there are two problems with that:
> 
>   a) From the clock core's perspective, hw->init is const.
> 
>   b) The clock core doesn't see the device_node until __clk_register is
>      called on the first clock.
> 
> So the only "race-free" way to detect protected-clocks is to do it in
> the middle of __clk_register, between when core->flags is initialized
> and calling __clk_core_init(). That requires scanning the device tree
> again for each clock, which is part of why I didn't do it that way.
> 
> 2) __clk_protect needs to be idempotent, for two reasons:
> 
>   a) Clocks with CLK_IS_CRITICAL in their init data are already
>      prepared/enabled, and we don't want to prepare/enable them again.
> 
>   b) of_clk_set_defaults() is called twice for (at least some) clock
>      controllers registered with CLK_OF_DECLARE. It is called first in
>      of_clk_add_provider()/of_clk_add_hw_provider() inside clk_init_cb,
>      and again afterward in of_clk_init(). The second call in
>      of_clk_init() may be unnecessary, but verifying that would require
>      auditing all users of CLK_OF_DECLARE to ensure they called one of
>      the of_clk_add{,_hw}_provider functions.
> 
> Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>

Reviewed-by: Maxime Ripard <mripard@xxxxxxxxxx>

Thanks!
Maxime

Attachment: signature.asc
Description: PGP signature