On niedziela, 13 stycznia 2019 14:16:24 CET Krzysztof Kozlowski wrote:
> On Fri, Jan 11, 2019 at 08:42:44PM +0100, Paweł Chmiel wrote:
> > There is possibility, that when probing driver, regulators are not yet
> > initialized. In this case we should return EPROBE_DEFER and wait till
> > they're initialized, since they're required currently for cpufreq driver
> > to work. Also move regulator initialization code at beginning of probe,
> > so we can defer as fast as posibble.
> >
> > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@xxxxxxxxx>
> > ---
> > Changes from v2:
> > - Handle all error paths in probe
> >
> > Changes from v1:
> > - Fix compilation error
> > - Reorganize code so it's smaller
> > ---
> > drivers/cpufreq/s5pv210-cpufreq.c | 68 ++++++++++++++++++++++---------
> > 1 file changed, 49 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
> > index f51697f1e0b3..627b132e3e61 100644
> > --- a/drivers/cpufreq/s5pv210-cpufreq.c
> > +++ b/drivers/cpufreq/s5pv210-cpufreq.c
> > @@ -584,7 +584,7 @@ static struct notifier_block s5pv210_cpufreq_reboot_notifier = {
> > static int s5pv210_cpufreq_probe(struct platform_device *pdev)
> > {
> > struct device_node *np;
> > - int id;
> > + int id, result = 0;
> >
> > /*
> > * HACK: This is a temporary workaround to get access to clock
> > @@ -594,18 +594,40 @@ static int s5pv210_cpufreq_probe(struct platform_device *pdev)
> > * this whole driver as soon as S5PV210 gets migrated to use
> > * cpufreq-dt driver.
> > */
> > + arm_regulator = regulator_get(NULL, "vddarm");
> > + if (IS_ERR(arm_regulator)) {
> > + if (PTR_ERR(arm_regulator) == -EPROBE_DEFER)
> > + pr_debug("vddarm regulator not ready, defer\n");
> > + else
> > + pr_err("failed to get regulator vddarm\n");
> > + result = PTR_ERR(arm_regulator);
> > + goto err_arm_regulator;
>
> Just return here.
Fixed.
>
> > + }
> > +
> > + int_regulator = regulator_get(NULL, "vddint");
> > + if (IS_ERR(int_regulator)) {
> > + if (PTR_ERR(int_regulator) == -EPROBE_DEFER)
> > + pr_debug("vddint regulator not ready, defer\n");
> > + else
> > + pr_err("failed to get regulator vddint\n");
> > + result = PTR_ERR(int_regulator);
> > + goto err_int_regulator;
> > + }
> > +
> > np = of_find_compatible_node(NULL, NULL, "samsung,s5pv210-clock");
> > if (!np) {
> > pr_err("%s: failed to find clock controller DT node\n",
> > __func__);
> > - return -ENODEV;
> > + result = -ENODEV;
> > + goto err_clock;
> > }
> >
> > clk_base = of_iomap(np, 0);
> > of_node_put(np);
> > if (!clk_base) {
> > pr_err("%s: failed to map clock registers\n", __func__);
> > - return -EFAULT;
> > + result = -EFAULT;
> > + goto err_clock;
> > }
> >
> > for_each_compatible_node(np, NULL, "samsung,s5pv210-dmc") {
> > @@ -614,7 +636,8 @@ static int s5pv210_cpufreq_probe(struct platform_device *pdev)
> > pr_err("%s: failed to get alias of dmc node '%pOFn'\n",
> > __func__, np);
> > of_node_put(np);
> > - return id;
> > + result = id;
> > + goto err_clk_base;
> > }
> >
> > dmc_base[id] = of_iomap(np, 0);
> > @@ -622,33 +645,40 @@ static int s5pv210_cpufreq_probe(struct platform_device *pdev)
> > pr_err("%s: failed to map dmc%d registers\n",
> > __func__, id);
> > of_node_put(np);
> > - return -EFAULT;
> > + result = -EFAULT;
> > + goto err_dmc;
> > }
> > }
> >
> > for (id = 0; id < ARRAY_SIZE(dmc_base); ++id) {
> > if (!dmc_base[id]) {
> > pr_err("%s: failed to find dmc%d node\n", __func__, id);
> > - return -ENODEV;
> > + result = -ENODEV;
> > + goto err_dmc;
> > }
> > }
> >
> > - arm_regulator = regulator_get(NULL, "vddarm");
> > - if (IS_ERR(arm_regulator)) {
> > - pr_err("failed to get regulator vddarm\n");
> > - return PTR_ERR(arm_regulator);
> > - }
> > -
> > - int_regulator = regulator_get(NULL, "vddint");
> > - if (IS_ERR(int_regulator)) {
> > - pr_err("failed to get regulator vddint\n");
> > - regulator_put(arm_regulator);
> > - return PTR_ERR(int_regulator);
> > - }
> > -
> > register_reboot_notifier(&s5pv210_cpufreq_reboot_notifier);
> >
> > return cpufreq_register_driver(&s5pv210_driver);
> > +
> > +err_dmc:
> > + for (id = 0; id < ARRAY_SIZE(dmc_base); ++id)
> > + if (dmc_base[id])
> > + iounmap(dmc_base[id]);
>
> You will iounmap the same pointer twice, if first device bind failed
> while following this error path and someone tries to bind second time
> (again with error leading to this error path). You zero the pointer
> either at beginning of probe or after iounmapping.
Good catch, also fixed in v4.
Thanks
>
> Best regards,
> Krzysztof
>
>
> > +
> > +err_clk_base:
> > + iounmap(clk_base);
> > +
> > +err_clock:
> > + regulator_put(int_regulator);
> > +
> > +err_int_regulator:
> > + regulator_put(arm_regulator);
> > +
> > +err_arm_regulator:
> > +
> > + return result;
> > }
> >
> > static struct platform_driver s5pv210_cpufreq_platdrv = {
>
