| Message ID | 1445342218-12895-1-git-send-email-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am Dienstag, den 20.10.2015, 13:56 +0200 schrieb Sascha Hauer: > Use devm_* functions for getting clocks and regulators. This simplfifies > the error path. Also return the error from the failing call instead of > collating all regulator and clock errors to -ENOENT. With this the > driver can properly handle -EPROBE_DEFER. Handling deferred probing > becomes necessary once the SoC internal regulators get supplies. While > the SoC internal regulators are registered early in architecture code, > the supplies may be regular I2C devices which may be probed after the > cpufreq driver. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> NACK This has come up several times before, maybe we should get a comment in the code. Using devm_ here is NOT safe. The cpu_dev is not the cpufreq device and will not be unbound if something return -EPROBE-DEFER in the cpufreq driver, so using devm will result in resource leaks. Please look at the git history of this file. Regards, Lucas > --- > drivers/cpufreq/imx6q-cpufreq.c | 125 ++++++++++++++++++++-------------------- > 1 file changed, 63 insertions(+), 62 deletions(-) > > diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c > index ef1fa81..f48f90a 100644 > --- a/drivers/cpufreq/imx6q-cpufreq.c > +++ b/drivers/cpufreq/imx6q-cpufreq.c > @@ -176,6 +176,56 @@ static struct cpufreq_driver imx6q_cpufreq_driver = { > .attr = cpufreq_generic_attr, > }; > > +static int imx6q_cpufreq_get_clocks(struct platform_device *pdev) > +{ > + arm_clk = devm_clk_get(cpu_dev, "arm"); > + if (IS_ERR(arm_clk)) > + return PTR_ERR(arm_clk); > + > + pll1_sys_clk = devm_clk_get(cpu_dev, "pll1_sys"); > + if (IS_ERR(pll1_sys_clk)) > + return PTR_ERR(pll1_sys_clk); > + > + pll1_sw_clk = devm_clk_get(cpu_dev, "pll1_sw"); > + if (IS_ERR(pll1_sw_clk)) > + return PTR_ERR(pll1_sw_clk); > + > + step_clk = devm_clk_get(cpu_dev, "step"); > + if (IS_ERR(step_clk)) > + return PTR_ERR(step_clk); > + > + pll2_pfd2_396m_clk = devm_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 = devm_clk_get(cpu_dev, "pll2_bus"); > + if (IS_ERR(pll2_bus_clk)) > + return PTR_ERR(pll2_bus_clk); > + > + secondary_sel_clk = devm_clk_get(cpu_dev, "secondary_sel"); > + if (IS_ERR(secondary_sel_clk)) > + return PTR_ERR(secondary_sel_clk); > + } > + > + return 0; > +} > + > +static int imx6q_cpufreq_get_regulators(struct platform_device *pdev) > +{ > + arm_reg = devm_regulator_get(cpu_dev, "arm"); > + if (IS_ERR(arm_reg)) > + return PTR_ERR(arm_reg); > + > + pu_reg = devm_regulator_get_optional(cpu_dev, "pu"); > + > + soc_reg = devm_regulator_get(cpu_dev, "soc"); > + if (IS_ERR(soc_reg)) > + return PTR_ERR(soc_reg); > + > + return 0; > +} > + > static int imx6q_cpufreq_probe(struct platform_device *pdev) > { > struct device_node *np; > @@ -198,35 +248,18 @@ 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; > - } > + ret = imx6q_cpufreq_get_clocks(pdev); > + if (ret) { > + dev_err(cpu_dev, "getting clocks failed with %d\n", ret); > + goto put_node; > } > > - 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_regulators(pdev); > + if (ret) { > + if (ret != -EPROBE_DEFER) > + dev_err(cpu_dev, "getting regulators failed with %d\n", > + ret); > + goto put_node; > } > > /* > @@ -239,7 +272,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 put_node; > } > > /* Because we have added the OPPs here, we must free them */ > @@ -256,7 +289,7 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev) > ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table); > if (ret) { > dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret); > - goto put_reg; > + goto out_free_opp; > } > > /* Make imx6_soc_volt array's size same as arm opp number */ > @@ -347,28 +380,7 @@ 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); > +put_node: > of_node_put(np); > return ret; > } > @@ -379,17 +391,6 @@ 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); > > return 0; > }
On Tue, Oct 20, 2015 at 02:26:58PM +0200, Lucas Stach wrote: > Am Dienstag, den 20.10.2015, 13:56 +0200 schrieb Sascha Hauer: > > Use devm_* functions for getting clocks and regulators. This simplfifies > > the error path. Also return the error from the failing call instead of > > collating all regulator and clock errors to -ENOENT. With this the > > driver can properly handle -EPROBE_DEFER. Handling deferred probing > > becomes necessary once the SoC internal regulators get supplies. While > > the SoC internal regulators are registered early in architecture code, > > the supplies may be regular I2C devices which may be probed after the > > cpufreq driver. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > NACK > > This has come up several times before, maybe we should get a comment in > the code. Indeed, yes. This would help not wasting developing time. Anyway, the driver needs to handle -EPROBE_DEFER properly, if not with devm_ then without it. Sascha
On Tue, Oct 20, 2015 at 2:47 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > On Tue, Oct 20, 2015 at 02:26:58PM +0200, Lucas Stach wrote: >> Am Dienstag, den 20.10.2015, 13:56 +0200 schrieb Sascha Hauer: >> > Use devm_* functions for getting clocks and regulators. This simplfifies >> > the error path. Also return the error from the failing call instead of >> > collating all regulator and clock errors to -ENOENT. With this the >> > driver can properly handle -EPROBE_DEFER. Handling deferred probing >> > becomes necessary once the SoC internal regulators get supplies. While >> > the SoC internal regulators are registered early in architecture code, >> > the supplies may be regular I2C devices which may be probed after the >> > cpufreq driver. >> > >> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> >> >> NACK >> >> This has come up several times before, maybe we should get a comment in >> the code. > > Indeed, yes. This would help not wasting developing time. > > Anyway, the driver needs to handle -EPROBE_DEFER properly, if not with > devm_ then without it. > About two days ago I sent two patches fixing this. Meanwhile Viresh accepted them so they should show up in the next kernel very soon. > Sascha > > -- > 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
On Tue, Oct 20, 2015 at 02:56:51PM +0200, Heiner Kallweit wrote: > On Tue, Oct 20, 2015 at 2:47 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > > On Tue, Oct 20, 2015 at 02:26:58PM +0200, Lucas Stach wrote: > >> Am Dienstag, den 20.10.2015, 13:56 +0200 schrieb Sascha Hauer: > >> > Use devm_* functions for getting clocks and regulators. This simplfifies > >> > the error path. Also return the error from the failing call instead of > >> > collating all regulator and clock errors to -ENOENT. With this the > >> > driver can properly handle -EPROBE_DEFER. Handling deferred probing > >> > becomes necessary once the SoC internal regulators get supplies. While > >> > the SoC internal regulators are registered early in architecture code, > >> > the supplies may be regular I2C devices which may be probed after the > >> > cpufreq driver. > >> > > >> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > >> > >> NACK > >> > >> This has come up several times before, maybe we should get a comment in > >> the code. > > > > Indeed, yes. This would help not wasting developing time. > > > > Anyway, the driver needs to handle -EPROBE_DEFER properly, if not with > > devm_ then without it. > > > About two days ago I sent two patches fixing this. Meanwhile Viresh > accepted them > so they should show up in the next kernel very soon. Thanks for the hint, just found the patches in the archives. Unfortunately your patches do not work properly, but I'll reply to your patch. Sascha
On Tue, Oct 20, 2015 at 3:17 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > On Tue, Oct 20, 2015 at 02:56:51PM +0200, Heiner Kallweit wrote: >> On Tue, Oct 20, 2015 at 2:47 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote: >> > On Tue, Oct 20, 2015 at 02:26:58PM +0200, Lucas Stach wrote: >> >> Am Dienstag, den 20.10.2015, 13:56 +0200 schrieb Sascha Hauer: >> >> > Use devm_* functions for getting clocks and regulators. This simplfifies >> >> > the error path. Also return the error from the failing call instead of >> >> > collating all regulator and clock errors to -ENOENT. With this the >> >> > driver can properly handle -EPROBE_DEFER. Handling deferred probing >> >> > becomes necessary once the SoC internal regulators get supplies. While >> >> > the SoC internal regulators are registered early in architecture code, >> >> > the supplies may be regular I2C devices which may be probed after the >> >> > cpufreq driver. >> >> > >> >> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> >> >> >> >> NACK >> >> >> >> This has come up several times before, maybe we should get a comment in >> >> the code. >> > >> > Indeed, yes. This would help not wasting developing time. >> > >> > Anyway, the driver needs to handle -EPROBE_DEFER properly, if not with >> > devm_ then without it. >> > >> About two days ago I sent two patches fixing this. Meanwhile Viresh >> accepted them >> so they should show up in the next kernel very soon. > > Thanks for the hint, just found the patches in the archives. > Unfortunately your patches do not work properly, but I'll reply to your > patch. > I think the patches do what they're supposed to do: fix handling -EPROBE_DEFER. However I agree that they're not sufficient to fix the other basic problem you pointed out. Therefore lets go with your approach. > Sascha > > -- > 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 |
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c index ef1fa81..f48f90a 100644 --- a/drivers/cpufreq/imx6q-cpufreq.c +++ b/drivers/cpufreq/imx6q-cpufreq.c @@ -176,6 +176,56 @@ static struct cpufreq_driver imx6q_cpufreq_driver = { .attr = cpufreq_generic_attr, }; +static int imx6q_cpufreq_get_clocks(struct platform_device *pdev) +{ + arm_clk = devm_clk_get(cpu_dev, "arm"); + if (IS_ERR(arm_clk)) + return PTR_ERR(arm_clk); + + pll1_sys_clk = devm_clk_get(cpu_dev, "pll1_sys"); + if (IS_ERR(pll1_sys_clk)) + return PTR_ERR(pll1_sys_clk); + + pll1_sw_clk = devm_clk_get(cpu_dev, "pll1_sw"); + if (IS_ERR(pll1_sw_clk)) + return PTR_ERR(pll1_sw_clk); + + step_clk = devm_clk_get(cpu_dev, "step"); + if (IS_ERR(step_clk)) + return PTR_ERR(step_clk); + + pll2_pfd2_396m_clk = devm_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 = devm_clk_get(cpu_dev, "pll2_bus"); + if (IS_ERR(pll2_bus_clk)) + return PTR_ERR(pll2_bus_clk); + + secondary_sel_clk = devm_clk_get(cpu_dev, "secondary_sel"); + if (IS_ERR(secondary_sel_clk)) + return PTR_ERR(secondary_sel_clk); + } + + return 0; +} + +static int imx6q_cpufreq_get_regulators(struct platform_device *pdev) +{ + arm_reg = devm_regulator_get(cpu_dev, "arm"); + if (IS_ERR(arm_reg)) + return PTR_ERR(arm_reg); + + pu_reg = devm_regulator_get_optional(cpu_dev, "pu"); + + soc_reg = devm_regulator_get(cpu_dev, "soc"); + if (IS_ERR(soc_reg)) + return PTR_ERR(soc_reg); + + return 0; +} + static int imx6q_cpufreq_probe(struct platform_device *pdev) { struct device_node *np; @@ -198,35 +248,18 @@ 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; - } + ret = imx6q_cpufreq_get_clocks(pdev); + if (ret) { + dev_err(cpu_dev, "getting clocks failed with %d\n", ret); + goto put_node; } - 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_regulators(pdev); + if (ret) { + if (ret != -EPROBE_DEFER) + dev_err(cpu_dev, "getting regulators failed with %d\n", + ret); + goto put_node; } /* @@ -239,7 +272,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 put_node; } /* Because we have added the OPPs here, we must free them */ @@ -256,7 +289,7 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev) ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table); if (ret) { dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret); - goto put_reg; + goto out_free_opp; } /* Make imx6_soc_volt array's size same as arm opp number */ @@ -347,28 +380,7 @@ 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); +put_node: of_node_put(np); return ret; } @@ -379,17 +391,6 @@ 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); return 0; }
Use devm_* functions for getting clocks and regulators. This simplfifies the error path. Also return the error from the failing call instead of collating all regulator and clock errors to -ENOENT. With this the driver can properly handle -EPROBE_DEFER. Handling deferred probing becomes necessary once the SoC internal regulators get supplies. While the SoC internal regulators are registered early in architecture code, the supplies may be regular I2C devices which may be probed after the cpufreq driver. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/cpufreq/imx6q-cpufreq.c | 125 ++++++++++++++++++++-------------------- 1 file changed, 63 insertions(+), 62 deletions(-)