| Message ID | 20220909093724.40078-1-angelogioacchino.delregno@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | cpufreq: mediatek: Fix KP and lockups on proc/sram regulators error | expand |
Jia, do you want to reply to this thread as the Fixes patch was added by you ? On 09-09-22, 11:37, AngeloGioacchino Del Regno wrote: > Function regulator_get_optional() returns a negative error number on > any kind of regulator_get() failure: failing to check for that in the > teardown path will lead to a kernel panic due to a call to function > regulator_disable(). I don't see how this can happen. The code does check if the regulators are enabled earlier or not. > Besides that, the "proc" regulator does actually provide power to the > CPU cluster(s): disabling it will produce a lockup on at least some > SoCs, such as MT8173. We are just dropping the count that we increased earlier, how will that disable the regulator which was already enabled ? > That consideration is also valid for the "sram" regulator, providing > power to the CPU caches instead, present on some other SoCs, such as > MT8183, MT8186 (and others). > > Resolve both situations and by simply removing the entire faulty > branches responsible for disabling the aforementioned regulators if > enabled, keeping in mind that these are enabled (and left enabled) > by the bootloader before booting the kernel. This looks fishy, we just keep on increasing the ref count of the regulator but never take it down.
On Wed, 2022-09-21 at 12:49 +0530, Viresh Kumar wrote: > Jia, do you want to reply to this thread as the Fixes patch was added > by you ? > > On 09-09-22, 11:37, AngeloGioacchino Del Regno wrote: > > Function regulator_get_optional() returns a negative error number > > on > > any kind of regulator_get() failure: failing to check for that in > > the > > teardown path will lead to a kernel panic due to a call to function > > regulator_disable(). > > I don't see how this can happen. The code does check if the > regulators > are enabled earlier or not. > Hi Angelo, Could you help provide more details, like the call stack of kernel panic? and how to reproduce this failure? > > Besides that, the "proc" regulator does actually provide power to > > the > > CPU cluster(s): disabling it will produce a lockup on at least some > > SoCs, such as MT8173. > > We are just dropping the count that we increased earlier, how will > that disable the regulator which was already enabled ? > > > That consideration is also valid for the "sram" regulator, > > providing > > power to the CPU caches instead, present on some other SoCs, such > > as > > MT8183, MT8186 (and others). > > > > Resolve both situations and by simply removing the entire faulty > > branches responsible for disabling the aforementioned regulators if > > enabled, keeping in mind that these are enabled (and left enabled) > > by the bootloader before booting the kernel. > > This looks fishy, we just keep on increasing the ref count of the > regulator but never take it down. > Angelo, Do you mean the ref count of the regulator in the kernel will be affected if that regulator is enabled earlier in the bootloader? Thanks.
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c index 7f2680bc9a0f..d68f16f475a6 100644 --- a/drivers/cpufreq/mediatek-cpufreq.c +++ b/drivers/cpufreq/mediatek-cpufreq.c @@ -534,11 +534,6 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) dev_pm_opp_of_cpumask_remove_table(&info->cpus); out_free_resources: - if (regulator_is_enabled(info->proc_reg)) - regulator_disable(info->proc_reg); - if (info->sram_reg && regulator_is_enabled(info->sram_reg)) - regulator_disable(info->sram_reg); - if (!IS_ERR(info->proc_reg)) regulator_put(info->proc_reg); if (!IS_ERR(info->sram_reg)) @@ -553,14 +548,10 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) static void mtk_cpu_dvfs_info_release(struct mtk_cpu_dvfs_info *info) { - if (!IS_ERR(info->proc_reg)) { - regulator_disable(info->proc_reg); + if (!IS_ERR(info->proc_reg)) regulator_put(info->proc_reg); - } - if (!IS_ERR(info->sram_reg)) { - regulator_disable(info->sram_reg); + if (!IS_ERR(info->sram_reg)) regulator_put(info->sram_reg); - } if (!IS_ERR(info->cpu_clk)) { clk_disable_unprepare(info->cpu_clk); clk_put(info->cpu_clk);
Function regulator_get_optional() returns a negative error number on any kind of regulator_get() failure: failing to check for that in the teardown path will lead to a kernel panic due to a call to function regulator_disable(). Besides that, the "proc" regulator does actually provide power to the CPU cluster(s): disabling it will produce a lockup on at least some SoCs, such as MT8173. That consideration is also valid for the "sram" regulator, providing power to the CPU caches instead, present on some other SoCs, such as MT8183, MT8186 (and others). Resolve both situations and by simply removing the entire faulty branches responsible for disabling the aforementioned regulators if enabled, keeping in mind that these are enabled (and left enabled) by the bootloader before booting the kernel. Fixes: 4b9ceb757bbb ("cpufreq: mediatek: Enable clocks and regulators") Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> --- drivers/cpufreq/mediatek-cpufreq.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)