| 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: <5192bc94-12c7-dce4-c2e6-fa4b8c1ced9d@gmail.com>
Date: Tue, 31 Jan 2023 14:19:11 +0100
From: Matthias Brugger <matthias.bgg@...il.com>
To: Roger Lu <roger.lu@...iatek.com>,
Enric Balletbo Serra <eballetbo@...il.com>,
Kevin Hilman <khilman@...nel.org>,
Nicolas Boichat <drinkcat@...gle.com>,
Stephen Boyd <sboyd@...nel.org>
Cc: Fan Chen <fan.chen@...iatek.com>,
Jia-wei Chang <jia-wei.chang@...iatek.com>,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org,
Project_Global_Chrome_Upstream_Group@...iatek.com
Subject: Re: [PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs
On 11/01/2023 08:45, Roger Lu wrote:
> In MediaTek HW design, svs and thermal both use the same clk source.
> It means that svs clk reference count from CCF includes thermal control
> counts. That makes svs driver confuse whether it disabled svs's main clk
> or not from CCF's perspective and lead to turn off their shared clk
> unexpectedly. Therefore, we add svs clk control APIs to make sure svs's
> main clk is controlled well by svs driver itself.
>
> Here is a NG example. Rely on CCF's reference count and cause problem.
>
> thermal probe (clk ref = 1)
> -> svs probe (clk ref = 2)
> -> svs suspend (clk ref = 1)
> -> thermal suspend (clk ref = 0)
> -> thermal resume (clk ref = 1)
> -> svs resume (encounter error, clk ref = 1)
> -> svs suspend (clk ref = 0)
> -> thermal suspend (Fail here, thermal HW control w/o clk)
>
> Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare() on err in svs_resume()")
> Signed-off-by: Roger Lu <roger.lu@...iatek.com>
That looks wrong. Although I don't out of my mind, there should be a way to tell
the clock framework that this clock is shared between several devices.
I wonder if using clk_enable and clk_disable in svs_resume/suspend wouldn't be
enough.
Regards,
Matthias
> ---
> drivers/soc/mediatek/mtk-svs.c | 57 ++++++++++++++++++++++++++--------
> 1 file changed, 44 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 9575aa645643..830263bad81e 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -326,6 +326,7 @@ static const u32 svs_regs_v2[] = {
> * @bank_max: total number of svs banks
> * @efuse: svs efuse data received from NVMEM framework
> * @tefuse: thermal efuse data received from NVMEM framework
> + * @clk_cnt: clock count shows the clk enable/disable times by svs driver
> */
> struct svs_platform {
> char *name;
> @@ -343,6 +344,7 @@ struct svs_platform {
> u32 bank_max;
> u32 *efuse;
> u32 *tefuse;
> + s32 clk_cnt;
> };
>
> struct svs_platform_data {
> @@ -502,6 +504,32 @@ static void svs_switch_bank(struct svs_platform *svsp)
> svs_writel_relaxed(svsp, svsb->core_sel, CORESEL);
> }
>
> +static bool svs_is_clk_enabled(struct svs_platform *svsp)
> +{
> + return svsp->clk_cnt > 0 ? true : false;
> +}
> +
> +static int svs_clk_enable(struct svs_platform *svsp)
> +{
> + int ret;
> +
> + ret = clk_prepare_enable(svsp->main_clk);
> + if (ret) {
> + dev_err(svsp->dev, "cannot enable main_clk: %d\n", ret);
> + return ret;
> + }
> +
> + svsp->clk_cnt++;
> +
> + return 0;
> +}
> +
> +static void svs_clk_disable(struct svs_platform *svsp)
> +{
> + clk_disable_unprepare(svsp->main_clk);
> + svsp->clk_cnt--;
> +}
> +
> static u32 svs_bank_volt_to_opp_volt(u32 svsb_volt, u32 svsb_volt_step,
> u32 svsb_volt_base)
> {
> @@ -1569,6 +1597,12 @@ static int svs_suspend(struct device *dev)
> int ret;
> u32 idx;
>
> + if (!svs_is_clk_enabled(svsp)) {
> + dev_err(svsp->dev, "svs clk is disabled already (%d)\n",
> + svsp->clk_cnt);
> + return 0;
> + }
> +
> for (idx = 0; idx < svsp->bank_max; idx++) {
> svsb = &svsp->banks[idx];
>
> @@ -1590,7 +1624,7 @@ static int svs_suspend(struct device *dev)
> return ret;
> }
>
> - clk_disable_unprepare(svsp->main_clk);
> + svs_clk_disable(svsp);
>
> return 0;
> }
> @@ -1600,16 +1634,14 @@ static int svs_resume(struct device *dev)
> struct svs_platform *svsp = dev_get_drvdata(dev);
> int ret;
>
> - ret = clk_prepare_enable(svsp->main_clk);
> - if (ret) {
> - dev_err(svsp->dev, "cannot enable main_clk, disable svs\n");
> + ret = svs_clk_enable(svsp);
> + if (ret)
> return ret;
> - }
>
> ret = reset_control_deassert(svsp->rst);
> if (ret) {
> dev_err(svsp->dev, "cannot deassert reset %d\n", ret);
> - goto out_of_resume;
> + goto svs_resume_clk_disable;
> }
>
> ret = svs_init02(svsp);
> @@ -1624,8 +1656,9 @@ static int svs_resume(struct device *dev)
> dev_err(svsp->dev, "assert reset: %d\n",
> reset_control_assert(svsp->rst));
>
> -out_of_resume:
> - clk_disable_unprepare(svsp->main_clk);
> +svs_resume_clk_disable:
> + svs_clk_disable(svsp);
> +
> return ret;
> }
>
> @@ -2411,11 +2444,9 @@ static int svs_probe(struct platform_device *pdev)
> goto svs_probe_free_resource;
> }
>
> - ret = clk_prepare_enable(svsp->main_clk);
> - if (ret) {
> - dev_err(svsp->dev, "cannot enable main clk: %d\n", ret);
> + ret = svs_clk_enable(svsp);
> + if (ret)
> goto svs_probe_free_resource;
> - }
>
> svsp->base = of_iomap(svsp->dev->of_node, 0);
> if (IS_ERR_OR_NULL(svsp->base)) {
> @@ -2456,7 +2487,7 @@ static int svs_probe(struct platform_device *pdev)
> iounmap(svsp->base);
>
> svs_probe_clk_disable:
> - clk_disable_unprepare(svsp->main_clk);
> + svs_clk_disable(svsp);
>
> svs_probe_free_resource:
> if (!IS_ERR_OR_NULL(svsp->efuse))
Powered by blists - more mailing lists