Re: [PATCH 3/5] cpufreq: imx6q: Fix regulator/clock error handling

[Lucas Stach <l.stach@pengutronix.de>]

Am Mittwoch, den 28.10.2015, 08:59 +0100 schrieb Sascha Hauer:
> When one of the regulator_get/clk_get calls fails the driver just
> returns -ENOENT. To properly handle the -EPROBE_DEFER case we have
> to forward the actual error value instead. This patch fixes that.
> 
> Since all clocks/regulators are global static variables the IS_ERR()
> check in the error path in probe() may erroneously work on the values
> assigned in the previous probe run. This means we have to reinitialize
> the variables between two probe runs().
> 
> The probe function already is quite overloaded, so this patch moves
> the regulator and clock allocation into a separate function and the
> dropping of these resourced into a corresponding cleanup function.
> This is easier to review for correctness than the current mix of
> reverse-cleanup and !IS_ERR checks at the end of probe().
> 
> While at it leave a comment that these resource allocations can't use
> devm_* to prevent people from trying to convert it.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Looks good to me now,

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/cpufreq/imx6q-cpufreq.c | 170 +++++++++++++++++++++++++---------------
>  1 file changed, 106 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index 6fefd19..f81cf8e 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -176,6 +176,103 @@ static struct cpufreq_driver imx6q_cpufreq_driver = {
>  	.attr = cpufreq_generic_attr,
>  };
>  
> +static int imx6q_cpufreq_get_resources(void)
> +{
> +	/*
> +	 * Do not use devm_* here. The resources are bound to the
> +	 * cpu_dev and not to this drivers platform_device.
> +	 */
> +	arm_clk = clk_get(cpu_dev, "arm");
> +	if (IS_ERR(arm_clk))
> +		return PTR_ERR(arm_clk);
> +
> +	pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
> +	if (IS_ERR(pll1_sys_clk))
> +		return PTR_ERR(pll1_sys_clk);
> +
> +	pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
> +	if (IS_ERR(pll1_sw_clk))
> +		return PTR_ERR(pll1_sw_clk);
> +
> +	step_clk = clk_get(cpu_dev, "step");
> +	if (IS_ERR(step_clk))
> +		return PTR_ERR(step_clk);
> +
> +	pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
> +	if (IS_ERR(pll2_pfd2_396m_clk))
> +		return PTR_ERR(pll2_pfd2_396m_clk);
> +
> +	if (of_machine_is_compatible("fsl,imx6ul")) {
> +		pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
> +		if (IS_ERR(pll2_bus_clk))
> +			return PTR_ERR(pll2_bus_clk);
> +
> +		secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
> +		if (IS_ERR(secondary_sel_clk))
> +			return PTR_ERR(secondary_sel_clk);
> +	}
> +
> +	arm_reg = regulator_get(cpu_dev, "arm");
> +	if (IS_ERR(arm_reg))
> +		return PTR_ERR(arm_reg);
> +
> +	pu_reg = regulator_get_optional(cpu_dev, "pu");
> +
> +	soc_reg = regulator_get(cpu_dev, "soc");
> +	if (IS_ERR(soc_reg))
> +		return PTR_ERR(soc_reg);
> +
> +	return 0;
> +}
> +
> +
> +static void imx6q_cpufreq_put_resources(void)
> +{
> +	/*
> +	 * Set all resources to NULL here so that they are correctly
> +	 * initialized next time we enter probe()
> +	 */
> +	if (!IS_ERR_OR_NULL(arm_reg))
> +		regulator_put(arm_reg);
> +	arm_reg = NULL;
> +
> +	if (!IS_ERR_OR_NULL(pu_reg))
> +		regulator_put(pu_reg);
> +	pu_reg = NULL;
> +
> +	if (!IS_ERR_OR_NULL(soc_reg))
> +		regulator_put(soc_reg);
> +	soc_reg = NULL;
> +
> +	if (!IS_ERR_OR_NULL(arm_clk))
> +		clk_put(arm_clk);
> +	arm_clk = NULL;
> +
> +	if (!IS_ERR_OR_NULL(pll1_sys_clk))
> +		clk_put(pll1_sys_clk);
> +	pll1_sys_clk = NULL;
> +
> +	if (!IS_ERR_OR_NULL(pll1_sw_clk))
> +		clk_put(pll1_sw_clk);
> +	pll1_sw_clk = NULL;
> +
> +	if (!IS_ERR_OR_NULL(step_clk))
> +		clk_put(step_clk);
> +	step_clk = NULL;
> +
> +	if (!IS_ERR_OR_NULL(pll2_pfd2_396m_clk))
> +		clk_put(pll2_pfd2_396m_clk);
> +	pll2_pfd2_396m_clk = NULL;
> +
> +	if (!IS_ERR_OR_NULL(pll2_bus_clk))
> +		clk_put(pll2_bus_clk);
> +	pll2_bus_clk = NULL;
> +
> +	if (!IS_ERR_OR_NULL(secondary_sel_clk))
> +		clk_put(secondary_sel_clk);
> +	secondary_sel_clk = NULL;
> +}
> +
>  static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np;
> @@ -198,36 +295,9 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  		return -ENOENT;
>  	}
>  
> -	arm_clk = clk_get(cpu_dev, "arm");
> -	pll1_sys_clk = clk_get(cpu_dev, "pll1_sys");
> -	pll1_sw_clk = clk_get(cpu_dev, "pll1_sw");
> -	step_clk = clk_get(cpu_dev, "step");
> -	pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m");
> -	if (IS_ERR(arm_clk) || IS_ERR(pll1_sys_clk) || IS_ERR(pll1_sw_clk) ||
> -	    IS_ERR(step_clk) || IS_ERR(pll2_pfd2_396m_clk)) {
> -		dev_err(cpu_dev, "failed to get clocks\n");
> -		ret = -ENOENT;
> -		goto put_clk;
> -	}
> -
> -	if (of_machine_is_compatible("fsl,imx6ul")) {
> -		pll2_bus_clk = clk_get(cpu_dev, "pll2_bus");
> -		secondary_sel_clk = clk_get(cpu_dev, "secondary_sel");
> -		if (IS_ERR(pll2_bus_clk) || IS_ERR(secondary_sel_clk)) {
> -			dev_err(cpu_dev, "failed to get clocks specific to imx6ul\n");
> -			ret = -ENOENT;
> -			goto put_clk;
> -		}
> -	}
> -
> -	arm_reg = regulator_get(cpu_dev, "arm");
> -	pu_reg = regulator_get_optional(cpu_dev, "pu");
> -	soc_reg = regulator_get(cpu_dev, "soc");
> -	if (IS_ERR(arm_reg) || IS_ERR(soc_reg)) {
> -		dev_err(cpu_dev, "failed to get regulators\n");
> -		ret = -ENOENT;
> -		goto put_reg;
> -	}
> +	ret = imx6q_cpufreq_get_resources();
> +	if (ret)
> +		goto drop_resources;
>  
>  	/*
>  	 * We expect an OPP table supplied by platform.
> @@ -239,7 +309,7 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  		ret = dev_pm_opp_of_add_table(cpu_dev);
>  		if (ret < 0) {
>  			dev_err(cpu_dev, "failed to init OPP table: %d\n", ret);
> -			goto put_reg;
> +			goto drop_resources;
>  		}
>  
>  		/* Because we have added the OPPs here, we must free them */
> @@ -348,28 +418,9 @@ free_freq_table:
>  out_free_opp:
>  	if (free_opp)
>  		dev_pm_opp_of_remove_table(cpu_dev);
> -put_reg:
> -	if (!IS_ERR(arm_reg))
> -		regulator_put(arm_reg);
> -	if (!IS_ERR(pu_reg))
> -		regulator_put(pu_reg);
> -	if (!IS_ERR(soc_reg))
> -		regulator_put(soc_reg);
> -put_clk:
> -	if (!IS_ERR(arm_clk))
> -		clk_put(arm_clk);
> -	if (!IS_ERR(pll1_sys_clk))
> -		clk_put(pll1_sys_clk);
> -	if (!IS_ERR(pll1_sw_clk))
> -		clk_put(pll1_sw_clk);
> -	if (!IS_ERR(step_clk))
> -		clk_put(step_clk);
> -	if (!IS_ERR(pll2_pfd2_396m_clk))
> -		clk_put(pll2_pfd2_396m_clk);
> -	if (!IS_ERR(pll2_bus_clk))
> -		clk_put(pll2_bus_clk);
> -	if (!IS_ERR(secondary_sel_clk))
> -		clk_put(secondary_sel_clk);
> +drop_resources:
> +	imx6q_cpufreq_put_resources();
> +
>  	of_node_put(np);
>  	return ret;
>  }
> @@ -380,17 +431,8 @@ static int imx6q_cpufreq_remove(struct platform_device *pdev)
>  	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
>  	if (free_opp)
>  		dev_pm_opp_of_remove_table(cpu_dev);
> -	regulator_put(arm_reg);
> -	if (!IS_ERR(pu_reg))
> -		regulator_put(pu_reg);
> -	regulator_put(soc_reg);
> -	clk_put(arm_clk);
> -	clk_put(pll1_sys_clk);
> -	clk_put(pll1_sw_clk);
> -	clk_put(step_clk);
> -	clk_put(pll2_pfd2_396m_clk);
> -	clk_put(pll2_bus_clk);
> -	clk_put(secondary_sel_clk);
> +
> +	imx6q_cpufreq_put_resources();
>  
>  	return 0;
>  }

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |