Re: [PATCH v1 2/3] cpufreq: mediatek: fix KP caused by handler usage after regulator_put/clk_put

Re: [PATCH v1 2/3] cpufreq: mediatek: fix KP caused by handler usage after regulator_put/clk_put

[Dan Carpenter]

On Fri, Mar 10, 2023 at 01:17:49PM +0800, jia-wei.chang wrote:
> From: "Jia-Wei Chang" <jia-wei.chang@mediatek.com>
> 
> Any kind of failure in mtk_cpu_dvfs_info_init() will lead to calling
> regulator_put() or clk_put() and the KP will occur since the regulator/clk
> handlers are used after released in mtk_cpu_dvfs_info_release().
> 

This patch is harmless but it's not required.

If the mtk_cpu_dvfs_info_init() function is not able to complete
successfully then it cleans up all the partial allocations.  If it is
able to allocate everything successfully, then the caller, which is
mtk_cpufreq_probe(), adds it to the &dvfs_info_list.  If the
probe() function is not able to complete successfully it releases
everything in the &dvfs_info_list.

In this way, mtk_cpu_dvfs_info_init() never frees anything which has
not been allocated and it never frees anything which has already been
freed.  All the IS_ERR() checks in mtk_cpu_dvfs_info_release() can be
removed.

In reviewing this code, I can't help but notice that mtk_cpu_dvfs_info_init()
is buggy.  Please read my blog on error handling:
https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

drivers/cpufreq/mediatek-cpufreq.c
   411          info->cpu_clk = clk_get(cpu_dev, "cpu");
   412          if (IS_ERR(info->cpu_clk)) {
   413                  ret = PTR_ERR(info->cpu_clk);
   414                  return dev_err_probe(cpu_dev, ret,
                        ^^^^^^^^^^^^^^^^^^^^
Here we have a direct return which means there is nothing to free.
Okay.

   415                                       "cpu%d: failed to get cpu clk\n", cpu);
   416          }
   417  
   418          info->inter_clk = clk_get(cpu_dev, "intermediate");
   419          if (IS_ERR(info->inter_clk)) {
   420                  ret = PTR_ERR(info->inter_clk);
   421                  dev_err_probe(cpu_dev, ret,
   422                                "cpu%d: failed to get intermediate clk\n", cpu);
   423                  goto out_free_resources;
                        ^^^^^^^^^^^^^^^^^^^^^^^
The last thing we successfully allocated was ->cpu_clk so ideally the
goto would be something like "goto put_cpu_clk;".  Let's assume this
clk_get() fails.


   424          }
   425  
   426          info->proc_reg = regulator_get_optional(cpu_dev, "proc");
                ^^^^^^^^^^^^^^
Note that this is where ->proc_reg is allocated.  Prior to that point
it is NULL because it's allocated with kzalloc().

   427          if (IS_ERR(info->proc_reg)) {
   428                  ret = PTR_ERR(info->proc_reg);
   429                  dev_err_probe(cpu_dev, ret,
   430                                "cpu%d: failed to get proc regulator\n", cpu);
   431                  goto out_free_resources;
   432          }

[ snip ]

   536  out_free_resources:
   537          if (regulator_is_enabled(info->proc_reg))
                                         ^^^^^^^^^^^^^^
Oops!  This is a NULL dereference because ->proc_reg wasn't allocated
yet.

   538                  regulator_disable(info->proc_reg);
   539          if (info->sram_reg && regulator_is_enabled(info->sram_reg))
   540                  regulator_disable(info->sram_reg);
   541  
   542          if (!IS_ERR(info->proc_reg))
   543                  regulator_put(info->proc_reg);
   544          if (!IS_ERR(info->sram_reg))
   545                  regulator_put(info->sram_reg);
   546          if (!IS_ERR(info->cpu_clk))
   547                  clk_put(info->cpu_clk);
   548          if (!IS_ERR(info->inter_clk))
   549                  clk_put(info->inter_clk);
   550  
   551          return ret;
   552  }

It would be safer and more readable to have:

err_put_inter_clk:
	clk_put(info->inter_clk);
err_put_cpu_clk:
	clk_put(info->cpu_clk);

etc.

regards,
dan carpenter

Re: [PATCH v1 2/3] cpufreq: mediatek: fix KP caused by handler usage after regulator_put/clk_put

[AngeloGioacchino Del Regno]

Il 10/03/23 06:17, jia-wei.chang ha scritto:
> From: "Jia-Wei Chang" <jia-wei.chang@mediatek.com>
> 
> Any kind of failure in mtk_cpu_dvfs_info_init() will lead to calling
> regulator_put() or clk_put() and the KP will occur since the regulator/clk
> handlers are used after released in mtk_cpu_dvfs_info_release().
> 
> To prevent the usage after regulator_put()/clk_put(), the regulator/clk
> handlers are reassigned to NULL value for validation check afterwards.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> Fixes: 4b9ceb757bbb ("cpufreq: mediatek: Enable clocks and regulators")
> Reported-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Link: https://lore.kernel.org/linux-arm-kernel/20220921071913.p7kwsjnnuad2jgvk@vireshk-i7/T/

