* Re: [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling
2015-10-20 13:44 ` Sascha Hauer
@ 2015-10-20 14:36 ` Heiner Kallweit
2015-10-20 15:55 ` [PATCH] cpufreq: imx6q: cleanup resource allocation kbuild test robot
2015-10-21 11:02 ` [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling Viresh Kumar
2 siblings, 0 replies; 6+ messages in thread
From: Heiner Kallweit @ 2015-10-20 14:36 UTC (permalink / raw)
To: Sascha Hauer; +Cc: Heiner Kallweit, Rafael J. Wysocki, Viresh Kumar, linux-pm
On Tue, Oct 20, 2015 at 3:44 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, Oct 20, 2015 at 03:31:02PM +0200, Sascha Hauer wrote:
>> On Tue, Oct 20, 2015 at 03:12:24PM +0200, Heiner Kallweit wrote:
>> > Properly handle the case that regulator_get_optional returns -EPROBE_DEFER
>> > and ignore other errors.
>> >
>> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> > ---
>> > drivers/cpufreq/imx6q-cpufreq.c | 17 ++++++++++++-----
>> > 1 file changed, 12 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
>> > index 0bb33dc..67dc81c 100644
>> > --- a/drivers/cpufreq/imx6q-cpufreq.c
>> > +++ b/drivers/cpufreq/imx6q-cpufreq.c
>> > @@ -67,7 +67,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
>> >
>> > /* scaling up? scale voltage before frequency */
>> > if (new_freq > old_freq) {
>> > - if (!IS_ERR(pu_reg)) {
>> > + if (pu_reg) {
>> > ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
>> > if (ret) {
>> > dev_err(cpu_dev, "failed to scale vddpu up: %d\n", ret);
>> > @@ -124,7 +124,7 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
>> > dev_warn(cpu_dev, "failed to scale vddsoc down: %d\n", ret);
>> > ret = 0;
>> > }
>> > - if (!IS_ERR(pu_reg)) {
>> > + if (pu_reg) {
>> > ret = regulator_set_voltage_tol(pu_reg, imx6_soc_volt[index], 0);
>> > if (ret) {
>> > dev_warn(cpu_dev, "failed to scale vddpu down: %d\n", ret);
>> > @@ -195,6 +195,13 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>> > }
>> >
>> > pu_reg = regulator_get_optional(cpu_dev, "pu");
>> > + if (IS_ERR(pu_reg)) {
>> > + if (PTR_ERR(pu_reg) == -EPROBE_DEFER) {
>> > + ret = -EPROBE_DEFER;
>> > + goto put_reg;
>> > + } else
>> > + pu_reg = NULL;
>> > + }
>> >
>> > soc_reg = regulator_get(cpu_dev, "soc");
>> > if (IS_ERR(soc_reg)) {
>> > @@ -285,7 +292,7 @@ soc_opp_out:
>> > ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
>> > if (ret > 0)
>> > transition_latency += ret * 1000;
>> > - if (!IS_ERR(pu_reg)) {
>> > + if (pu_reg) {
>> > ret = regulator_set_voltage_time(pu_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
>> > if (ret > 0)
>> > transition_latency += ret * 1000;
>> > @@ -325,7 +332,7 @@ out_free_opp:
>> > put_reg:
>> > if (!IS_ERR(arm_reg))
>> > regulator_put(arm_reg);
>> > - if (!IS_ERR(pu_reg))
>> > + if (!IS_ERR_OR_NULL(pu_reg))
>> > regulator_put(pu_reg);
>> > if (!IS_ERR(soc_reg))
>> > regulator_put(soc_reg);
>>
>> These checks do not work properly. We assume here that the pointers are
>> correctly initialized, but this is only true for the first probe. After
>> a -EPROBE_DEFER the pointers are still initialized from the previous probe.
>> We either have to jump between the above IS_ERR checks when the middle
>> regulator failed or explicitly set the *_reg variables to some error
>> value on probe enter.
>
> I think my favourite approach would be to go in the direction as
> indicated below. At least it would be easier to review for correctness
> than the current mix of reverse-cleanup and !IS_ERR checks at the end of probe().
>
Agree. Just two remarks regarding pu_reg:
- IMHO the call to regulator_get_optional can return -EPROBE_DEFER as
well and that
would need to be checked.
- I don't like this "!IS_ERR()" too much every time pu_reg is used.
That's what I addressed
in my second patch. If you share the opinion this could be addressed
in a separate patch
to separate the fix from the improvement.
> Sascha
>
> ------------------------------------8<--------------------------------
>
> From 7661b5b5407bfa4d29ba48059819937daaa2fa32 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Tue, 20 Oct 2015 15:39:05 +0200
> Subject: [PATCH] cpufreq: imx6q: cleanup resource allocation
>
> Put all regulator and clock allocation into a separate function and
> create 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().
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> drivers/cpufreq/imx6q-cpufreq.c | 168 +++++++++++++++++++++++++---------------
> 1 file changed, 104 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index ef1fa81..a049d43 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -176,6 +176,101 @@ static struct cpufreq_driver imx6q_cpufreq_driver = {
> .attr = cpufreq_generic_attr,
> };
>
> +static int imx6q_cpufreq_get_resources(struct platform_device *pdev)
> +{
> + /*
> + * 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");
I think this call can return -EPROBE_DEFER as well.
> +
> + soc_reg = regulator_get(cpu_dev, "soc");
> + if (IS_ERR(soc_reg))
> + return PTR_ERR(soc_reg);
> +
> + return 0;
> +}
> +
> +
> +static int imx6q_cpufreq_put_resources(struct platform_device *pdev)
> +{
> + 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 +293,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(pdev);
> + if (ret)
> + goto drop_resources;
>
> /*
> * We expect an OPP table supplied by platform.
> @@ -239,7 +307,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 */
> @@ -347,28 +415,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;
> }
> @@ -379,17 +428,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;
> }
> --
> 2.6.1
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] cpufreq: imx6q: cleanup resource allocation
2015-10-20 13:44 ` Sascha Hauer
2015-10-20 14:36 ` Heiner Kallweit
@ 2015-10-20 15:55 ` kbuild test robot
2015-10-21 11:02 ` [PATCH 2/2] cpufreq: imx: fix regulator_get_optional error handling Viresh Kumar
2 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2015-10-20 15:55 UTC (permalink / raw)
To: Sascha Hauer
Cc: kbuild-all, Heiner Kallweit, Rafael J. Wysocki, Viresh Kumar,
linux-pm
[-- Attachment #1: Type: text/plain, Size: 7095 bytes --]
Hi Sascha,
[auto build test ERROR on pm/linux-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]
url: https://github.com/0day-ci/linux/commits/Sascha-Hauer/cpufreq-imx6q-cleanup-resource-allocation/20151020-214609
config: arm-imx_v6_v7_defconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All error/warnings (new ones prefixed by >>):
drivers/cpufreq/imx6q-cpufreq.c: In function 'imx6q_cpufreq_put_resources':
>> drivers/cpufreq/imx6q-cpufreq.c:272:1: warning: no return statement in function returning non-void [-Wreturn-type]
}
^
drivers/cpufreq/imx6q-cpufreq.c: In function 'imx6q_cpufreq_probe':
>> drivers/cpufreq/imx6q-cpufreq.c:419:2: error: too few arguments to function 'imx6q_cpufreq_put_resources'
imx6q_cpufreq_put_resources();
^
drivers/cpufreq/imx6q-cpufreq.c:229:12: note: declared here
static int imx6q_cpufreq_put_resources(struct platform_device *pdev)
^
>> drivers/cpufreq/imx6q-cpufreq.c:327:3: error: label 'put_reg' used but not defined
goto put_reg;
^
drivers/cpufreq/imx6q-cpufreq.c: In function 'imx6q_cpufreq_remove':
drivers/cpufreq/imx6q-cpufreq.c:432:2: error: too few arguments to function 'imx6q_cpufreq_put_resources'
imx6q_cpufreq_put_resources();
^
drivers/cpufreq/imx6q-cpufreq.c:229:12: note: declared here
static int imx6q_cpufreq_put_resources(struct platform_device *pdev)
^
vim +/imx6q_cpufreq_put_resources +419 drivers/cpufreq/imx6q-cpufreq.c
266
267 if (!IS_ERR_OR_NULL(secondary_sel_clk))
268 clk_put(secondary_sel_clk);
269 secondary_sel_clk = NULL;
270
271
> 272 }
273
274 static int imx6q_cpufreq_probe(struct platform_device *pdev)
275 {
276 struct device_node *np;
277 struct dev_pm_opp *opp;
278 unsigned long min_volt, max_volt;
279 int num, ret;
280 const struct property *prop;
281 const __be32 *val;
282 u32 nr, i, j;
283
284 cpu_dev = get_cpu_device(0);
285 if (!cpu_dev) {
286 pr_err("failed to get cpu0 device\n");
287 return -ENODEV;
288 }
289
290 np = of_node_get(cpu_dev->of_node);
291 if (!np) {
292 dev_err(cpu_dev, "failed to find cpu0 node\n");
293 return -ENOENT;
294 }
295
296 ret = imx6q_cpufreq_get_resources(pdev);
297 if (ret)
298 goto drop_resources;
299
300 /*
301 * We expect an OPP table supplied by platform.
302 * Just, incase the platform did not supply the OPP
303 * table, it will try to get it.
304 */
305 num = dev_pm_opp_get_opp_count(cpu_dev);
306 if (num < 0) {
307 ret = dev_pm_opp_of_add_table(cpu_dev);
308 if (ret < 0) {
309 dev_err(cpu_dev, "failed to init OPP table: %d\n", ret);
310 goto drop_resources;
311 }
312
313 /* Because we have added the OPPs here, we must free them */
314 free_opp = true;
315
316 num = dev_pm_opp_get_opp_count(cpu_dev);
317 if (num < 0) {
318 ret = num;
319 dev_err(cpu_dev, "no OPP table is found: %d\n", ret);
320 goto out_free_opp;
321 }
322 }
323
324 ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
325 if (ret) {
326 dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
> 327 goto put_reg;
328 }
329
330 /* Make imx6_soc_volt array's size same as arm opp number */
331 imx6_soc_volt = devm_kzalloc(cpu_dev, sizeof(*imx6_soc_volt) * num, GFP_KERNEL);
332 if (imx6_soc_volt == NULL) {
333 ret = -ENOMEM;
334 goto free_freq_table;
335 }
336
337 prop = of_find_property(np, "fsl,soc-operating-points", NULL);
338 if (!prop || !prop->value)
339 goto soc_opp_out;
340
341 /*
342 * Each OPP is a set of tuples consisting of frequency and
343 * voltage like <freq-kHz vol-uV>.
344 */
345 nr = prop->length / sizeof(u32);
346 if (nr % 2 || (nr / 2) < num)
347 goto soc_opp_out;
348
349 for (j = 0; j < num; j++) {
350 val = prop->value;
351 for (i = 0; i < nr / 2; i++) {
352 unsigned long freq = be32_to_cpup(val++);
353 unsigned long volt = be32_to_cpup(val++);
354 if (freq_table[j].frequency == freq) {
355 imx6_soc_volt[soc_opp_count++] = volt;
356 break;
357 }
358 }
359 }
360
361 soc_opp_out:
362 /* use fixed soc opp volt if no valid soc opp info found in dtb */
363 if (soc_opp_count != num) {
364 dev_warn(cpu_dev, "can NOT find valid fsl,soc-operating-points property in dtb, use default value!\n");
365 for (j = 0; j < num; j++)
366 imx6_soc_volt[j] = PU_SOC_VOLTAGE_NORMAL;
367 if (freq_table[num - 1].frequency * 1000 == FREQ_1P2_GHZ)
368 imx6_soc_volt[num - 1] = PU_SOC_VOLTAGE_HIGH;
369 }
370
371 if (of_property_read_u32(np, "clock-latency", &transition_latency))
372 transition_latency = CPUFREQ_ETERNAL;
373
374 /*
375 * Calculate the ramp time for max voltage change in the
376 * VDDSOC and VDDPU regulators.
377 */
378 ret = regulator_set_voltage_time(soc_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
379 if (ret > 0)
380 transition_latency += ret * 1000;
381 if (!IS_ERR(pu_reg)) {
382 ret = regulator_set_voltage_time(pu_reg, imx6_soc_volt[0], imx6_soc_volt[num - 1]);
383 if (ret > 0)
384 transition_latency += ret * 1000;
385 }
386
387 /*
388 * OPP is maintained in order of increasing frequency, and
389 * freq_table initialised from OPP is therefore sorted in the
390 * same order.
391 */
392 rcu_read_lock();
393 opp = dev_pm_opp_find_freq_exact(cpu_dev,
394 freq_table[0].frequency * 1000, true);
395 min_volt = dev_pm_opp_get_voltage(opp);
396 opp = dev_pm_opp_find_freq_exact(cpu_dev,
397 freq_table[--num].frequency * 1000, true);
398 max_volt = dev_pm_opp_get_voltage(opp);
399 rcu_read_unlock();
400 ret = regulator_set_voltage_time(arm_reg, min_volt, max_volt);
401 if (ret > 0)
402 transition_latency += ret * 1000;
403
404 ret = cpufreq_register_driver(&imx6q_cpufreq_driver);
405 if (ret) {
406 dev_err(cpu_dev, "failed register driver: %d\n", ret);
407 goto free_freq_table;
408 }
409
410 of_node_put(np);
411 return 0;
412
413 free_freq_table:
414 dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
415 out_free_opp:
416 if (free_opp)
417 dev_pm_opp_of_remove_table(cpu_dev);
418 drop_resources:
> 419 imx6q_cpufreq_put_resources();
420
421 of_node_put(np);
422 return ret;
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 26987 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread