| 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
| ||
|
Message-ID: <f6cd735b-2ba4-7456-684d-b20f0b46effe@collabora.com>
Date: Mon, 23 May 2022 12:04:37 +0200
From: AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>
To: Chen-Yu Tsai <wenst@...omium.org>,
Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>
Cc: Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Chun-Jie Chen <chun-jie.chen@...iatek.com>,
Miles Chen <miles.chen@...iatek.com>,
linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/4] clk: mediatek: mux: add clk notifier functions
Il 23/05/22 10:59, Chen-Yu Tsai ha scritto:
> With device frequency scaling, the mux clock that (indirectly) feeds the
> device selects between a dedicated PLL, and some other stable clocks.
>
> When a clk rate change is requested, the (normally) upstream PLL is
> reconfigured. It's possible for the clock output of the PLL to become
> unstable during this process.
>
> To avoid causing the device to glitch, the mux should temporarily be
> switched over to another "stable" clock during the PLL rate change.
> This is done with clk notifiers.
>
> This patch adds common functions for notifiers to temporarily and
> transparently reparent mux clocks.
>
> This was loosely based on commit 8adfb08605a9 ("clk: sunxi-ng: mux: Add
> clk notifier functions").
>
> Signed-off-by: Chen-Yu Tsai <wenst@...omium.org>
> ---
> drivers/clk/mediatek/clk-mux.c | 42 ++++++++++++++++++++++++++++++++++
> drivers/clk/mediatek/clk-mux.h | 15 ++++++++++++
> 2 files changed, 57 insertions(+)
>
> diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
> index cd5f9fd8cb98..f84a5a753c09 100644
> --- a/drivers/clk/mediatek/clk-mux.c
> +++ b/drivers/clk/mediatek/clk-mux.c
> @@ -4,6 +4,7 @@
> * Author: Owen Chen <owen.chen@...iatek.com>
> */
>
> +#include <linux/clk.h>
> #include <linux/clk-provider.h>
> #include <linux/compiler_types.h>
> #include <linux/container_of.h>
> @@ -259,4 +260,45 @@ void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
> }
> EXPORT_SYMBOL_GPL(mtk_clk_unregister_muxes);
>
> +/*
> + * This clock notifier is called when the frequency of the of the parent
> + * PLL clock is to be changed. The idea is to switch the parent to a
> + * stable clock, such as the main oscillator, while the PLL frequency
> + * stabilizes.
> + */
> +static int mtk_clk_mux_notifier_cb(struct notifier_block *nb,
> + unsigned long event, void *_data)
> +{
> + struct clk_notifier_data *data = _data;
> + struct mtk_mux_nb *mux_nb = to_mtk_mux_nb(nb);
> + const struct mtk_mux *mux = mux_nb->mux;
> + struct clk_hw *hw;
> + int ret = 0;
> +
> + hw = __clk_get_hw(data->clk);
> +
> + switch (event) {
> + case PRE_RATE_CHANGE:
> + mux_nb->original_index = mux->ops->get_parent(hw);
> + ret = mux->ops->set_parent(hw, mux_nb->bypass_index);
> + break;
> +
> + case POST_RATE_CHANGE:
> + case ABORT_RATE_CHANGE:
I agree with this change, entirely - but there's an issue here.
If we enter ABORT_RATE_CHANGE, this means that "something has failed": now,
what if the failure point was the PLL being unable to lock?
In that case, we would switch the parent back to a PLL that's not outputting
any clock, crashing the GPU, or a bogus rate, potentially undervolting the GPU.
I think that the best idea here would be to do something like..
switch (event) {
case PRE_RATE_CHANGE:
mux_nb->old_parent_idx = mux->ops->get_parent(hw);
ret = mux->ops->set_parent(hw, mux_nb->safe_parent_idx);
break;
case POST_RATE_CHANGE:
ret = mux->ops->set_parent(hw, mux_nb->old_parent_idx);
break;
case ABORT_RATE_CHANGE:
ret = -EINVAL; /* or -ECANCELED, whatever... */
break;
}
> + ret = mux->ops->set_parent(hw, mux_nb->original_index);
> + break;
> + }
> +
> + return notifier_from_errno(ret);
> +}
> +
> +int devm_mtk_clk_mux_notifier_register(struct device *dev, struct clk *clk,
> + struct mtk_mux_nb *mux_nb)
> +{
> + mux_nb->nb.notifier_call = mtk_clk_mux_notifier_cb;
> +
> + return devm_clk_notifier_register(dev, clk, &mux_nb->nb);
> +}
> +EXPORT_SYMBOL_GPL(devm_mtk_clk_mux_notifier_register);
> +
> MODULE_LICENSE("GPL");
> diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
> index 6539c58f5d7d..506e91125a3d 100644
> --- a/drivers/clk/mediatek/clk-mux.h
> +++ b/drivers/clk/mediatek/clk-mux.h
> @@ -7,12 +7,14 @@
> #ifndef __DRV_CLK_MTK_MUX_H
> #define __DRV_CLK_MTK_MUX_H
>
> +#include <linux/notifier.h>
> #include <linux/spinlock.h>
> #include <linux/types.h>
>
> struct clk;
> struct clk_hw_onecell_data;
> struct clk_ops;
> +struct device;
> struct device_node;
>
> struct mtk_mux {
> @@ -89,4 +91,17 @@ int mtk_clk_register_muxes(const struct mtk_mux *muxes,
> void mtk_clk_unregister_muxes(const struct mtk_mux *muxes, int num,
> struct clk_hw_onecell_data *clk_data);
>
> +struct mtk_mux_nb {
> + struct notifier_block nb;
> + const struct mtk_mux *mux;
> +
> + u8 bypass_index; /* Which parent to temporarily use */
> + u8 original_index; /* Set by notifier callback */
I think that the following names are more explanatory:
u8 safe_parent_idx;
u8 old_parent_idx;
...because I see this as a mechanism to switch the mux to a "safe" clock output
and then back to the PLL (like it's done on some qcom clocks as well).
You're free to ignore this comment, as this is, of course, just a personal opinion.
Cheers,
Angelo
Powered by blists - more mailing lists