Actually, it's not just Reported-by, but also please add:
Suggested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>


Anyway, apart from that:
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v1 1/3] cpufreq: mediatek: fix passing zero to 'PTR_ERR'

[AngeloGioacchino Del Regno]

Il 10/03/23 06:17, jia-wei.chang ha scritto:
> From: "Jia-Wei Chang" <jia-wei.chang@mediatek.com>
> 
> In order to prevent passing zero to 'PTR_ERR' in
> mtk_cpu_dvfs_info_init(), we fix the return value of of_get_cci() using
> error pointer by explicitly casting error number.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> Fixes: 0daa47325bae ("cpufreq: mediatek: Link CCI device to CPU")
> Reported-by: Dan Carpenter <error27@gmail.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v1 3/3] cpufreq: mediatek: raise proc/sram max voltage for MT7622 and MT8516

[AngeloGioacchino Del Regno]

Il 10/03/23 06:17, jia-wei.chang ha scritto:
> From: "Jia-Wei Chang" <jia-wei.chang@mediatek.com>
> 
> Since the upper boundary of proc/sram voltage of MT7622 and MT8516 are
> 1350 mV and 1300 mV respectively, both are greater than the value of
> MT2701 1150 mV, we fix it by adding the corresponding platform data and
> specify proc/sram_max_volt to support MT7622 and MT8516 individually.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> Fixes: ead858bd128d ("cpufreq: mediatek: Move voltage limits to platform data")
> Fixes: 6a17b3876bc8 ("cpufreq: mediatek: Refine mtk_cpufreq_voltage_tracking()")
> Reported-by: Nick Hainke <vincent@systemli.org>
> Link: https://lore.kernel.org/lkml/75216e0c-9d36-7ada-1507-1bb4a91a3326@systemli.org/T/

that's 2/3 my patch, 1/3 your patch.....
...in that case, send my patch as it is and add one more for MT8516.

Regards,
Angelo

> ---
>   drivers/cpufreq/mediatek-cpufreq.c | 26 ++++++++++++++++++++++----
>   1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> index cb8b76f9c2c3..f6824d4bf85d 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c
> @@ -707,6 +707,15 @@ static const struct mtk_cpufreq_platform_data mt2701_platform_data = {
>   	.ccifreq_supported = false,
>   };
>   
> +static const struct mtk_cpufreq_platform_data mt7622_platform_data = {
> +	.min_volt_shift = 100000,
> +	.max_volt_shift = 200000,
> +	.proc_max_volt = 1360000,
> +	.sram_min_volt = 0,
> +	.sram_max_volt = 1360000,
> +	.ccifreq_supported = false,
> +};
> +
>   static const struct mtk_cpufreq_platform_data mt8183_platform_data = {
>   	.min_volt_shift = 100000,
>   	.max_volt_shift = 200000,
> @@ -725,20 +734,29 @@ static const struct mtk_cpufreq_platform_data mt8186_platform_data = {
>   	.ccifreq_supported = true,
>   };
>   
> +static const struct mtk_cpufreq_platform_data mt8516_platform_data = {
> +	.min_volt_shift = 100000,
> +	.max_volt_shift = 200000,
> +	.proc_max_volt = 1310000,
> +	.sram_min_volt = 0,
> +	.sram_max_volt = 1310000,
> +	.ccifreq_supported = false,
> +};
> +
>   /* List of machines supported by this driver */
>   static const struct of_device_id mtk_cpufreq_machines[] __initconst = {
>   	{ .compatible = "mediatek,mt2701", .data = &mt2701_platform_data },
>   	{ .compatible = "mediatek,mt2712", .data = &mt2701_platform_data },
> -	{ .compatible = "mediatek,mt7622", .data = &mt2701_platform_data },
> -	{ .compatible = "mediatek,mt7623", .data = &mt2701_platform_data },
> -	{ .compatible = "mediatek,mt8167", .data = &mt2701_platform_data },
> +	{ .compatible = "mediatek,mt7622", .data = &mt7622_platform_data },
> +	{ .compatible = "mediatek,mt7623", .data = &mt7622_platform_data },
> +	{ .compatible = "mediatek,mt8167", .data = &mt8516_platform_data },
>   	{ .compatible = "mediatek,mt817x", .data = &mt2701_platform_data },
>   	{ .compatible = "mediatek,mt8173", .data = &mt2701_platform_data },
>   	{ .compatible = "mediatek,mt8176", .data = &mt2701_platform_data },
>   	{ .compatible = "mediatek,mt8183", .data = &mt8183_platform_data },
>   	{ .compatible = "mediatek,mt8186", .data = &mt8186_platform_data },
>   	{ .compatible = "mediatek,mt8365", .data = &mt2701_platform_data },
> -	{ .compatible = "mediatek,mt8516", .data = &mt2701_platform_data },
> +	{ .compatible = "mediatek,mt8516", .data = &mt8516_platform_data },
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(of, mtk_cpufreq_machines);