Linux-ARM-Kernel Archive on lore.kernel.org
 help / Atom feed
* [PATCH v3] cpufreq: s5pv210: Defer probe if getting regulators fail
@ 2019-01-11 19:42 Paweł Chmiel
  2019-01-13 13:16 ` Krzysztof Kozlowski
  0 siblings, 1 reply; 3+ messages in thread
From: Paweł Chmiel @ 2019-01-11 19:42 UTC (permalink / raw)
  To: kgene
  Cc: linux-samsung-soc, linux-pm, viresh.kumar, rjw, linux-kernel,
	krzk, linux-arm-kernel, Paweł Chmiel

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@gmail.com>
---
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;
+	}
+
+	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]);
+
+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 = {
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] cpufreq: s5pv210: Defer probe if getting regulators fail
  2019-01-11 19:42 [PATCH v3] cpufreq: s5pv210: Defer probe if getting regulators fail Paweł Chmiel
@ 2019-01-13 13:16 ` Krzysztof Kozlowski
  2019-01-13 19:54   ` Paweł Chmiel
  0 siblings, 1 reply; 3+ messages in thread
From: Krzysztof Kozlowski @ 2019-01-13 13:16 UTC (permalink / raw)
  To: Paweł Chmiel
  Cc: linux-samsung-soc, linux-pm, viresh.kumar, rjw, linux-kernel,
	kgene, linux-arm-kernel

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@gmail.com>
> ---
> 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.

> +	}
> +
> +	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.

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 = {
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] cpufreq: s5pv210: Defer probe if getting regulators fail
  2019-01-13 13:16 ` Krzysztof Kozlowski
@ 2019-01-13 19:54   ` Paweł Chmiel
  0 siblings, 0 replies; 3+ messages in thread
From: Paweł Chmiel @ 2019-01-13 19:54 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: linux-samsung-soc, linux-pm, viresh.kumar, rjw, linux-kernel,
	kgene, linux-arm-kernel

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@gmail.com>
> > ---
> > 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 = {
> 





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-11 19:42 [PATCH v3] cpufreq: s5pv210: Defer probe if getting regulators fail Paweł Chmiel
2019-01-13 13:16 ` Krzysztof Kozlowski
2019-01-13 19:54   ` Paweł Chmiel

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org infradead-linux-arm-kernel@archiver.kernel.org
	public-inbox-index linux-arm-kernel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox