diff mbox series

PCI: rockchip: Properly handle optional regulators

Message ID 20190828150737.30285-1-thierry.reding@gmail.com (mailing list archive)
State New, archived
Headers show
Series PCI: rockchip: Properly handle optional regulators | expand

Commit Message

Thierry Reding Aug. 28, 2019, 3:07 p.m. UTC 
From: Thierry Reding <treding@nvidia.com>

regulator_get_optional() can fail for a number of reasons besides probe
deferral. It can for example return -ENOMEM if it runs out of memory as
it tries to allocate data structures. Propagating only -EPROBE_DEFER is
problematic because it results in these legitimately fatal errors being
treated as "regulator not specified in DT".

What we really want is to ignore the optional regulators only if they
have not been specified in DT. regulator_get_optional() returns -ENODEV
in this case, so that's the special case that we need to handle. So we
propagate all errors, except -ENODEV, so that real failures will still
cause the driver to fail probe.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/pci/controller/pcie-rockchip-host.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Andrew Murray Aug. 28, 2019, 3:32 p.m. UTC | #1 
On Wed, Aug 28, 2019 at 05:07:37PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> regulator_get_optional() can fail for a number of reasons besides probe
> deferral. It can for example return -ENOMEM if it runs out of memory as
> it tries to allocate data structures. Propagating only -EPROBE_DEFER is
> problematic because it results in these legitimately fatal errors being
> treated as "regulator not specified in DT".
> 
> What we really want is to ignore the optional regulators only if they
> have not been specified in DT. regulator_get_optional() returns -ENODEV
> in this case, so that's the special case that we need to handle. So we
> propagate all errors, except -ENODEV, so that real failures will still
> cause the driver to fail probe.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/pci/controller/pcie-rockchip-host.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index 8d20f1793a61..ef8e677ce9d1 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -608,29 +608,29 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>  
>  	rockchip->vpcie12v = devm_regulator_get_optional(dev, "vpcie12v");
>  	if (IS_ERR(rockchip->vpcie12v)) {
> -		if (PTR_ERR(rockchip->vpcie12v) == -EPROBE_DEFER)
> -			return -EPROBE_DEFER;
> +		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
> +			return PTR_ERR(rockchip->vpcie12v);
>  		dev_info(dev, "no vpcie12v regulator found\n");

In the event that -ENODEV is returned - we don't set vpcie12v to NULL, however
it seems that this is OK as vpcie12v is tested with IS_ERR before use everywhere
else in this file.

By the way it looks like this patch pattern could be applied right across the
kernel, there are also others in PCI: pci-imx6 and pcie-histb.c - not sure if
you wanted to fix those up too.

Reviewed-by: Andrew Murray <andrew.murray@arm.com>

>  	}
>  
>  	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
>  	if (IS_ERR(rockchip->vpcie3v3)) {
> -		if (PTR_ERR(rockchip->vpcie3v3) == -EPROBE_DEFER)
> -			return -EPROBE_DEFER;
> +		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
> +			return PTR_ERR(rockchip->vpcie3v3);
>  		dev_info(dev, "no vpcie3v3 regulator found\n");
>  	}
>  
>  	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
>  	if (IS_ERR(rockchip->vpcie1v8)) {
> -		if (PTR_ERR(rockchip->vpcie1v8) == -EPROBE_DEFER)
> -			return -EPROBE_DEFER;
> +		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
> +			return PTR_ERR(rockchip->vpcie1v8);
>  		dev_info(dev, "no vpcie1v8 regulator found\n");
>  	}
>  
>  	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
>  	if (IS_ERR(rockchip->vpcie0v9)) {
> -		if (PTR_ERR(rockchip->vpcie0v9) == -EPROBE_DEFER)
> -			return -EPROBE_DEFER;
> +		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
> +			return PTR_ERR(rockchip->vpcie0v9);
>  		dev_info(dev, "no vpcie0v9 regulator found\n");
>  	}
>  
> -- 
> 2.22.0
>
Thierry Reding Aug. 28, 2019, 3:41 p.m. UTC | #2 
On Wed, Aug 28, 2019 at 04:32:44PM +0100, Andrew Murray wrote:
> On Wed, Aug 28, 2019 at 05:07:37PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > regulator_get_optional() can fail for a number of reasons besides probe
> > deferral. It can for example return -ENOMEM if it runs out of memory as
> > it tries to allocate data structures. Propagating only -EPROBE_DEFER is
> > problematic because it results in these legitimately fatal errors being
> > treated as "regulator not specified in DT".
> > 
> > What we really want is to ignore the optional regulators only if they
> > have not been specified in DT. regulator_get_optional() returns -ENODEV
> > in this case, so that's the special case that we need to handle. So we
> > propagate all errors, except -ENODEV, so that real failures will still
> > cause the driver to fail probe.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/pci/controller/pcie-rockchip-host.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> > index 8d20f1793a61..ef8e677ce9d1 100644
> > --- a/drivers/pci/controller/pcie-rockchip-host.c
> > +++ b/drivers/pci/controller/pcie-rockchip-host.c
> > @@ -608,29 +608,29 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
> >  
> >  	rockchip->vpcie12v = devm_regulator_get_optional(dev, "vpcie12v");
> >  	if (IS_ERR(rockchip->vpcie12v)) {
> > -		if (PTR_ERR(rockchip->vpcie12v) == -EPROBE_DEFER)
> > -			return -EPROBE_DEFER;
> > +		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
> > +			return PTR_ERR(rockchip->vpcie12v);
> >  		dev_info(dev, "no vpcie12v regulator found\n");
> 
> In the event that -ENODEV is returned - we don't set vpcie12v to NULL, however
> it seems that this is OK as vpcie12v is tested with IS_ERR before use everywhere
> else in this file.

Yeah, I was trying to keep the diff small here. There doesn't seem to be
any canonical way to mark regulators as "not available". This driver
leaves it set as an error pointer, the Tegra PCI driver sets it to NULL.
They're both valid ways to do it as long as they use the proper checks
before using them. So I wasn't trying to force one way or another, just
kept it the way it was and only fixed the problematic checks.

> By the way it looks like this patch pattern could be applied right across the
> kernel, there are also others in PCI: pci-imx6 and pcie-histb.c - not sure if
> you wanted to fix those up too.

I hadn't checked any other drivers, but I can take another look and
prepare patches for them.

> Reviewed-by: Andrew Murray <andrew.murray@arm.com>

Thanks,
Thierry

> >  	}
> >  
> >  	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> >  	if (IS_ERR(rockchip->vpcie3v3)) {
> > -		if (PTR_ERR(rockchip->vpcie3v3) == -EPROBE_DEFER)
> > -			return -EPROBE_DEFER;
> > +		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
> > +			return PTR_ERR(rockchip->vpcie3v3);
> >  		dev_info(dev, "no vpcie3v3 regulator found\n");
> >  	}
> >  
> >  	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
> >  	if (IS_ERR(rockchip->vpcie1v8)) {
> > -		if (PTR_ERR(rockchip->vpcie1v8) == -EPROBE_DEFER)
> > -			return -EPROBE_DEFER;
> > +		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
> > +			return PTR_ERR(rockchip->vpcie1v8);
> >  		dev_info(dev, "no vpcie1v8 regulator found\n");
> >  	}
> >  
> >  	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
> >  	if (IS_ERR(rockchip->vpcie0v9)) {
> > -		if (PTR_ERR(rockchip->vpcie0v9) == -EPROBE_DEFER)
> > -			return -EPROBE_DEFER;
> > +		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
> > +			return PTR_ERR(rockchip->vpcie0v9);
> >  		dev_info(dev, "no vpcie0v9 regulator found\n");
> >  	}
> >  
> > -- 
> > 2.22.0
> >
Heiko Stuebner Aug. 28, 2019, 6:15 p.m. UTC | #3 
Am Mittwoch, 28. August 2019, 17:07:37 CEST schrieb Thierry Reding:
> From: Thierry Reding <treding@nvidia.com>
> 
> regulator_get_optional() can fail for a number of reasons besides probe
> deferral. It can for example return -ENOMEM if it runs out of memory as
> it tries to allocate data structures. Propagating only -EPROBE_DEFER is
> problematic because it results in these legitimately fatal errors being
> treated as "regulator not specified in DT".
> 
> What we really want is to ignore the optional regulators only if they
> have not been specified in DT. regulator_get_optional() returns -ENODEV
> in this case, so that's the special case that we need to handle. So we
> propagate all errors, except -ENODEV, so that real failures will still
> cause the driver to fail probe.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

