Re: [PATCH resent] cpufreq: sparc: Fix exception handling in two functions

[Viresh Kumar]

On 25-03-23, 15:02, Markus Elfring wrote:
> Date: Sat, 18 Mar 2023 11:40:11 +0100
> 
> The label “err_out” was used to jump to another pointer check despite of
> the detail in the implementation of the functions “us2e_freq_init”
> and “us3_freq_init” that it was determined already that the corresponding
> variable contained a null pointer (because of a failed memory allocation).
> 
> 1. Use additional labels.
> 
> 2. Reorder jump targets at the end.
> 
> 3. Delete an extra pointer check which became unnecessary
>    with this refactoring.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac ("Linux-2.6.12-rc2")
> Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/cpufreq/sparc-us2e-cpufreq.c | 12 ++++++------
>  drivers/cpufreq/sparc-us3-cpufreq.c  | 12 ++++++------
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cpufreq/sparc-us2e-cpufreq.c b/drivers/cpufreq/sparc-us2e-cpufreq.c
> index 92acbb25abb3..8534d8b1af56 100644
> --- a/drivers/cpufreq/sparc-us2e-cpufreq.c
> +++ b/drivers/cpufreq/sparc-us2e-cpufreq.c
> @@ -324,12 +324,12 @@ static int __init us2e_freq_init(void)
>  		ret = -ENOMEM;
>  		driver = kzalloc(sizeof(*driver), GFP_KERNEL);
>  		if (!driver)
> -			goto err_out;
> +			goto reset_freq_table;

I would just return error from here.

> 
>  		us2e_freq_table = kzalloc((NR_CPUS * sizeof(*us2e_freq_table)),
>  			GFP_KERNEL);
>  		if (!us2e_freq_table)
> -			goto err_out;
> +			goto free_driver;
> 
>  		driver->init = us2e_freq_cpu_init;
>  		driver->verify = cpufreq_generic_frequency_table_verify;
> @@ -346,11 +346,11 @@ static int __init us2e_freq_init(void)
>  		return 0;
> 
>  err_out:
> -		if (driver) {
> -			kfree(driver);
> -			cpufreq_us2e_driver = NULL;
> -		}
>  		kfree(us2e_freq_table);
> +free_driver:
> +		kfree(driver);
> +		cpufreq_us2e_driver = NULL;
> +reset_freq_table:
>  		us2e_freq_table = NULL;

This wasn't set at this point, no point clearing it here. Also this
clearing of global variables isn't required at all, as after this
point no other function shall get called.

similar comments for the other file.

>  		return ret;
>  	}
> diff --git a/drivers/cpufreq/sparc-us3-cpufreq.c b/drivers/cpufreq/sparc-us3-cpufreq.c
> index e41b35b16afd..325f61bb2e40 100644
> --- a/drivers/cpufreq/sparc-us3-cpufreq.c
> +++ b/drivers/cpufreq/sparc-us3-cpufreq.c
> @@ -172,12 +172,12 @@ static int __init us3_freq_init(void)
>  		ret = -ENOMEM;
>  		driver = kzalloc(sizeof(*driver), GFP_KERNEL);
>  		if (!driver)
> -			goto err_out;
> +			goto reset_freq_table;
> 
>  		us3_freq_table = kzalloc((NR_CPUS * sizeof(*us3_freq_table)),
>  			GFP_KERNEL);
>  		if (!us3_freq_table)
> -			goto err_out;
> +			goto free_driver;
> 
>  		driver->init = us3_freq_cpu_init;
>  		driver->verify = cpufreq_generic_frequency_table_verify;
> @@ -194,11 +194,11 @@ static int __init us3_freq_init(void)
>  		return 0;
> 
>  err_out:
> -		if (driver) {
> -			kfree(driver);
> -			cpufreq_us3_driver = NULL;
> -		}
>  		kfree(us3_freq_table);
> +free_driver:
> +		kfree(driver);
> +		cpufreq_us3_driver = NULL;
> +reset_freq_table:
>  		us3_freq_table = NULL;
>  		return ret;
>  	}
> --
> 2.40.0

-- 
viresh