[v2,06/19] PM / devfreq: tegra: Fix missed error checking on devfreq initialization failure
diff mbox series

Message ID 20190415145505.18397-7-digetx@gmail.com
State Not Applicable
Headers show
Series
  • NVIDIA Tegra devfreq improvements and Tegra20/30 support
Related show

Commit Message

Dmitry Osipenko April 15, 2019, 2:54 p.m. UTC
The code doesn't check for devfreq initialization failure, fix it.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra-devfreq.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Chanwoo Choi April 16, 2019, 2:32 a.m. UTC | #1
Hi,

patch6/7/8/9 are for handling of exception handling in probe() function.
Actually, I'm not sure that there are special reason to split out
the patches. I think that you can squash patch6/7/8/9 to only one patch.

Also, even if patch6/7/8/9 handle the exception handling in probe(),
the tegra_devfreq_probe() doesn't support the restoring sequence
when fail happen. I think that if you want to fix the fail case of probe(),
please add the restoring sequence about following function.
- clk_disable_unprepare()
- clk_notifier_unregister()
- dev_pm_opp_remove()


On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
> The code doesn't check for devfreq initialization failure, fix it.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra-devfreq.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index f0f0d78f6cbf..aa0478464b35 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -707,6 +707,10 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  						 &tegra_devfreq_profile,
>  						 "tegra_actmon",
>  						 NULL);
> +	if (IS_ERR(tegra->devfreq)) {
> +		err = PTR_ERR(tegra->devfreq);
> +		return err;
> +	}
>  
>  	return 0;
>  }
>
Dmitry Osipenko April 16, 2019, 2:29 p.m. UTC | #2
16.04.2019 5:32, Chanwoo Choi пишет:
> Hi,
> 
> patch6/7/8/9 are for handling of exception handling in probe() function.
> Actually, I'm not sure that there are special reason to split out
> the patches. I think that you can squash patch6/7/8/9 to only one patch.

Indeed, I was rebasing and reordering patches multiple times and looks like there is no reason not to squash these patches now.

> Also, even if patch6/7/8/9 handle the exception handling in probe(),
> the tegra_devfreq_probe() doesn't support the restoring sequence
> when fail happen. I think that if you want to fix the fail case of probe(),
> please add the restoring sequence about following function.
> - clk_disable_unprepare()
> - clk_notifier_unregister()
> - dev_pm_opp_remove()

When all of 6/7/8/9 patches are applied, the clk_notifier_register() becomes the last invocation of the probe function and clk_enable() is kept at the first place of the probe order. Hence the sequence you're suggesting is already incorrect because error-unwinding order usually should be opposite to the probe order. It looks to me that the current final result of these patches is already correct.
Chanwoo Choi April 17, 2019, 1:01 a.m. UTC | #3
Hi,

On 19. 4. 16. 오후 11:29, Dmitry Osipenko wrote:
> 16.04.2019 5:32, Chanwoo Choi пишет:
>> Hi,
>>
>> patch6/7/8/9 are for handling of exception handling in probe() function.
>> Actually, I'm not sure that there are special reason to split out
>> the patches. I think that you can squash patch6/7/8/9 to only one patch.
> 
> Indeed, I was rebasing and reordering patches multiple times and looks like there is no reason not to squash these patches now.
> 
>> Also, even if patch6/7/8/9 handle the exception handling in probe(),
>> the tegra_devfreq_probe() doesn't support the restoring sequence
>> when fail happen. I think that if you want to fix the fail case of probe(),
>> please add the restoring sequence about following function.
>> - clk_disable_unprepare()
>> - clk_notifier_unregister()
>> - dev_pm_opp_remove()
> 
> When all of 6/7/8/9 patches are applied, the clk_notifier_register() becomes the last invocation of the probe function and clk_enable() is kept at the first place of the probe order. Hence the sequence you're suggesting is already incorrect because error-unwinding order usually should be opposite to the probe order. It looks to me that the current final result of these patches is already correct.

You're right. When I replied it, I have not considered the order.
Sorry, it made you some confusion.

> 
>

Patch
diff mbox series

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index f0f0d78f6cbf..aa0478464b35 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -707,6 +707,10 @@  static int tegra_devfreq_probe(struct platform_device *pdev)
 						 &tegra_devfreq_profile,
 						 "tegra_actmon",
 						 NULL);
+	if (IS_ERR(tegra->devfreq)) {
+		err = PTR_ERR(tegra->devfreq);
+		return err;
+	}
 
 	return 0;
 }