[v4,03/24] PM / devfreq: tegra30: Handle possible round-rate error
diff mbox series

Message ID 20190707223303.6755-4-digetx@gmail.com
State New
Headers show
Series
  • More improvements for Tegra30 devfreq driver
Related show

Commit Message

Dmitry Osipenko July 7, 2019, 10:32 p.m. UTC
The EMC clock rate rounding technically could fail, hence let's handle
the error cases properly.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Comments

Chanwoo Choi July 16, 2019, 11:50 a.m. UTC | #1
On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> The EMC clock rate rounding technically could fail, hence let's handle
> the error cases properly.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra30-devfreq.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 5e2b133babdd..5e606ae3f238 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -592,8 +592,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  	struct tegra_devfreq_device *dev;
>  	struct tegra_devfreq *tegra;
>  	struct devfreq *devfreq;
> -	unsigned long rate;
>  	unsigned int i;
> +	long rate;
>  	int err;
>  
>  	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> @@ -650,8 +650,14 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  
>  	reset_control_deassert(tegra->reset);
>  
> -	tegra->max_freq = clk_round_rate(tegra->emc_clock, ULONG_MAX) / KHZ;
> +	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
> +	if (rate < 0) {
> +		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
> +		return rate;
> +	}
> +
>  	tegra->cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
> +	tegra->max_freq = rate / KHZ;
>  
>  	for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
>  		dev = tegra->devices + i;
> @@ -662,6 +668,13 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  	for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
>  		rate = clk_round_rate(tegra->emc_clock, rate);
>  

Please remove unneeded blank line.

> +		if (rate < 0) {
> +			dev_err(&pdev->dev,
> +				"Failed to round clock rate: %ld\n", rate);
> +			err = rate;
> +			goto remove_opps;
> +		}

Also, this patch doesn't contain code which restore the previous
tegra->cur_freq/max_freq when error happen.