on a rk3399-gru-scarlet with no 12v regulator defined and pcie-wifi
keeping on working with this patch:

Tested-by: Heiko Stuebner <heiko@sntech.de>

Change itself also looks correct,

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Thanks for doing that cleanup
Heiko
Shawn Lin Aug. 29, 2019, 2:31 a.m. UTC | #4 
On 2019/8/28 23:07, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> regulator_get_optional() can fail for a number of reasons besides probe
> deferral. It can for example return -ENOMEM if it runs out of memory as
> it tries to allocate data structures. Propagating only -EPROBE_DEFER is
> problematic because it results in these legitimately fatal errors being
> treated as "regulator not specified in DT".
> 
> What we really want is to ignore the optional regulators only if they
> have not been specified in DT. regulator_get_optional() returns -ENODEV
> in this case, so that's the special case that we need to handle. So we
> propagate all errors, except -ENODEV, so that real failures will still
> cause the driver to fail probe.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

LGTM,
Acked-by: Shawn Lin <shawn.lin@rock-chips.com>

> ---
>   drivers/pci/controller/pcie-rockchip-host.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index 8d20f1793a61..ef8e677ce9d1 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -608,29 +608,29 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>   
>   	rockchip->vpcie12v = devm_regulator_get_optional(dev, "vpcie12v");
>   	if (IS_ERR(rockchip->vpcie12v)) {
> -		if (PTR_ERR(rockchip->vpcie12v) == -EPROBE_DEFER)
> -			return -EPROBE_DEFER;
> +		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
> +			return PTR_ERR(rockchip->vpcie12v);
>   		dev_info(dev, "no vpcie12v regulator found\n");
>   	}
>   
>   	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
>   	if (IS_ERR(rockchip->vpcie3v3)) {
> -		if (PTR_ERR(rockchip->vpcie3v3) == -EPROBE_DEFER)
> -			return -EPROBE_DEFER;
> +		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
> +			return PTR_ERR(rockchip->vpcie3v3);
>   		dev_info(dev, "no vpcie3v3 regulator found\n");
>   	}
>   
>   	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
>   	if (IS_ERR(rockchip->vpcie1v8)) {
> -		if (PTR_ERR(rockchip->vpcie1v8) == -EPROBE_DEFER)
> -			return -EPROBE_DEFER;
> +		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
> +			return PTR_ERR(rockchip->vpcie1v8);
>   		dev_info(dev, "no vpcie1v8 regulator found\n");
>   	}
>   
>   	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
>   	if (IS_ERR(rockchip->vpcie0v9)) {
> -		if (PTR_ERR(rockchip->vpcie0v9) == -EPROBE_DEFER)
> -			return -EPROBE_DEFER;
> +		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
> +			return PTR_ERR(rockchip->vpcie0v9);
>   		dev_info(dev, "no vpcie0v9 regulator found\n");
>   	}
>   
>
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index 8d20f1793a61..ef8e677ce9d1 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -608,29 +608,29 @@  static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
 
 	rockchip->vpcie12v = devm_regulator_get_optional(dev, "vpcie12v");
 	if (IS_ERR(rockchip->vpcie12v)) {
-		if (PTR_ERR(rockchip->vpcie12v) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
+		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
+			return PTR_ERR(rockchip->vpcie12v);
 		dev_info(dev, "no vpcie12v regulator found\n");
 	}
 
 	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
 	if (IS_ERR(rockchip->vpcie3v3)) {
-		if (PTR_ERR(rockchip->vpcie3v3) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
+		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
+			return PTR_ERR(rockchip->vpcie3v3);
 		dev_info(dev, "no vpcie3v3 regulator found\n");
 	}
 
 	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
 	if (IS_ERR(rockchip->vpcie1v8)) {
-		if (PTR_ERR(rockchip->vpcie1v8) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
+		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
+			return PTR_ERR(rockchip->vpcie1v8);
 		dev_info(dev, "no vpcie1v8 regulator found\n");
 	}
 
 	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
 	if (IS_ERR(rockchip->vpcie0v9)) {
-		if (PTR_ERR(rockchip->vpcie0v9) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
+		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
+			return PTR_ERR(rockchip->vpcie0v9);
 		dev_info(dev, "no vpcie0v9 regulator found\n");
 	}