diff mbox series

cpufreq: brcmstb-avs-cpufreq: Fix some resource leaks in the error handling path of the probe function

Message ID 20201219101751.181783-1-christophe.jaillet@wanadoo.fr (mailing list archive)
State New, archived
Delegated to: viresh kumar
Headers show
Series cpufreq: brcmstb-avs-cpufreq: Fix some resource leaks in the error handling path of the probe function | expand

Commit Message

Christophe JAILLET Dec. 19, 2020, 10:17 a.m. UTC
If 'cpufreq_register_driver()' fails, we must release the resources
allocated in 'brcm_avs_prepare_init()' as already done in the remove
function.

To do that, introduce a new function 'brcm_avs_prepare_uninit()' in order
to avoid code duplication. This also makes the code more readable (IMHO).

Fixes: de322e085995 ("cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
I'm not sure that the existing error handling in the remove function is
correct and/or needed.
---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

Comments

Florian Fainelli Dec. 21, 2020, 11 p.m. UTC | #1
On 12/19/2020 2:17 AM, Christophe JAILLET wrote:
> If 'cpufreq_register_driver()' fails, we must release the resources
> allocated in 'brcm_avs_prepare_init()' as already done in the remove
> function.
> 
> To do that, introduce a new function 'brcm_avs_prepare_uninit()' in order
> to avoid code duplication. This also makes the code more readable (IMHO).
> 
> Fixes: de322e085995 ("cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

We could have considered switching to device managed APIs for automatic
cleanup, but this will do as well. Thanks Christophe.
--
Florian
Viresh Kumar Dec. 22, 2020, 4:35 a.m. UTC | #2
On 19-12-20, 11:17, Christophe JAILLET wrote:
> If 'cpufreq_register_driver()' fails, we must release the resources
> allocated in 'brcm_avs_prepare_init()' as already done in the remove
> function.
> 
> To do that, introduce a new function 'brcm_avs_prepare_uninit()' in order
> to avoid code duplication. This also makes the code more readable (IMHO).
> 
> Fixes: de322e085995 ("cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> I'm not sure that the existing error handling in the remove function is
> correct and/or needed.
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index 3e31e5d28b79..750ca7cfccb0 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -597,6 +597,16 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static void brcm_avs_prepare_uninit(struct platform_device *pdev)
> +{
> +	struct private_data *priv;
> +
> +	priv = platform_get_drvdata(pdev);
> +
> +	iounmap(priv->avs_intr_base);
> +	iounmap(priv->base);
> +}
> +
>  static int brcm_avs_cpufreq_init(struct cpufreq_policy *policy)
>  {
>  	struct cpufreq_frequency_table *freq_table;
> @@ -732,21 +742,26 @@ static int brcm_avs_cpufreq_probe(struct platform_device *pdev)
>  
>  	brcm_avs_driver.driver_data = pdev;
>  
> -	return cpufreq_register_driver(&brcm_avs_driver);
> +	ret = cpufreq_register_driver(&brcm_avs_driver);
> +	if (ret)
> +		goto err_uninit;
> +
> +	return 0;
> +
> +err_uninit:
> +	brcm_avs_prepare_uninit(pdev);
> +	return ret;

Maybe rewrite as:

	ret = cpufreq_register_driver(&brcm_avs_driver);
	if (ret)
                brcm_avs_prepare_uninit(pdev);
	return ret;

>  }
>  
>  static int brcm_avs_cpufreq_remove(struct platform_device *pdev)
>  {
> -	struct private_data *priv;
>  	int ret;
>  
>  	ret = cpufreq_unregister_driver(&brcm_avs_driver);
>  	if (ret)
>  		return ret;

Instead of returning here, it can be just WARN_ON(ret); and then go on and free
the resources and this needs to be done in a separate patch.

>  
> -	priv = platform_get_drvdata(pdev);
> -	iounmap(priv->base);
> -	iounmap(priv->avs_intr_base);
> +	brcm_avs_prepare_uninit(pdev);
>  
>  	return 0;
>  }
Christophe JAILLET Dec. 27, 2020, 5:22 p.m. UTC | #3
Le 22/12/2020 à 05:35, Viresh Kumar a écrit :
> On 19-12-20, 11:17, Christophe JAILLET wrote:
>> If 'cpufreq_register_driver()' fails, we must release the resources
>> allocated in 'brcm_avs_prepare_init()' as already done in the remove
>> function.
>>
>> To do that, introduce a new function 'brcm_avs_prepare_uninit()' in order
>> to avoid code duplication. This also makes the code more readable (IMHO).
>>
>> Fixes: de322e085995 ("cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> I'm not sure that the existing error handling in the remove function is
>> correct and/or needed.
>> ---
>>   drivers/cpufreq/brcmstb-avs-cpufreq.c | 25 ++++++++++++++++++++-----
>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> index 3e31e5d28b79..750ca7cfccb0 100644
>> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> @@ -597,6 +597,16 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
>>   	return ret;
>>   }
>>   
>> +static void brcm_avs_prepare_uninit(struct platform_device *pdev)
>> +{
>> +	struct private_data *priv;
>> +
>> +	priv = platform_get_drvdata(pdev);
>> +
>> +	iounmap(priv->avs_intr_base);
>> +	iounmap(priv->base);
>> +}
>> +
>>   static int brcm_avs_cpufreq_init(struct cpufreq_policy *policy)
>>   {
>>   	struct cpufreq_frequency_table *freq_table;
>> @@ -732,21 +742,26 @@ static int brcm_avs_cpufreq_probe(struct platform_device *pdev)
>>   
>>   	brcm_avs_driver.driver_data = pdev;
>>   
>> -	return cpufreq_register_driver(&brcm_avs_driver);
>> +	ret = cpufreq_register_driver(&brcm_avs_driver);
>> +	if (ret)
>> +		goto err_uninit;
>> +
>> +	return 0;
>> +
>> +err_uninit:
>> +	brcm_avs_prepare_uninit(pdev);
>> +	return ret;
> 
> Maybe rewrite as:
> 
> 	ret = cpufreq_register_driver(&brcm_avs_driver);
> 	if (ret)
>                  brcm_avs_prepare_uninit(pdev);
> 	return ret;
> 

Personlaly, I prefer what I have proposed. Having a clear and dedicated 
error handling path is more future proff, IMHO.

>>   }
>>   
>>   static int brcm_avs_cpufreq_remove(struct platform_device *pdev)
>>   {
>> -	struct private_data *priv;
>>   	int ret;
>>   
>>   	ret = cpufreq_unregister_driver(&brcm_avs_driver);
>>   	if (ret)
>>   		return ret;
> 
> Instead of returning here, it can be just WARN_ON(ret); and then go on and free
> the resources and this needs to be done in a separate patch.

Ok, I agree (see my comment below the --- in my patch).
I'll send a patch for it when the first patch will be applied, unless 
you prefer if I resend as a serie.

CJ

> 
>>   
>> -	priv = platform_get_drvdata(pdev);
>> -	iounmap(priv->base);
>> -	iounmap(priv->avs_intr_base);
>> +	brcm_avs_prepare_uninit(pdev);
>>   
>>   	return 0;
>>   }
>
Viresh Kumar Dec. 28, 2020, 5:37 a.m. UTC | #4
On 27-12-20, 18:22, Christophe JAILLET wrote:
> Le 22/12/2020 à 05:35, Viresh Kumar a écrit :
> > On 19-12-20, 11:17, Christophe JAILLET wrote:
> > > If 'cpufreq_register_driver()' fails, we must release the resources
> > > allocated in 'brcm_avs_prepare_init()' as already done in the remove
> > > function.
> > > 
> > > To do that, introduce a new function 'brcm_avs_prepare_uninit()' in order
> > > to avoid code duplication. This also makes the code more readable (IMHO).
> > > 
> > > Fixes: de322e085995 ("cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs")
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > > I'm not sure that the existing error handling in the remove function is
> > > correct and/or needed.
> > > ---
> > >   drivers/cpufreq/brcmstb-avs-cpufreq.c | 25 ++++++++++++++++++++-----
> > >   1 file changed, 20 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> > > index 3e31e5d28b79..750ca7cfccb0 100644
> > > --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> > > +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> > > @@ -597,6 +597,16 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
> > >   	return ret;
> > >   }
> > > +static void brcm_avs_prepare_uninit(struct platform_device *pdev)
> > > +{
> > > +	struct private_data *priv;
> > > +
> > > +	priv = platform_get_drvdata(pdev);
> > > +
> > > +	iounmap(priv->avs_intr_base);
> > > +	iounmap(priv->base);
> > > +}
> > > +
> > >   static int brcm_avs_cpufreq_init(struct cpufreq_policy *policy)
> > >   {
> > >   	struct cpufreq_frequency_table *freq_table;
> > > @@ -732,21 +742,26 @@ static int brcm_avs_cpufreq_probe(struct platform_device *pdev)
> > >   	brcm_avs_driver.driver_data = pdev;
> > > -	return cpufreq_register_driver(&brcm_avs_driver);
> > > +	ret = cpufreq_register_driver(&brcm_avs_driver);
> > > +	if (ret)
> > > +		goto err_uninit;
> > > +
> > > +	return 0;
> > > +
> > > +err_uninit:
> > > +	brcm_avs_prepare_uninit(pdev);
> > > +	return ret;
> > 
> > Maybe rewrite as:
> > 
> > 	ret = cpufreq_register_driver(&brcm_avs_driver);
> > 	if (ret)
> >                  brcm_avs_prepare_uninit(pdev);
> > 	return ret;
> > 
> 
> Personlaly, I prefer what I have proposed. Having a clear and dedicated
> error handling path is more future proff, IMHO.

I would have agreed to that if there were other things we were handling in the
error path, but right now we are adding an extra label, goto, etc without any
need. If in future this needs a change, we can always come back to it and update
with a label. But right now I would suggest to keep it simple.

> > >   }
> > >   static int brcm_avs_cpufreq_remove(struct platform_device *pdev)
> > >   {
> > > -	struct private_data *priv;
> > >   	int ret;
> > >   	ret = cpufreq_unregister_driver(&brcm_avs_driver);
> > >   	if (ret)
> > >   		return ret;
> > 
> > Instead of returning here, it can be just WARN_ON(ret); and then go on and free
> > the resources and this needs to be done in a separate patch.
> 
> Ok, I agree (see my comment below the --- in my patch).
> I'll send a patch for it when the first patch will be applied, unless you
> prefer if I resend as a serie.

Based on the above comment from me, I am expecting another version from you for
this patch. So you can fix both the issues in the same patchset.
diff mbox series

Patch

diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
index 3e31e5d28b79..750ca7cfccb0 100644
--- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
+++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
@@ -597,6 +597,16 @@  static int brcm_avs_prepare_init(struct platform_device *pdev)
 	return ret;
 }
 
+static void brcm_avs_prepare_uninit(struct platform_device *pdev)
+{
+	struct private_data *priv;
+
+	priv = platform_get_drvdata(pdev);
+
+	iounmap(priv->avs_intr_base);
+	iounmap(priv->base);
+}
+
 static int brcm_avs_cpufreq_init(struct cpufreq_policy *policy)
 {
 	struct cpufreq_frequency_table *freq_table;
@@ -732,21 +742,26 @@  static int brcm_avs_cpufreq_probe(struct platform_device *pdev)
 
 	brcm_avs_driver.driver_data = pdev;
 
-	return cpufreq_register_driver(&brcm_avs_driver);
+	ret = cpufreq_register_driver(&brcm_avs_driver);
+	if (ret)
+		goto err_uninit;
+
+	return 0;
+
+err_uninit:
+	brcm_avs_prepare_uninit(pdev);
+	return ret;
 }
 
 static int brcm_avs_cpufreq_remove(struct platform_device *pdev)
 {
-	struct private_data *priv;
 	int ret;
 
 	ret = cpufreq_unregister_driver(&brcm_avs_driver);
 	if (ret)
 		return ret;
 
-	priv = platform_get_drvdata(pdev);
-	iounmap(priv->base);
-	iounmap(priv->avs_intr_base);
+	brcm_avs_prepare_uninit(pdev);
 
 	return 0;
 }