> +
>  		err = dev_pm_opp_add(&pdev->dev, rate, 0);
>  		if (err) {
>  			dev_err(&pdev->dev, "Failed to add OPP: %d\n", err);
>
Dmitry Osipenko July 16, 2019, 1:09 p.m. UTC | #2
16.07.2019 14:50, Chanwoo Choi пишет:
> On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
>> The EMC clock rate rounding technically could fail, hence let's handle
>> the error cases properly.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/tegra30-devfreq.c | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>> index 5e2b133babdd..5e606ae3f238 100644
>> --- a/drivers/devfreq/tegra30-devfreq.c
>> +++ b/drivers/devfreq/tegra30-devfreq.c
>> @@ -592,8 +592,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>  	struct tegra_devfreq_device *dev;
>>  	struct tegra_devfreq *tegra;
>>  	struct devfreq *devfreq;
>> -	unsigned long rate;
>>  	unsigned int i;
>> +	long rate;
>>  	int err;
>>  
>>  	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
>> @@ -650,8 +650,14 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>  
>>  	reset_control_deassert(tegra->reset);
>>  
>> -	tegra->max_freq = clk_round_rate(tegra->emc_clock, ULONG_MAX) / KHZ;
>> +	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
>> +	if (rate < 0) {
>> +		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
>> +		return rate;
>> +	}
>> +
>>  	tegra->cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
>> +	tegra->max_freq = rate / KHZ;
>>  
>>  	for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
>>  		dev = tegra->devices + i;
>> @@ -662,6 +668,13 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>  	for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
>>  		rate = clk_round_rate(tegra->emc_clock, rate);
>>  
> 
> Please remove unneeded blank line.

I can remove it, but it was added specifically to ease reading of the code.

>> +		if (rate < 0) {
>> +			dev_err(&pdev->dev,
>> +				"Failed to round clock rate: %ld\n", rate);
>> +			err = rate;
>> +			goto remove_opps;
>> +		}
> 
> Also, this patch doesn't contain code which restore the previous
> tegra->cur_freq/max_freq when error happen.

The error here results in abortion of the driver's probing, hence
nothing need to be restored in that case because nothing was changed at
this point yet.
Chanwoo Choi July 17, 2019, 6:38 a.m. UTC | #3
On 19. 7. 16. 오후 10:09, Dmitry Osipenko wrote:
> 16.07.2019 14:50, Chanwoo Choi пишет:
>> On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
>>> The EMC clock rate rounding technically could fail, hence let's handle
>>> the error cases properly.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/devfreq/tegra30-devfreq.c | 17 +++++++++++++++--
>>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>>> index 5e2b133babdd..5e606ae3f238 100644
>>> --- a/drivers/devfreq/tegra30-devfreq.c
>>> +++ b/drivers/devfreq/tegra30-devfreq.c
>>> @@ -592,8 +592,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>>  	struct tegra_devfreq_device *dev;
>>>  	struct tegra_devfreq *tegra;
>>>  	struct devfreq *devfreq;
>>> -	unsigned long rate;
>>>  	unsigned int i;
>>> +	long rate;
>>>  	int err;
>>>  
>>>  	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
>>> @@ -650,8 +650,14 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>>  
>>>  	reset_control_deassert(tegra->reset);
>>>  
>>> -	tegra->max_freq = clk_round_rate(tegra->emc_clock, ULONG_MAX) / KHZ;
>>> +	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
>>> +	if (rate < 0) {
>>> +		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
>>> +		return rate;
>>> +	}
>>> +
>>>  	tegra->cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
>>> +	tegra->max_freq = rate / KHZ;
>>>  
>>>  	for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
>>>  		dev = tegra->devices + i;
>>> @@ -662,6 +668,13 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>>  	for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
>>>  		rate = clk_round_rate(tegra->emc_clock, rate);
>>>  
>>
>> Please remove unneeded blank line.
> 
> I can remove it, but it was added specifically to ease reading of the code.
> 
>>> +		if (rate < 0) {
>>> +			dev_err(&pdev->dev,
>>> +				"Failed to round clock rate: %ld\n", rate);
>>> +			err = rate;
>>> +			goto remove_opps;
>>> +		}
>>
>> Also, this patch doesn't contain code which restore the previous
>> tegra->cur_freq/max_freq when error happen.
> 
> The error here results in abortion of the driver's probing, hence
> nothing need to be restored in that case because nothing was changed at
> this point yet.
> 
> 

OK.

Patch
diff mbox series

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 5e2b133babdd..5e606ae3f238 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -592,8 +592,8 @@  static int tegra_devfreq_probe(struct platform_device *pdev)
 	struct tegra_devfreq_device *dev;
 	struct tegra_devfreq *tegra;
 	struct devfreq *devfreq;
-	unsigned long rate;
 	unsigned int i;
+	long rate;
 	int err;
 
 	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
@@ -650,8 +650,14 @@  static int tegra_devfreq_probe(struct platform_device *pdev)
 
 	reset_control_deassert(tegra->reset);
 
-	tegra->max_freq = clk_round_rate(tegra->emc_clock, ULONG_MAX) / KHZ;
+	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
+	if (rate < 0) {
+		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
+		return rate;
+	}
+
 	tegra->cur_freq = clk_get_rate(tegra->emc_clock) / KHZ;
+	tegra->max_freq = rate / KHZ;
 
 	for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) {
 		dev = tegra->devices + i;
@@ -662,6 +668,13 @@  static int tegra_devfreq_probe(struct platform_device *pdev)
 	for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) {
 		rate = clk_round_rate(tegra->emc_clock, rate);
 
+		if (rate < 0) {
+			dev_err(&pdev->dev,
+				"Failed to round clock rate: %ld\n", rate);
+			err = rate;
+			goto remove_opps;
+		}
+
 		err = dev_pm_opp_add(&pdev->dev, rate, 0);
 		if (err) {
 			dev_err(&pdev->dev, "Failed to add OPP: %d\n", err);