| Message ID | 20240628-mtk-cpufreq-dvfs-fail-init-err-v1-1-19c55db23011@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: mediatek: Use dev_err_probe in every error path in probe | expand |
Il 28/06/24 21:48, Nícolas F. R. A. Prado ha scritto: > Use the dev_err_probe() helper to log the errors on every error path in > the probe function and its sub-functions. This includes > * adding error messages where there was none > * converting over dev_err/dev_warn > * removing the top-level error message after mtk_cpu_dvfs_info_init() is > called, since every error path inside that function already logs the > error reason. This gets rid of the misleading error message when probe > is deferred: > > mtk-cpufreq mtk-cpufreq: failed to initialize dvfs info for cpu0 > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > --- > drivers/cpufreq/mediatek-cpufreq.c | 66 ++++++++++++++++++-------------------- > 1 file changed, 31 insertions(+), 35 deletions(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index 518606adf14e..b21425bb83be 100644 > --- a/drivers/cpufreq/mediatek-cpufreq.c > +++ b/drivers/cpufreq/mediatek-cpufreq.c ..snip.. > @@ -487,7 +488,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) > rate = clk_get_rate(info->inter_clk); > opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate); > if (IS_ERR(opp)) { > - dev_err(cpu_dev, "cpu%d: failed to get intermediate opp\n", cpu); > + dev_err_probe(cpu_dev, ret, "cpu%d: failed to get intermediate opp\n", cpu); > ret = PTR_ERR(opp); I believe you want to first assign ret, and then use it in dev_err_probe() :-P Please fix. After which: Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Cheers! > goto out_disable_inter_clock; > } > @@ -501,7 +502,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) > info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier; > ret = dev_pm_opp_register_notifier(cpu_dev, &info->opp_nb); > if (ret) { > - dev_err(cpu_dev, "cpu%d: failed to register opp notifier\n", cpu); > + dev_err_probe(cpu_dev, ret, "cpu%d: failed to register opp notifier\n", cpu); > goto out_disable_inter_clock; > } > > @@ -629,11 +630,9 @@ static int mtk_cpufreq_probe(struct platform_device *pdev) > int cpu, ret; > > data = dev_get_platdata(&pdev->dev); > - if (!data) { > - dev_err(&pdev->dev, > - "failed to get mtk cpufreq platform data\n"); > - return -ENODEV; > - } > + if (!data) > + return dev_err_probe(&pdev->dev, -ENODEV, > + "failed to get mtk cpufreq platform data\n"); > > for_each_possible_cpu(cpu) { > info = mtk_cpu_dvfs_info_lookup(cpu); > @@ -643,24 +642,21 @@ static int mtk_cpufreq_probe(struct platform_device *pdev) > info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); > if (!info) { > ret = -ENOMEM; > + dev_err_probe(&pdev->dev, ret, "Failed to allocate dvfs_info\n"); > goto release_dvfs_info_list; > } > > info->soc_data = data; > ret = mtk_cpu_dvfs_info_init(info, cpu); > - if (ret) { > - dev_err(&pdev->dev, > - "failed to initialize dvfs info for cpu%d\n", > - cpu); > + if (ret) > goto release_dvfs_info_list; > - } > > list_add(&info->list_head, &dvfs_info_list); > } > > ret = cpufreq_register_driver(&mtk_cpufreq_driver); > if (ret) { > - dev_err(&pdev->dev, "failed to register mtk cpufreq driver\n"); > + dev_err_probe(&pdev->dev, ret, "failed to register mtk cpufreq driver\n"); > goto release_dvfs_info_list; > } > > > --- > base-commit: 0fc4bfab2cd45f9acb86c4f04b5191e114e901ed > change-id: 20240627-mtk-cpufreq-dvfs-fail-init-err-0a662ca72de2 > > Best regards,
On 28-06-24, 15:48, Nícolas F. R. A. Prado wrote: > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > @@ -629,11 +630,9 @@ static int mtk_cpufreq_probe(struct platform_device *pdev) > int cpu, ret; > > data = dev_get_platdata(&pdev->dev); > - if (!data) { > - dev_err(&pdev->dev, > - "failed to get mtk cpufreq platform data\n"); > - return -ENODEV; > - } > + if (!data) > + return dev_err_probe(&pdev->dev, -ENODEV, What's the point of calling dev_err_probe() when we know for sure that the error isn't EPROBE_DEFER ? > + "failed to get mtk cpufreq platform data\n"); > > for_each_possible_cpu(cpu) { > info = mtk_cpu_dvfs_info_lookup(cpu); > @@ -643,24 +642,21 @@ static int mtk_cpufreq_probe(struct platform_device *pdev) > info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); > if (!info) { > ret = -ENOMEM; > + dev_err_probe(&pdev->dev, ret, "Failed to allocate dvfs_info\n"); Same here.
Il 02/07/24 07:57, Viresh Kumar ha scritto: > On 28-06-24, 15:48, Nícolas F. R. A. Prado wrote: >> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c >> @@ -629,11 +630,9 @@ static int mtk_cpufreq_probe(struct platform_device *pdev) >> int cpu, ret; >> >> data = dev_get_platdata(&pdev->dev); >> - if (!data) { >> - dev_err(&pdev->dev, >> - "failed to get mtk cpufreq platform data\n"); >> - return -ENODEV; >> - } >> + if (!data) >> + return dev_err_probe(&pdev->dev, -ENODEV, > > What's the point of calling dev_err_probe() when we know for sure that > the error isn't EPROBE_DEFER ? > Logging consistency, that's all; the alternative would be to rewrite the dev_err() messages to be consistent with what dev_err_probe() says, so that'd be dev_err("error %pe: (message)"); >> + "failed to get mtk cpufreq platform data\n"); >> >> for_each_possible_cpu(cpu) { >> info = mtk_cpu_dvfs_info_lookup(cpu); >> @@ -643,24 +642,21 @@ static int mtk_cpufreq_probe(struct platform_device *pdev) >> info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); >> if (!info) { >> ret = -ENOMEM; >> + dev_err_probe(&pdev->dev, ret, "Failed to allocate dvfs_info\n"); > By the way, forgot to point that out in my former review: to make it shorter, instead of "ret = -ENOMEM; dev_err_probe()" you can write it as... ret = dev_err_probe(&pdev->dev, -ENOMEM, ".... message"); Cheers, Angelo > Same here. >
On 02-07-24, 10:26, AngeloGioacchino Del Regno wrote: > Il 02/07/24 07:57, Viresh Kumar ha scritto: > > On 28-06-24, 15:48, Nícolas F. R. A. Prado wrote: > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > > > @@ -629,11 +630,9 @@ static int mtk_cpufreq_probe(struct platform_device *pdev) > > > int cpu, ret; > > > data = dev_get_platdata(&pdev->dev); > > > - if (!data) { > > > - dev_err(&pdev->dev, > > > - "failed to get mtk cpufreq platform data\n"); > > > - return -ENODEV; > > > - } > > > + if (!data) > > > + return dev_err_probe(&pdev->dev, -ENODEV, > > > > What's the point of calling dev_err_probe() when we know for sure that > > the error isn't EPROBE_DEFER ? > > > > Logging consistency, that's all; the alternative would be to rewrite the dev_err() > messages to be consistent with what dev_err_probe() says, so that'd be > dev_err("error %pe: (message)"); That would be better I guess. There is no point adding inefficient code. > > > + "failed to get mtk cpufreq platform data\n"); > > > for_each_possible_cpu(cpu) { > > > info = mtk_cpu_dvfs_info_lookup(cpu); > > > @@ -643,24 +642,21 @@ static int mtk_cpufreq_probe(struct platform_device *pdev) > > > info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); > > > if (!info) { > > > ret = -ENOMEM; > > > + dev_err_probe(&pdev->dev, ret, "Failed to allocate dvfs_info\n"); > > > > By the way, forgot to point that out in my former review: to make it shorter, > instead of "ret = -ENOMEM; dev_err_probe()" you can write it as... > > ret = dev_err_probe(&pdev->dev, -ENOMEM, ".... message"); `ret` will be be used I guess with the `goto` statement to return error and so the change was like this.
Il 02/07/24 10:43, Viresh Kumar ha scritto: > On 02-07-24, 10:26, AngeloGioacchino Del Regno wrote: >> Il 02/07/24 07:57, Viresh Kumar ha scritto: >>> On 28-06-24, 15:48, Nícolas F. R. A. Prado wrote: >>>> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c >>>> @@ -629,11 +630,9 @@ static int mtk_cpufreq_probe(struct platform_device *pdev) >>>> int cpu, ret; >>>> data = dev_get_platdata(&pdev->dev); >>>> - if (!data) { >>>> - dev_err(&pdev->dev, >>>> - "failed to get mtk cpufreq platform data\n"); >>>> - return -ENODEV; >>>> - } >>>> + if (!data) >>>> + return dev_err_probe(&pdev->dev, -ENODEV, >>> >>> What's the point of calling dev_err_probe() when we know for sure that >>> the error isn't EPROBE_DEFER ? >>> >> >> Logging consistency, that's all; the alternative would be to rewrite the dev_err() >> messages to be consistent with what dev_err_probe() says, so that'd be >> dev_err("error %pe: (message)"); > > That would be better I guess. There is no point adding inefficient > code. > >>>> + "failed to get mtk cpufreq platform data\n"); >>>> for_each_possible_cpu(cpu) { >>>> info = mtk_cpu_dvfs_info_lookup(cpu); >>>> @@ -643,24 +642,21 @@ static int mtk_cpufreq_probe(struct platform_device *pdev) >>>> info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); >>>> if (!info) { >>>> ret = -ENOMEM; >>>> + dev_err_probe(&pdev->dev, ret, "Failed to allocate dvfs_info\n"); >>> >> >> By the way, forgot to point that out in my former review: to make it shorter, >> instead of "ret = -ENOMEM; dev_err_probe()" you can write it as... >> >> ret = dev_err_probe(&pdev->dev, -ENOMEM, ".... message"); > > `ret` will be be used I guess with the `goto` statement to return > error and so the change was like this. > Yes it will, but `ret = dev_err_probe(dev, -ENOMEM, "...")` is still kind of a shortcut, as that will effectively assign -ENOMEM to ret, so that the error is still returned like before ;-) Cheers
On 03-07-24, 14:43, AngeloGioacchino Del Regno wrote: > Yes it will, but `ret = dev_err_probe(dev, -ENOMEM, "...")` is still kind of a > shortcut, as that will effectively assign -ENOMEM to ret, so that the error is > still returned like before ;-) Ahh, my bad. I missed that you are assigning `ret` again.
On Tue, Jul 02, 2024 at 02:13:07PM +0530, Viresh Kumar wrote: > On 02-07-24, 10:26, AngeloGioacchino Del Regno wrote: > > Il 02/07/24 07:57, Viresh Kumar ha scritto: > > > On 28-06-24, 15:48, Nícolas F. R. A. Prado wrote: > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > > > > @@ -629,11 +630,9 @@ static int mtk_cpufreq_probe(struct platform_device *pdev) > > > > int cpu, ret; > > > > data = dev_get_platdata(&pdev->dev); > > > > - if (!data) { > > > > - dev_err(&pdev->dev, > > > > - "failed to get mtk cpufreq platform data\n"); > > > > - return -ENODEV; > > > > - } > > > > + if (!data) > > > > + return dev_err_probe(&pdev->dev, -ENODEV, > > > > > > What's the point of calling dev_err_probe() when we know for sure that > > > the error isn't EPROBE_DEFER ? > > > > > > > Logging consistency, that's all; the alternative would be to rewrite the dev_err() > > messages to be consistent with what dev_err_probe() says, so that'd be > > dev_err("error %pe: (message)"); > > That would be better I guess. There is no point adding inefficient > code. Well, that would add duplication of the error format string, and make the error message (the actual information here) less clear in the source code. Also, dev_err doesn't return the error so it needs to be written in two lines, instead of a single one. Consider it's not just about consistency of the log messages, but consistency of the source code as well: with so many drivers transitioning to the use of `return dev_err_probe(...);` patterns, sticking to the pattern means it's immediately clear what the code does. And it is a desirable pattern because it removes all boilerplate and leaves just the real information (the error code and the message). Besides, the inneficient code amounts to an extra va_list usage and an int comparison. That would only run once during boot and only in the error path... I'm confident in saying this won't amount to any real performance gain. So the usage of dev_err_probe() everywhere for log and source code standardization is well worth it. Thanks, Nícolas
On 05-07-24, 11:13, Nícolas F. R. A. Prado wrote: > That would only run once during boot and only in the error path... > I'm confident in saying this won't amount to any real performance gain. So the > usage of dev_err_probe() everywhere for log and source code standardization is > well worth it. Hmm. Fair enough.
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c index 518606adf14e..b21425bb83be 100644 --- a/drivers/cpufreq/mediatek-cpufreq.c +++ b/drivers/cpufreq/mediatek-cpufreq.c @@ -390,28 +390,23 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) int ret; cpu_dev = get_cpu_device(cpu); - if (!cpu_dev) { - dev_err(cpu_dev, "failed to get cpu%d device\n", cpu); - return -ENODEV; - } + if (!cpu_dev) + return dev_err_probe(cpu_dev, -ENODEV, "failed to get cpu%d device\n", cpu); info->cpu_dev = cpu_dev; info->ccifreq_bound = false; if (info->soc_data->ccifreq_supported) { info->cci_dev = of_get_cci(info->cpu_dev); - if (IS_ERR(info->cci_dev)) { - ret = PTR_ERR(info->cci_dev); - dev_err(cpu_dev, "cpu%d: failed to get cci device\n", cpu); - return -ENODEV; - } + if (IS_ERR(info->cci_dev)) + return dev_err_probe(cpu_dev, PTR_ERR(info->cci_dev), + "cpu%d: failed to get cci device\n", + cpu); } info->cpu_clk = clk_get(cpu_dev, "cpu"); - if (IS_ERR(info->cpu_clk)) { - ret = PTR_ERR(info->cpu_clk); - return dev_err_probe(cpu_dev, ret, + if (IS_ERR(info->cpu_clk)) + return dev_err_probe(cpu_dev, PTR_ERR(info->cpu_clk), "cpu%d: failed to get cpu clk\n", cpu); - } info->inter_clk = clk_get(cpu_dev, "intermediate"); if (IS_ERR(info->inter_clk)) { @@ -431,7 +426,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) ret = regulator_enable(info->proc_reg); if (ret) { - dev_warn(cpu_dev, "cpu%d: failed to enable vproc\n", cpu); + dev_err_probe(cpu_dev, ret, "cpu%d: failed to enable vproc\n", cpu); goto out_free_proc_reg; } @@ -439,14 +434,17 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) info->sram_reg = regulator_get_optional(cpu_dev, "sram"); if (IS_ERR(info->sram_reg)) { ret = PTR_ERR(info->sram_reg); - if (ret == -EPROBE_DEFER) + if (ret == -EPROBE_DEFER) { + dev_err_probe(cpu_dev, ret, + "cpu%d: Failed to get sram regulator\n", cpu); goto out_disable_proc_reg; + } info->sram_reg = NULL; } else { ret = regulator_enable(info->sram_reg); if (ret) { - dev_warn(cpu_dev, "cpu%d: failed to enable vsram\n", cpu); + dev_err_probe(cpu_dev, ret, "cpu%d: failed to enable vsram\n", cpu); goto out_free_sram_reg; } } @@ -454,31 +452,34 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) /* Get OPP-sharing information from "operating-points-v2" bindings */ ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, &info->cpus); if (ret) { - dev_err(cpu_dev, + dev_err_probe(cpu_dev, ret, "cpu%d: failed to get OPP-sharing information\n", cpu); goto out_disable_sram_reg; } ret = dev_pm_opp_of_cpumask_add_table(&info->cpus); if (ret) { - dev_warn(cpu_dev, "cpu%d: no OPP table\n", cpu); + dev_err_probe(cpu_dev, ret, "cpu%d: no OPP table\n", cpu); goto out_disable_sram_reg; } ret = clk_prepare_enable(info->cpu_clk); - if (ret) + if (ret) { + dev_err_probe(cpu_dev, ret, "cpu%d: failed to enable cpu clk\n", cpu); goto out_free_opp_table; + } ret = clk_prepare_enable(info->inter_clk); - if (ret) + if (ret) { + dev_err_probe(cpu_dev, ret, "cpu%d: failed to enable inter clk\n", cpu); goto out_disable_mux_clock; + } if (info->soc_data->ccifreq_supported) { info->vproc_on_boot = regulator_get_voltage(info->proc_reg); if (info->vproc_on_boot < 0) { ret = info->vproc_on_boot; - dev_err(info->cpu_dev, - "invalid Vproc value: %d\n", info->vproc_on_boot); + dev_err_probe(info->cpu_dev, ret, "invalid Vproc value\n"); goto out_disable_inter_clock; } } @@ -487,7 +488,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) rate = clk_get_rate(info->inter_clk); opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate); if (IS_ERR(opp)) { - dev_err(cpu_dev, "cpu%d: failed to get intermediate opp\n", cpu); + dev_err_probe(cpu_dev, ret, "cpu%d: failed to get intermediate opp\n", cpu); ret = PTR_ERR(opp); goto out_disable_inter_clock; } @@ -501,7 +502,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier; ret = dev_pm_opp_register_notifier(cpu_dev, &info->opp_nb); if (ret) { - dev_err(cpu_dev, "cpu%d: failed to register opp notifier\n", cpu); + dev_err_probe(cpu_dev, ret, "cpu%d: failed to register opp notifier\n", cpu); goto out_disable_inter_clock; } @@ -629,11 +630,9 @@ static int mtk_cpufreq_probe(struct platform_device *pdev) int cpu, ret; data = dev_get_platdata(&pdev->dev); - if (!data) { - dev_err(&pdev->dev, - "failed to get mtk cpufreq platform data\n"); - return -ENODEV; - } + if (!data) + return dev_err_probe(&pdev->dev, -ENODEV, + "failed to get mtk cpufreq platform data\n"); for_each_possible_cpu(cpu) { info = mtk_cpu_dvfs_info_lookup(cpu); @@ -643,24 +642,21 @@ static int mtk_cpufreq_probe(struct platform_device *pdev) info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); if (!info) { ret = -ENOMEM; + dev_err_probe(&pdev->dev, ret, "Failed to allocate dvfs_info\n"); goto release_dvfs_info_list; } info->soc_data = data; ret = mtk_cpu_dvfs_info_init(info, cpu); - if (ret) { - dev_err(&pdev->dev, - "failed to initialize dvfs info for cpu%d\n", - cpu); + if (ret) goto release_dvfs_info_list; - } list_add(&info->list_head, &dvfs_info_list); } ret = cpufreq_register_driver(&mtk_cpufreq_driver); if (ret) { - dev_err(&pdev->dev, "failed to register mtk cpufreq driver\n"); + dev_err_probe(&pdev->dev, ret, "failed to register mtk cpufreq driver\n"); goto release_dvfs_info_list; }
Use the dev_err_probe() helper to log the errors on every error path in the probe function and its sub-functions. This includes * adding error messages where there was none * converting over dev_err/dev_warn * removing the top-level error message after mtk_cpu_dvfs_info_init() is called, since every error path inside that function already logs the error reason. This gets rid of the misleading error message when probe is deferred: mtk-cpufreq mtk-cpufreq: failed to initialize dvfs info for cpu0 Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- drivers/cpufreq/mediatek-cpufreq.c | 66 ++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 35 deletions(-) --- base-commit: 0fc4bfab2cd45f9acb86c4f04b5191e114e901ed change-id: 20240627-mtk-cpufreq-dvfs-fail-init-err-0a662ca72de2 Best regards,