[PATCH 1/5] PCI: exynos: Properly handle optional PHYs

[PATCH 1/5] PCI: exynos: Properly handle optional PHYs

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

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

What we really want is to ignore the optional PHYs only if they have not
been specified in DT. devm_of_phy_get() 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.

Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Kukjin Kim <kgene@kernel.org>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/pci/controller/dwc/pci-exynos.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
index cee5f2f590e2..14a6ba4067fb 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -465,7 +465,7 @@ static int __init exynos_pcie_probe(struct platform_device *pdev)
 
 	ep->phy = devm_of_phy_get(dev, np, NULL);
 	if (IS_ERR(ep->phy)) {
-		if (PTR_ERR(ep->phy) == -EPROBE_DEFER)
+		if (PTR_ERR(ep->phy) != -ENODEV)
 			return PTR_ERR(ep->phy);
 
 		ep->phy = NULL;
-- 
2.22.0

[PATCH 2/5] PCI: imx6: Properly handle optional regulators

[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.

Cc: Richard Zhu <hongxing.zhu@nxp.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: kernel@pengutronix.de
Cc: linux-imx@nxp.com
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 8b8efa3063f5..acfbd34032a8 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1174,8 +1174,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 
 	imx6_pcie->vpcie = devm_regulator_get_optional(&pdev->dev, "vpcie");
 	if (IS_ERR(imx6_pcie->vpcie)) {
-		if (PTR_ERR(imx6_pcie->vpcie) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
+		if (PTR_ERR(imx6_pcie->vpcie) != -ENODEV)
+			return PTR_ERR(imx6_pcie->vpcie);
 		imx6_pcie->vpcie = NULL;
 	}
 
-- 
2.22.0

Re: [PATCH 2/5] PCI: imx6: Properly handle optional regulators

[Andrew Murray]

On Wed, Aug 28, 2019 at 06:36:33PM +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.
> 
> Cc: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: kernel@pengutronix.de
> Cc: linux-imx@nxp.com
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 8b8efa3063f5..acfbd34032a8 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1174,8 +1174,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
>  
>  	imx6_pcie->vpcie = devm_regulator_get_optional(&pdev->dev, "vpcie");
>  	if (IS_ERR(imx6_pcie->vpcie)) {
> -		if (PTR_ERR(imx6_pcie->vpcie) == -EPROBE_DEFER)
> -			return -EPROBE_DEFER;
> +		if (PTR_ERR(imx6_pcie->vpcie) != -ENODEV)
> +			return PTR_ERR(imx6_pcie->vpcie);

Once you've applied Bjorn's feedback you can add:

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

>  		imx6_pcie->vpcie = NULL;
>  	}
>  
> -- 
> 2.22.0
> 

[PATCH 3/5] PCI: armada8x: Properly handle optional PHYs

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

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

What we really want is to ignore the optional PHYs only if they have not
been specified in DT. devm_of_phy_get_by_index() 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.

Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/pci/controller/dwc/pcie-armada8k.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
index 3d55dc78d999..49596547e8c2 100644
--- a/drivers/pci/controller/dwc/pcie-armada8k.c
+++ b/drivers/pci/controller/dwc/pcie-armada8k.c
@@ -118,11 +118,10 @@ static int armada8k_pcie_setup_phys(struct armada8k_pcie *pcie)
 
 	for (i = 0; i < ARMADA8K_PCIE_MAX_LANES; i++) {
 		pcie->phy[i] = devm_of_phy_get_by_index(dev, node, i);
-		if (IS_ERR(pcie->phy[i]) &&
-		    (PTR_ERR(pcie->phy[i]) == -EPROBE_DEFER))
-			return PTR_ERR(pcie->phy[i]);
-
 		if (IS_ERR(pcie->phy[i])) {
+			if (PTR_ERR(pcie->phy[i]) != -ENODEV)
+				return PTR_ERR(pcie->phy[i]);
+
 			pcie->phy[i] = NULL;
 			continue;
 		}
-- 
2.22.0

Re: [PATCH 3/5] PCI: armada8x: Properly handle optional PHYs

[Andrew Murray]

On Wed, Aug 28, 2019 at 06:36:34PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> devm_of_phy_get_by_index() can fail for a number of resides besides
> probe deferral. It can for example return -ENOMEM if it runs out of
> memory as it tries to allocate devres structures. Propagating only
> -EPROBE_DEFER is problematic because it results in these legitimately
> fatal errors being treated as "PHY not specified in DT".
> 
> What we really want is to ignore the optional PHYs only if they have not
> been specified in DT. devm_of_phy_get_by_index() 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.
> 
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/pci/controller/dwc/pcie-armada8k.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
> index 3d55dc78d999..49596547e8c2 100644
> --- a/drivers/pci/controller/dwc/pcie-armada8k.c
> +++ b/drivers/pci/controller/dwc/pcie-armada8k.c
> @@ -118,11 +118,10 @@ static int armada8k_pcie_setup_phys(struct armada8k_pcie *pcie)
>  
>  	for (i = 0; i < ARMADA8K_PCIE_MAX_LANES; i++) {
>  		pcie->phy[i] = devm_of_phy_get_by_index(dev, node, i);
> -		if (IS_ERR(pcie->phy[i]) &&
> -		    (PTR_ERR(pcie->phy[i]) == -EPROBE_DEFER))
> -			return PTR_ERR(pcie->phy[i]);
> -
>  		if (IS_ERR(pcie->phy[i])) {
> +			if (PTR_ERR(pcie->phy[i]) != -ENODEV)
> +				return PTR_ERR(pcie->phy[i]);
> +

Once you've applied Bjorn's feedback you can add:

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

>  			pcie->phy[i] = NULL;
>  			continue;
>  		}
> -- 
> 2.22.0
> 

[PATCH 4/5] PCI: histb: Properly handle optional regulators

[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.

Cc: Shawn Guo <shawn.guo@linaro.org>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/pci/controller/dwc/pcie-histb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
index 954bc2b74bbc..811b5c6d62ea 100644
--- a/drivers/pci/controller/dwc/pcie-histb.c
+++ b/drivers/pci/controller/dwc/pcie-histb.c
@@ -340,8 +340,8 @@ static int histb_pcie_probe(struct platform_device *pdev)
 
 	hipcie->vpcie = devm_regulator_get_optional(dev, "vpcie");
 	if (IS_ERR(hipcie->vpcie)) {
-		if (PTR_ERR(hipcie->vpcie) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
+		if (PTR_ERR(hipcie->vpcie) != -ENODEV)
+			return PTR_ERR(hipcie->vpcie);
 		hipcie->vpcie = NULL;
 	}
 
-- 
2.22.0

Re: [PATCH 4/5] PCI: histb: Properly handle optional regulators

[Andrew Murray]

On Wed, Aug 28, 2019 at 06:36:35PM +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.
> 
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/pci/controller/dwc/pcie-histb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
> index 954bc2b74bbc..811b5c6d62ea 100644
> --- a/drivers/pci/controller/dwc/pcie-histb.c
> +++ b/drivers/pci/controller/dwc/pcie-histb.c
> @@ -340,8 +340,8 @@ static int histb_pcie_probe(struct platform_device *pdev)
>  
>  	hipcie->vpcie = devm_regulator_get_optional(dev, "vpcie");
>  	if (IS_ERR(hipcie->vpcie)) {
> -		if (PTR_ERR(hipcie->vpcie) == -EPROBE_DEFER)
> -			return -EPROBE_DEFER;
> +		if (PTR_ERR(hipcie->vpcie) != -ENODEV)
> +			return PTR_ERR(hipcie->vpcie);

Once you've applied Bjorn's feedback you can add:

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

>  		hipcie->vpcie = NULL;
>  	}
>  
> -- 
> 2.22.0
> 

[PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

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

What we really want is to ignore the optional PHYs only if they have not
been specified in DT. devm_phy_optional_get() is a function that exactly
does what's required here, so use that instead.

Cc: Ray Jui <rjui@broadcom.com>
Cc: Scott Branden <sbranden@broadcom.com>
Cc: bcm-kernel-feedback-list@broadcom.com
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/pci/controller/pcie-iproc-platform.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/pcie-iproc-platform.c b/drivers/pci/controller/pcie-iproc-platform.c
index 5a3550b6bb29..9ee6200a66f4 100644
--- a/drivers/pci/controller/pcie-iproc-platform.c
+++ b/drivers/pci/controller/pcie-iproc-platform.c
@@ -93,12 +93,9 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
 	pcie->need_ib_cfg = of_property_read_bool(np, "dma-ranges");
 
 	/* PHY use is optional */
-	pcie->phy = devm_phy_get(dev, "pcie-phy");
-	if (IS_ERR(pcie->phy)) {
-		if (PTR_ERR(pcie->phy) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
-		pcie->phy = NULL;
-	}
+	pcie->phy = devm_phy_optional_get(dev, "pcie-phy");
+	if (IS_ERR(pcie->phy))
+		return PTR_ERR(pcie->phy);
 
 	ret = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff, &resources,
 						    &iobase);
-- 
2.22.0

Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Andrew Murray]

On Wed, Aug 28, 2019 at 06:36:36PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> devm_phy_get() can fail for a number of resides besides probe deferral.
> It can for example return -ENOMEM if it runs out of memory as it tries
> to allocate devres structures. Propagating only -EPROBE_DEFER is
> problematic because it results in these legitimately fatal errors being
> treated as "PHY not specified in DT".
> 
> What we really want is to ignore the optional PHYs only if they have not
> been specified in DT. devm_phy_optional_get() is a function that exactly
> does what's required here, so use that instead.
> 
> Cc: Ray Jui <rjui@broadcom.com>
> Cc: Scott Branden <sbranden@broadcom.com>
> Cc: bcm-kernel-feedback-list@broadcom.com
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/pci/controller/pcie-iproc-platform.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-iproc-platform.c b/drivers/pci/controller/pcie-iproc-platform.c
> index 5a3550b6bb29..9ee6200a66f4 100644
> --- a/drivers/pci/controller/pcie-iproc-platform.c
> +++ b/drivers/pci/controller/pcie-iproc-platform.c
> @@ -93,12 +93,9 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
>  	pcie->need_ib_cfg = of_property_read_bool(np, "dma-ranges");
>  
>  	/* PHY use is optional */
> -	pcie->phy = devm_phy_get(dev, "pcie-phy");
> -	if (IS_ERR(pcie->phy)) {
> -		if (PTR_ERR(pcie->phy) == -EPROBE_DEFER)
> -			return -EPROBE_DEFER;
> -		pcie->phy = NULL;
> -	}
> +	pcie->phy = devm_phy_optional_get(dev, "pcie-phy");
> +	if (IS_ERR(pcie->phy))
> +		return PTR_ERR(pcie->phy);

Once you've applied Bjorn's feedback you can add:

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

I initially thought that you forgot to check for -ENODEV - though I can see
that the implementation of devm_phy_optional_get very helpfully does this for
us and returns NULL instead of an error.

What is also confusing is that devm_regulator_get_optional, despite its
_optional suffix doesn't do this and returns an error. I wonder if
devm_phy_optional_get should be changed to return NULL instead of an error
instead of -ENODEV. I've copied Liam/Mark for feedback.

Thanks,

Andrew Murray

>  
>  	ret = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff, &resources,
>  						    &iobase);
> -- 
> 2.22.0
> 

Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1727 bytes --]

On Wed, Aug 28, 2019 at 10:26:55PM +0100, Andrew Murray wrote:

> I initially thought that you forgot to check for -ENODEV - though I can see
> that the implementation of devm_phy_optional_get very helpfully does this for
> us and returns NULL instead of an error.

> What is also confusing is that devm_regulator_get_optional, despite its
> _optional suffix doesn't do this and returns an error. I wonder if
> devm_phy_optional_get should be changed to return NULL instead of an error
> instead of -ENODEV. I've copied Liam/Mark for feedback.

The regulator API has an assumption that people will write bad DTs and
not describe all the regulators in the system, this is especially likely
in cases where consumer drivers initially don't have regulator support
and then get it added since people often only describe supplies actively
used by drivers.  In order to handle this gracefully the API will
substitute in a dummy regulator if it sees that the regulator just isn't
described in the system but a consumer requests it, this will ensure
that for most simple uses the consumer will function fine even if the DT
is not fully described.  Since most devices won't physically work if
some of their supplies are missing this is a good default assumption.  

If a consumer could genuinely have some missing supplies (some devices
do support this for various reasons) then this support would mean that
the consumer would have to have some extra property to say that the
regulator is intentionally missing which would be bad.  Instead what we
do is let the consumer say that real systems could actually be missing
the regulator and that the dummy shouldn't be used so that the consumer
can handle this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Andrew Murray]

On Wed, Aug 28, 2019 at 10:49:01PM +0100, Mark Brown wrote:
> On Wed, Aug 28, 2019 at 10:26:55PM +0100, Andrew Murray wrote:
> 
> > I initially thought that you forgot to check for -ENODEV - though I can see
> > that the implementation of devm_phy_optional_get very helpfully does this for
> > us and returns NULL instead of an error.
> 
> > What is also confusing is that devm_regulator_get_optional, despite its
> > _optional suffix doesn't do this and returns an error. I wonder if
> > devm_phy_optional_get should be changed to return NULL instead of an error
> > instead of -ENODEV. I've copied Liam/Mark for feedback.
> 
> The regulator API has an assumption that people will write bad DTs and
> not describe all the regulators in the system, this is especially likely
> in cases where consumer drivers initially don't have regulator support
> and then get it added since people often only describe supplies actively
> used by drivers.  In order to handle this gracefully the API will
> substitute in a dummy regulator if it sees that the regulator just isn't
> drescribed in the system but a consumer requests it, this will ensure
> that for most simple uses the consumer will function fine even if the DT
> is not fully described.  Since most devices won't physically work if
> some of their supplies are missing this is a good default assumption.  

Right, if I understand correctly this is the behaviour when regulator_get
is called (e.g. NORMAL_GET) - you get a dummy instead of an error.

> 
> If a consumer could genuinely have some missing supplies (some devices
> do support this for various reasons) then this support would mean that
> the consumer would have to have some extra property to say that the
> regulator is intentionally missing which would be bad.  Instead what we
> do is let the consumer say that real systems could actually be missing
> the regulator and that the dummy shouldn't be used so that the consumer
> can handle this.

And if I understand correctly this is the behaviour when
regulator_get_optional is called (e.g. OPTIONAL_GET) - you get -ENODEV
instead of a dummy.

But why do we return -ENODEV and not NULL for OPTIONAL_GET?

Looking at some of the consumer drivers I can see that lots of them don't
correctly handle the return value of regulator_get_optional:

 - some fail their probes and return upon IS_ERR(ret) - for example even
   if -ENODEV is returned.

 - some don't fail their probes and assume the regulator isn't present upon
   IS_ERR(ret) - yet this may not be correct as the regulator may be present
   but -ENOMEM was returned.

Given that a common pattern is to set a consumer regulator pointer to NULL
upon -ENODEV - if regulator_get_optional did this for them, then it would
be more difficult for consumer drivers to get the error handling wrong and
would remove some consumer boiler plate code.

(Of course some consumers won't set a regulator pointer to NULL and instead
test it against IS_ERR instead of NULL everywhere (IS_ERR(NULL) is false) -
but such a change may be a reason to not use IS_ERR everywhere).

As I understand, if a consumer wants to fail upon an absent regulator
it seems the only way they can do this is call regulator_get_optional (which
seems odd) and test for -ENODEV. I'm not sure if there is actually a use-case
for this.

I guess I'm looking here for something that can simplify consumer error
handling - it's easy to get wrong and it seems that many drivers may be wrong.

Thanks,

Andrew Murray

Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4815 bytes --]

On Thu, Aug 29, 2019 at 11:09:34AM +0100, Andrew Murray wrote:
> On Wed, Aug 28, 2019 at 10:49:01PM +0100, Mark Brown wrote:
> > On Wed, Aug 28, 2019 at 10:26:55PM +0100, Andrew Murray wrote:
> > 
> > > I initially thought that you forgot to check for -ENODEV - though I can see
> > > that the implementation of devm_phy_optional_get very helpfully does this for
> > > us and returns NULL instead of an error.
> > 
> > > What is also confusing is that devm_regulator_get_optional, despite its
> > > _optional suffix doesn't do this and returns an error. I wonder if
> > > devm_phy_optional_get should be changed to return NULL instead of an error
> > > instead of -ENODEV. I've copied Liam/Mark for feedback.
> > 
> > The regulator API has an assumption that people will write bad DTs and
> > not describe all the regulators in the system, this is especially likely
> > in cases where consumer drivers initially don't have regulator support
> > and then get it added since people often only describe supplies actively
> > used by drivers.  In order to handle this gracefully the API will
> > substitute in a dummy regulator if it sees that the regulator just isn't
> > drescribed in the system but a consumer requests it, this will ensure
> > that for most simple uses the consumer will function fine even if the DT
> > is not fully described.  Since most devices won't physically work if
> > some of their supplies are missing this is a good default assumption.  
> 
> Right, if I understand correctly this is the behaviour when regulator_get
> is called (e.g. NORMAL_GET) - you get a dummy instead of an error.
> 
> > 
> > If a consumer could genuinely have some missing supplies (some devices
> > do support this for various reasons) then this support would mean that
> > the consumer would have to have some extra property to say that the
> > regulator is intentionally missing which would be bad.  Instead what we
> > do is let the consumer say that real systems could actually be missing
> > the regulator and that the dummy shouldn't be used so that the consumer
> > can handle this.
> 
> And if I understand correctly this is the behaviour when
> regulator_get_optional is called (e.g. OPTIONAL_GET) - you get -ENODEV
> instead of a dummy.
> 
> But why do we return -ENODEV and not NULL for OPTIONAL_GET?
> 
> Looking at some of the consumer drivers I can see that lots of them don't
> correctly handle the return value of regulator_get_optional:
> 
>  - some fail their probes and return upon IS_ERR(ret) - for example even
>    if -ENODEV is returned.
> 
>  - some don't fail their probes and assume the regulator isn't present upon
>    IS_ERR(ret) - yet this may not be correct as the regulator may be present
>    but -ENOMEM was returned.
> 
> Given that a common pattern is to set a consumer regulator pointer to NULL
> upon -ENODEV - if regulator_get_optional did this for them, then it would
> be more difficult for consumer drivers to get the error handling wrong and
> would remove some consumer boiler plate code.
> 
> (Of course some consumers won't set a regulator pointer to NULL and instead
> test it against IS_ERR instead of NULL everywhere (IS_ERR(NULL) is false) -
> but such a change may be a reason to not use IS_ERR everywhere).
> 
> As I understand, if a consumer wants to fail upon an absent regulator
> it seems the only way they can do this is call regulator_get_optional (which
> seems odd) and test for -ENODEV. I'm not sure if there is actually a use-case
> for this.
> 
> I guess I'm looking here for something that can simplify consumer error
> handling - it's easy to get wrong and it seems that many drivers may be wrong.

Agreed. However, this requires a thorough audit of all callers of
regulator_get_optional() to make sure they behave in a sane way. To
further complicate things, unless we want to convert all ~100 callers
in a single patch we need to convert all of them to set the regulator
pointer to NULL on -ENODEV. After that we can make the change to
regulator_get_optional() and only then can we remove the now obsolete
boilerplate from those ~100 callers. Not impossible, but pretty time-
consuming.

While at it, we could also add optional variants to some of the
*phy*_get() functions to convert those as well. Currently there's only
optional variants for phy_get() and devm_phy_get(), but a bunch of
drivers use of_phy_get() or of_phy_get_by_index(). Though especially the
latter isn't very common with optional PHYs, I think.

I also noticed a slightly similar pattern for GPIOs. Perhaps this would
be a good task for someone with good semantic patch skills. Or perhaps
something to add to the janitors' TODO list? Not sure if that's still a
thing, though.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Andrew Murray]

On Thu, Aug 29, 2019 at 12:48:06PM +0200, Thierry Reding wrote:
> On Thu, Aug 29, 2019 at 11:09:34AM +0100, Andrew Murray wrote:
> > On Wed, Aug 28, 2019 at 10:49:01PM +0100, Mark Brown wrote:
> > > On Wed, Aug 28, 2019 at 10:26:55PM +0100, Andrew Murray wrote:
> > > 
> > > > I initially thought that you forgot to check for -ENODEV - though I can see
> > > > that the implementation of devm_phy_optional_get very helpfully does this for
> > > > us and returns NULL instead of an error.
> > > 
> > > > What is also confusing is that devm_regulator_get_optional, despite its
> > > > _optional suffix doesn't do this and returns an error. I wonder if
> > > > devm_phy_optional_get should be changed to return NULL instead of an error
> > > > instead of -ENODEV. I've copied Liam/Mark for feedback.
> > > 
> > > The regulator API has an assumption that people will write bad DTs and
> > > not describe all the regulators in the system, this is especially likely
> > > in cases where consumer drivers initially don't have regulator support
> > > and then get it added since people often only describe supplies actively
> > > used by drivers.  In order to handle this gracefully the API will
> > > substitute in a dummy regulator if it sees that the regulator just isn't
> > > drescribed in the system but a consumer requests it, this will ensure
> > > that for most simple uses the consumer will function fine even if the DT
> > > is not fully described.  Since most devices won't physically work if
> > > some of their supplies are missing this is a good default assumption.  
> > 
> > Right, if I understand correctly this is the behaviour when regulator_get
> > is called (e.g. NORMAL_GET) - you get a dummy instead of an error.
> > 
> > > 
> > > If a consumer could genuinely have some missing supplies (some devices
> > > do support this for various reasons) then this support would mean that
> > > the consumer would have to have some extra property to say that the
> > > regulator is intentionally missing which would be bad.  Instead what we
> > > do is let the consumer say that real systems could actually be missing
> > > the regulator and that the dummy shouldn't be used so that the consumer
> > > can handle this.
> > 
> > And if I understand correctly this is the behaviour when
> > regulator_get_optional is called (e.g. OPTIONAL_GET) - you get -ENODEV
> > instead of a dummy.
> > 
> > But why do we return -ENODEV and not NULL for OPTIONAL_GET?
> > 
> > Looking at some of the consumer drivers I can see that lots of them don't
> > correctly handle the return value of regulator_get_optional:
> > 
> >  - some fail their probes and return upon IS_ERR(ret) - for example even
> >    if -ENODEV is returned.
> > 
> >  - some don't fail their probes and assume the regulator isn't present upon
> >    IS_ERR(ret) - yet this may not be correct as the regulator may be present
> >    but -ENOMEM was returned.
> > 
> > Given that a common pattern is to set a consumer regulator pointer to NULL
> > upon -ENODEV - if regulator_get_optional did this for them, then it would
> > be more difficult for consumer drivers to get the error handling wrong and
> > would remove some consumer boiler plate code.
> > 
> > (Of course some consumers won't set a regulator pointer to NULL and instead
> > test it against IS_ERR instead of NULL everywhere (IS_ERR(NULL) is false) -
> > but such a change may be a reason to not use IS_ERR everywhere).
> > 
> > As I understand, if a consumer wants to fail upon an absent regulator
> > it seems the only way they can do this is call regulator_get_optional (which
> > seems odd) and test for -ENODEV. I'm not sure if there is actually a use-case
> > for this.
> > 
> > I guess I'm looking here for something that can simplify consumer error
> > handling - it's easy to get wrong and it seems that many drivers may be wrong.
> 
> Agreed. However, this requires a thorough audit of all callers of
> regulator_get_optional() to make sure they behave in a sane way. To
> further complicate things, unless we want to convert all ~100 callers
> in a single patch we need to convert all of them to set the regulator
> pointer to NULL on -ENODEV. After that we can make the change to
> regulator_get_optional() and only then can we remove the now obsolete
> boilerplate from those ~100 callers. Not impossible, but pretty time-
> consuming.

This makes sense.

> 
> While at it, we could also add optional variants to some of the
> *phy*_get() functions to convert those as well. Currently there's only
> optional variants for phy_get() and devm_phy_get(), but a bunch of
> drivers use of_phy_get() or of_phy_get_by_index(). Though especially the
> latter isn't very common with optional PHYs, I think.
> 
> I also noticed a slightly similar pattern for GPIOs. Perhaps this would
> be a good task for someone with good semantic patch skills. Or perhaps
> something to add to the janitors' TODO list? Not sure if that's still a
> thing, though.

Yeah I'm pretty sure this applies to several APIs.

Janitors is still around - http://vger.kernel.org/vger-lists.html#kernel-janitors

I think there are others too, such as the Linux Kernel Mentorship Program

Thanks,

Andrew Murray

> 
> Thierry

Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]

On Thu, Aug 29, 2019 at 11:09:34AM +0100, Andrew Murray wrote:

> But why do we return -ENODEV and not NULL for OPTIONAL_GET?

Why would we return NULL?  The caller is going to have to check the
error code anyway since they need to handle -EPROBE_DEFER and NULL is
never a valid thing to use with the regulator API.

> Looking at some of the consumer drivers I can see that lots of them don't
> correctly handle the return value of regulator_get_optional:

This API is *really* commonly abused, especially in the graphics
subsystem which for some reason has lots of users even though I don't
think I've ever seen a sensible use of the API there.  As far as I can
tell the graphics subsystem mostly suffers from people cut'n'pasting
from the Qualcomm BSP which is full of really bad practice.  It's really
frustrating since I will intermittently go through and clean things up
but the uses just keep coming back into the subsystem.

> Given that a common pattern is to set a consumer regulator pointer to NULL
> upon -ENODEV - if regulator_get_optional did this for them, then it would
> be more difficult for consumer drivers to get the error handling wrong and
> would remove some consumer boiler plate code.

No, they'd all still be broken for probe deferral (which is commonly
caused by off-chip devices) and they'd crash as soon as they try to call
any further regulator API functions.

> I guess I'm looking here for something that can simplify consumer error
> handling - it's easy to get wrong and it seems that many drivers may be wrong.

The overwhelming majority of the users just shouldn't be using this API.
If there weren't devices that absolutely *need* this API it wouldn't be
there in the first place since it seems like a magent for bad code, it's
so disappointing how bad the majority of the consumer code is.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4096 bytes --]

On Thu, Aug 29, 2019 at 12:17:29PM +0100, Mark Brown wrote:
> On Thu, Aug 29, 2019 at 11:09:34AM +0100, Andrew Murray wrote:
> 
> > But why do we return -ENODEV and not NULL for OPTIONAL_GET?
> 
> Why would we return NULL?  The caller is going to have to check the
> error code anyway since they need to handle -EPROBE_DEFER and NULL is
> never a valid thing to use with the regulator API.

I think the idea is that consumers would do something like this:

	supply = regulator_get_optional(dev, "foo");
	if (IS_ERR(supply)) {
		/* -EPROBE_DEFER handled as part of this */
		return PTR_ERR(supply);
	}

	/*
	 * Optional supply is NULL here, making it "easier" to check
	 * whether or not to use it.
	 */

I suppose checking using IS_ERR() is equally simple, so this mostly
boils down to taste...

The PHY subsystem, for instance, uses a similar approach but it goes one
step further. All APIs can take NULL as their struct phy * argument and
return success in that case, which makes it really trivial to deal with
optional PHYs. You really don't have to care about them at all after you
get NULL from phy_get_optional().

> > Looking at some of the consumer drivers I can see that lots of them don't
> > correctly handle the return value of regulator_get_optional:
> 
> This API is *really* commonly abused, especially in the graphics
> subsystem which for some reason has lots of users even though I don't
> think I've ever seen a sensible use of the API there.  As far as I can
> tell the graphics subsystem mostly suffers from people cut'n'pasting
> from the Qualcomm BSP which is full of really bad practice.  It's really
> frustrating since I will intermittently go through and clean things up
> but the uses just keep coming back into the subsystem.

The intention here is to make it more difficult to abuse. Obviously if
people keep copying abuses from one driver to another that's a different
story. But if there was no way to abuse the API and if it automatically
did the right thing, even copy/paste abuse would be difficult to pull
off.

> > Given that a common pattern is to set a consumer regulator pointer to NULL
> > upon -ENODEV - if regulator_get_optional did this for them, then it would
> > be more difficult for consumer drivers to get the error handling wrong and
> > would remove some consumer boiler plate code.
> 
> No, they'd all still be broken for probe deferral (which is commonly
> caused by off-chip devices) and they'd crash as soon as they try to call
> any further regulator API functions.

I think what Andrew was suggesting here was to make it easier for people
to determine whether or not an optional regulator was in fact requested
successfully or not. Many consumers already set the optional supply to
NULL and then check for that before calling any regulator API.

If regulator_get_optional() returned NULL for absent optional supplies,
this could be unified across all drivers. And it would allow treating
NULL regulators special, if that's something you'd be willing to do.

In either case, the number of abuses shows that people clearly don't
understand how to use this. So there are two options: a) fix abuse every
time we come across it or b) try to change the API to make it more
difficult to abuse.

> > I guess I'm looking here for something that can simplify consumer error
> > handling - it's easy to get wrong and it seems that many drivers may be wrong.
> 
> The overwhelming majority of the users just shouldn't be using this API.
> If there weren't devices that absolutely *need* this API it wouldn't be
> there in the first place since it seems like a magent for bad code, it's
> so disappointing how bad the majority of the consumer code is.

Yeah, I guess in many cases this API is used as a cheap way to model
always-on, fixed-voltage regulators. It's pretty hard to change those
after the fact, though, because they're usually happening as part of
device tree bindings implementation, so by the time we notice them,
they've often become ABI...

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Andrew Murray]

On Thu, Aug 29, 2019 at 01:46:03PM +0200, Thierry Reding wrote:
> On Thu, Aug 29, 2019 at 12:17:29PM +0100, Mark Brown wrote:
> > On Thu, Aug 29, 2019 at 11:09:34AM +0100, Andrew Murray wrote:
> > 
> > > But why do we return -ENODEV and not NULL for OPTIONAL_GET?
> > 
> > Why would we return NULL?  The caller is going to have to check the
> > error code anyway since they need to handle -EPROBE_DEFER and NULL is
> > never a valid thing to use with the regulator API.
> 
> I think the idea is that consumers would do something like this:
> 
> 	supply = regulator_get_optional(dev, "foo");
> 	if (IS_ERR(supply)) {
> 		/* -EPROBE_DEFER handled as part of this */
> 		return PTR_ERR(supply);
> 	}
> 
> 	/*
> 	 * Optional supply is NULL here, making it "easier" to check
> 	 * whether or not to use it.
> 	 */

Indeed. This way the error path is only for errors (if you consider that
requesting an optional regulator that doesn't exist isn't an error - and
if you also consider that -EPROBE_DEFER is an error, or at least a reason
to bail).

> 
> I suppose checking using IS_ERR() is equally simple, so this mostly
> boils down to taste...
> 
> The PHY subsystem, for instance, uses a similar approach but it goes one
> step further. All APIs can take NULL as their struct phy * argument and
> return success in that case, which makes it really trivial to deal with
> optional PHYs. You really don't have to care about them at all after you
> get NULL from phy_get_optional().

I can see how this makes everything very convenient for the driver, though
this doesn't smell right.

> 
> > > Looking at some of the consumer drivers I can see that lots of them don't
> > > correctly handle the return value of regulator_get_optional:
> > 
> > This API is *really* commonly abused, especially in the graphics
> > subsystem which for some reason has lots of users even though I don't
> > think I've ever seen a sensible use of the API there.  As far as I can
> > tell the graphics subsystem mostly suffers from people cut'n'pasting
> > from the Qualcomm BSP which is full of really bad practice.  It's really
> > frustrating since I will intermittently go through and clean things up
> > but the uses just keep coming back into the subsystem.
> 
> The intention here is to make it more difficult to abuse. Obviously if
> people keep copying abuses from one driver to another that's a different
> story. But if there was no way to abuse the API and if it automatically
> did the right thing, even copy/paste abuse would be difficult to pull
> off.

That's the motativation here.

> 
> > > Given that a common pattern is to set a consumer regulator pointer to NULL
> > > upon -ENODEV - if regulator_get_optional did this for them, then it would
> > > be more difficult for consumer drivers to get the error handling wrong and
> > > would remove some consumer boiler plate code.
> > 
> > No, they'd all still be broken for probe deferral (which is commonly
> > caused by off-chip devices) and they'd crash as soon as they try to call
> > any further regulator API functions.

regulator_get_optional would still return -EPROBE_DEFER and this would be
caught in the above example set out by Thierry.

> 
> I think what Andrew was suggesting here was to make it easier for people
> to determine whether or not an optional regulator was in fact requested
> successfully or not. Many consumers already set the optional supply to
> NULL and then check for that before calling any regulator API.
> 
> If regulator_get_optional() returned NULL for absent optional supplies,
> this could be unified across all drivers. And it would allow treating
> NULL regulators special, if that's something you'd be willing to do.
> 
> In either case, the number of abuses shows that people clearly don't
> understand how to use this. So there are two options: a) fix abuse every
> time we come across it or b) try to change the API to make it more
> difficult to abuse.

Sure. I think we end up with something like:

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e0c0cf462004..67e2a6d7abf6 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1868,6 +1868,9 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
                }
 
                switch (get_type) {
+               case OPTIONAL_GET:
+                       return NULL;
+
                case NORMAL_GET:
                        /*
                         * Assume that a regulator is physically present and


Of course there may be other approaches.

> 
> > > I guess I'm looking here for something that can simplify consumer error
> > > handling - it's easy to get wrong and it seems that many drivers may be wrong.
> > 
> > The overwhelming majority of the users just shouldn't be using this API.
> > If there weren't devices that absolutely *need* this API it wouldn't be
> > there in the first place since it seems like a magent for bad code, it's
> > so disappointing how bad the majority of the consumer code is.
> 
> Yeah, I guess in many cases this API is used as a cheap way to model
> always-on, fixed-voltage regulators. It's pretty hard to change those
> after the fact, though, because they're usually happening as part of
> device tree bindings implementation, so by the time we notice them,
> they've often become ABI...

Thanks,

Andrew Murray

> 
> Thierry

Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1275 bytes --]

On Thu, Aug 29, 2019 at 01:08:35PM +0100, Andrew Murray wrote:
> On Thu, Aug 29, 2019 at 01:46:03PM +0200, Thierry Reding wrote:

> > If regulator_get_optional() returned NULL for absent optional supplies,
> > this could be unified across all drivers. And it would allow treating
> > NULL regulators special, if that's something you'd be willing to do.

> > In either case, the number of abuses shows that people clearly don't
> > understand how to use this. So there are two options: a) fix abuse every
> > time we come across it or b) try to change the API to make it more
> > difficult to abuse.

> Sure. I think we end up with something like:

> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index e0c0cf462004..67e2a6d7abf6 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1868,6 +1868,9 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
>                 }
>  
>                 switch (get_type) {
> +               case OPTIONAL_GET:
> +                       return NULL;
> +

Implementing returning NULL is not hard.  How returning NULL discourages
people from using regulator_get_optional() when they shouldn't be using
it in the first place is not clear to me.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Andrew Murray]

On Thu, Aug 29, 2019 at 02:16:03PM +0100, Mark Brown wrote:
> On Thu, Aug 29, 2019 at 01:08:35PM +0100, Andrew Murray wrote:
> > On Thu, Aug 29, 2019 at 01:46:03PM +0200, Thierry Reding wrote:
> 
> > > If regulator_get_optional() returned NULL for absent optional supplies,
> > > this could be unified across all drivers. And it would allow treating
> > > NULL regulators special, if that's something you'd be willing to do.
> 
> > > In either case, the number of abuses shows that people clearly don't
> > > understand how to use this. So there are two options: a) fix abuse every
> > > time we come across it or b) try to change the API to make it more
> > > difficult to abuse.
> 
> > Sure. I think we end up with something like:
> 
> > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > index e0c0cf462004..67e2a6d7abf6 100644
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -1868,6 +1868,9 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
> >                 }
> >  
> >                 switch (get_type) {
> > +               case OPTIONAL_GET:
> > +                       return NULL;
> > +
> 
> Implementing returning NULL is not hard.  How returning NULL discourages
> people from using regulator_get_optional() when they shouldn't be using
> it in the first place is not clear to me.

I think this is the part I haven't understood until now.

There are many consumer drivers that will not have a regulator specified in
the DT - this may be because they are optional (possibly a rare thing) or
because they don't need to be specified (because they are always on and
require no software interaction)...

Where they are not specified, because there is really no reason for them to
be described in the DT - then these drivers should use regulator_get and
be happy with a dummy regulator. This gives a benefit as if another hardware
version uses the same driver but does have a regulator that needs software
control then we can be confident it will work.

Where regulators are really optional, then regulator_get_optional is used
and -ENODEV can be used by the caller to perform a different course of action
if required. (Does this use-case actually exist?)

I guess I interpreted _optional as 'it's OK if you can't provide a regulator',
whereas the meaning is really 'get me a regulator that may not exist'.

Is my understanding correct? If so I guess another course of action would
be to clean-up users of _optional and convert them to regulator_get where
appropriate?

Thanks,

Andrew Murray

Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]

On Thu, Aug 29, 2019 at 02:43:46PM +0100, Andrew Murray wrote:

> Where they are not specified, because there is really no reason for them to
> be described in the DT - then these drivers should use regulator_get and
> be happy with a dummy regulator. This gives a benefit as if another hardware
> version uses the same driver but does have a regulator that needs software
> control then we can be confident it will work.

The common use case for this is that some boards have flexible power
control which allows them save energy by powering off chips that are not
in use, the driver doesn't super care if it actually gets powered off or
not but it's nice to be able to do so.

> Where regulators are really optional, then regulator_get_optional is used
> and -ENODEV can be used by the caller to perform a different course of action
> if required. (Does this use-case actually exist?)

Yes.  There are two main use cases.  One is for things like MMC where
there's optional supplies that can be used for some more advanced use
cases but their use needs to be negotiated between the host and device,
these typically enable lower power or higher performance modes at the
cost of complexity.  The other is for devices which have the option of
using an internal regulator but can use an external one.  This is
typically used either for performance reasons (the external regulator
might perform better but will increase board cost) or if some systems
need multiple devices to be operating with the same reference voltage.

> I guess I interpreted _optional as 'it's OK if you can't provide a regulator',
> whereas the meaning is really 'get me a regulator that may not exist'.

> Is my understanding correct? If so I guess another course of action would

Yes.

> be to clean-up users of _optional and convert them to regulator_get where
> appropriate?

Yes, I keep doing that intermittently (hence my frustration with more
usages continually popping up in graphics drivers).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 5568 bytes --]

On Thu, Aug 29, 2019 at 01:46:03PM +0200, Thierry Reding wrote:
> On Thu, Aug 29, 2019 at 12:17:29PM +0100, Mark Brown wrote:

> > Why would we return NULL?  The caller is going to have to check the
> > error code anyway since they need to handle -EPROBE_DEFER and NULL is
> > never a valid thing to use with the regulator API.

> I think the idea is that consumers would do something like this:

> 	supply = regulator_get_optional(dev, "foo");
> 	if (IS_ERR(supply)) {
> 		/* -EPROBE_DEFER handled as part of this */
> 		return PTR_ERR(supply);
> 	}

> 	/*
> 	 * Optional supply is NULL here, making it "easier" to check
> 	 * whether or not to use it.
> 	 */

> I suppose checking using IS_ERR() is equally simple, so this mostly
> boils down to taste...

Or setting a flag saying which mode to drive the chip in for example.

> The PHY subsystem, for instance, uses a similar approach but it goes one
> step further. All APIs can take NULL as their struct phy * argument and
> return success in that case, which makes it really trivial to deal with
> optional PHYs. You really don't have to care about them at all after you
> get NULL from phy_get_optional().

That case really doesn't exist for the regulator API, that's the case
which is handled by the dummy regulator in the regulator API - it really
is rare to see devices that operate without power.  I suspect the PHY
case is similar in that there always is a PHY of some kind.  If a
consumer can be written like that then it just shouldn't be using
regulator_get_optional() in the first place.

> > > Looking at some of the consumer drivers I can see that lots of them don't
> > > correctly handle the return value of regulator_get_optional:

> > This API is *really* commonly abused, especially in the graphics
> > subsystem which for some reason has lots of users even though I don't
> > think I've ever seen a sensible use of the API there.  As far as I can
> > tell the graphics subsystem mostly suffers from people cut'n'pasting
> > from the Qualcomm BSP which is full of really bad practice.  It's really
> > frustrating since I will intermittently go through and clean things up
> > but the uses just keep coming back into the subsystem.

> The intention here is to make it more difficult to abuse. Obviously if
> people keep copying abuses from one driver to another that's a different
> story. But if there was no way to abuse the API and if it automatically
> did the right thing, even copy/paste abuse would be difficult to pull
> off.

I fail to see how returning NULL would change anything with regard to
the API being abused, if anything I think it'd make it more likely to
have people write things that break for probe deferral since it's not
reminding people about error codes (that's very marginal though).

> > No, they'd all still be broken for probe deferral (which is commonly
> > caused by off-chip devices) and they'd crash as soon as they try to call
> > any further regulator API functions.

> I think what Andrew was suggesting here was to make it easier for people
> to determine whether or not an optional regulator was in fact requested
> successfully or not. Many consumers already set the optional supply to
> NULL and then check for that before calling any regulator API.

I think that really is very marginal.

> If regulator_get_optional() returned NULL for absent optional supplies,
> this could be unified across all drivers. And it would allow treating
> NULL regulators special, if that's something you'd be willing to do.

Not really, no.  What we're doing at the minute at least mitigates
against the problems we used to have with people just not handling
errors at all and having the dummy regulator gives us some opportunity
to do checks on API usage in the consumers that would otherwise not get
run which helps robustness for the systems where there are real,
controllable regulators providing those supplies.

> In either case, the number of abuses shows that people clearly don't
> understand how to use this. So there are two options: a) fix abuse every
> time we come across it or b) try to change the API to make it more
> difficult to abuse.

I don't think there's much that can be done to avoid abuse of the API, 
I've thought of things like having a #define around the prototype for
"yes I really meant to use this" which consumers would have to define in
an effort to try to flag up to developers and reviewers that it's not
normal.

> > > I guess I'm looking here for something that can simplify consumer error
> > > handling - it's easy to get wrong and it seems that many drivers may be wrong.

> > The overwhelming majority of the users just shouldn't be using this API.
> > If there weren't devices that absolutely *need* this API it wouldn't be
> > there in the first place since it seems like a magent for bad code, it's
> > so disappointing how bad the majority of the consumer code is.

> Yeah, I guess in many cases this API is used as a cheap way to model
> always-on, fixed-voltage regulators. It's pretty hard to change those
> after the fact, though, because they're usually happening as part of
> device tree bindings implementation, so by the time we notice them,
> they've often become ABI...

I don't follow here?  Such users are going to be the common case for the
regulator API and shouldn't have any problems using normal regulator_get().  
When I say users shouldn't be using this API I mean _get_optional()
specifically.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 8531 bytes --]

On Thu, Aug 29, 2019 at 02:03:23PM +0100, Mark Brown wrote:
> On Thu, Aug 29, 2019 at 01:46:03PM +0200, Thierry Reding wrote:
> > On Thu, Aug 29, 2019 at 12:17:29PM +0100, Mark Brown wrote:
> 
> > > Why would we return NULL?  The caller is going to have to check the
> > > error code anyway since they need to handle -EPROBE_DEFER and NULL is
> > > never a valid thing to use with the regulator API.
> 
> > I think the idea is that consumers would do something like this:
> 
> > 	supply = regulator_get_optional(dev, "foo");
> > 	if (IS_ERR(supply)) {
> > 		/* -EPROBE_DEFER handled as part of this */
> > 		return PTR_ERR(supply);
> > 	}
> 
> > 	/*
> > 	 * Optional supply is NULL here, making it "easier" to check
> > 	 * whether or not to use it.
> > 	 */
> 
> > I suppose checking using IS_ERR() is equally simple, so this mostly
> > boils down to taste...
> 
> Or setting a flag saying which mode to drive the chip in for example.
> 
> > The PHY subsystem, for instance, uses a similar approach but it goes one
> > step further. All APIs can take NULL as their struct phy * argument and
> > return success in that case, which makes it really trivial to deal with
> > optional PHYs. You really don't have to care about them at all after you
> > get NULL from phy_get_optional().
> 
> That case really doesn't exist for the regulator API, that's the case
> which is handled by the dummy regulator in the regulator API - it really
> is rare to see devices that operate without power.  I suspect the PHY
> case is similar in that there always is a PHY of some kind.  If a
> consumer can be written like that then it just shouldn't be using
> regulator_get_optional() in the first place.

One recent example, which is actually what spawned this series of
cleanups, that comes to mind here is the case where we need regulators
to supply power to a PCI device. The PCI device that consumes power is
itself not listed in the device tree, because it is usually enumerated
via the PCI bus. However, we still have to provide power to a PCI slot
to make sure the device powers up and can be enumerated in the first
place. For this particular case it's pretty obvious that the supplies
are in fact required.

However, the same PCI controller can also be used with onboard devices
where no standard 3.3 V and 12 V need to be supplied (as in the PCIe
slot case above). Instead there are usually different power supplies
to power the device. Also, since these supplies are usually required to
bring the device up and make it detectable on the PCI bus, these
supplies are typically always on. Also, since the PCI controller is not
the consumer of the power supplies, it's impossible to specify which
supplies exactly are needed by the device (they'd have to be described
by a binding for these devices, but drivers couldn't be requesting them
because without the supplies being enabled the device would never show
up on the PCI bus and the driver would never bind).

Do you think those 3.3 V and 12 V regulators would qualify for use of
the regulator_get_optional() API? Because in the case where the PCIe
controller doesn't drive a PCIe slot, the 3.3 V and 12 V supplies really
are not there.

> > > > Looking at some of the consumer drivers I can see that lots of them don't
> > > > correctly handle the return value of regulator_get_optional:
> 
> > > This API is *really* commonly abused, especially in the graphics
> > > subsystem which for some reason has lots of users even though I don't
> > > think I've ever seen a sensible use of the API there.  As far as I can
> > > tell the graphics subsystem mostly suffers from people cut'n'pasting
> > > from the Qualcomm BSP which is full of really bad practice.  It's really
> > > frustrating since I will intermittently go through and clean things up
> > > but the uses just keep coming back into the subsystem.
> 
> > The intention here is to make it more difficult to abuse. Obviously if
> > people keep copying abuses from one driver to another that's a different
> > story. But if there was no way to abuse the API and if it automatically
> > did the right thing, even copy/paste abuse would be difficult to pull
> > off.
> 
> I fail to see how returning NULL would change anything with regard to
> the API being abused, if anything I think it'd make it more likely to
> have people write things that break for probe deferral since it's not
> reminding people about error codes (that's very marginal though).

People would of course still need to check the return value for errors.
Returning NULL would only make sure that if there's really no regulator
there's a standard way to signal that. And drivers could always use
similar code patterns to deal with optional regulators. Right now there
are two patterns: set regulator to NULL if the regulator is not
available (and use !supply checks before calling regulator APIs) or
leave the regulator set to whatever error pointer was returned (and use
!IS_ERR() checks before calling regulator APIs).

In many cases above, the drivers will continue without a regulator
irrespective of the error code returned. If we return NULL in exactly
only the case where the regulator is not there and an ERR_PTR()
otherwise, it becomes much clearer that all errors should just be
propagated to the caller (including -EPROBE_DEFER) and otherwise we can
continue with appropriate NULL checks.

Again, yeah, this might be marginal.

> > > No, they'd all still be broken for probe deferral (which is commonly
> > > caused by off-chip devices) and they'd crash as soon as they try to call
> > > any further regulator API functions.
> 
> > I think what Andrew was suggesting here was to make it easier for people
> > to determine whether or not an optional regulator was in fact requested
> > successfully or not. Many consumers already set the optional supply to
> > NULL and then check for that before calling any regulator API.
> 
> I think that really is very marginal.
> 
> > If regulator_get_optional() returned NULL for absent optional supplies,
> > this could be unified across all drivers. And it would allow treating
> > NULL regulators special, if that's something you'd be willing to do.
> 
> Not really, no.  What we're doing at the minute at least mitigates
> against the problems we used to have with people just not handling
> errors at all and having the dummy regulator gives us some opportunity
> to do checks on API usage in the consumers that would otherwise not get
> run which helps robustness for the systems where there are real,
> controllable regulators providing those supplies.
> 
> > In either case, the number of abuses shows that people clearly don't
> > understand how to use this. So there are two options: a) fix abuse every
> > time we come across it or b) try to change the API to make it more
> > difficult to abuse.
> 
> I don't think there's much that can be done to avoid abuse of the API, 
> I've thought of things like having a #define around the prototype for
> "yes I really meant to use this" which consumers would have to define in
> an effort to try to flag up to developers and reviewers that it's not
> normal.
> 
> > > > I guess I'm looking here for something that can simplify consumer error
> > > > handling - it's easy to get wrong and it seems that many drivers may be wrong.
> 
> > > The overwhelming majority of the users just shouldn't be using this API.
> > > If there weren't devices that absolutely *need* this API it wouldn't be
> > > there in the first place since it seems like a magent for bad code, it's
> > > so disappointing how bad the majority of the consumer code is.
> 
> > Yeah, I guess in many cases this API is used as a cheap way to model
> > always-on, fixed-voltage regulators. It's pretty hard to change those
> > after the fact, though, because they're usually happening as part of
> > device tree bindings implementation, so by the time we notice them,
> > they've often become ABI...
> 
> I don't follow here?  Such users are going to be the common case for the
> regulator API and shouldn't have any problems using normal regulator_get().  
> When I say users shouldn't be using this API I mean _get_optional()
> specifically.

True, even if the regulator is specified as optional in the bindings,
using regulator_get() would effectively make it optional for the driver
given that it returns the dummy.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3549 bytes --]

On Thu, Aug 29, 2019 at 04:58:20PM +0200, Thierry Reding wrote:

> However, the same PCI controller can also be used with onboard devices
> where no standard 3.3 V and 12 V need to be supplied (as in the PCIe
> slot case above). Instead there are usually different power supplies
> to power the device. Also, since these supplies are usually required to
> bring the device up and make it detectable on the PCI bus, these
> supplies are typically always on. Also, since the PCI controller is not
> the consumer of the power supplies, it's impossible to specify which
> supplies exactly are needed by the device (they'd have to be described
> by a binding for these devices, but drivers couldn't be requesting them
> because without the supplies being enabled the device would never show
> up on the PCI bus and the driver would never bind).

These on board devices that might have non-standard supplies are more of
a problem as you say.  This is a general problem with enumerable buses
that get deployed in embedded contexts, things like clocks and GPIOs can
also be required for enumeration.  Ideally we'd have some pre-probe
callback that could sort these things out but that's core device model
work I've never found the time/enthusiasm to work on.  IIRC there is
already a PCI binding for DT so I guess we could do something like say
it's up to the driver for the PCI device and just call probe() even
without enumeration and require the device to power things up but that
feels very messy.

> Do you think those 3.3 V and 12 V regulators would qualify for use of
> the regulator_get_optional() API? Because in the case where the PCIe
> controller doesn't drive a PCIe slot, the 3.3 V and 12 V supplies really
> are not there.

It doesn't fill me with joy.  I think the main thing is working out
where to attach the supply.

The cleanest and most idiomatic thing from a regulator perspective for
the physical slots would be for the supplies to be attached to the PCI
slot rather than the controller in the DT, even though they will get
driven by the controller driver in practice.  This would mean that
controllers would have optional slot objects with mandatory regulators,
the controller would then have to cope with powering the slot up before
enumerating it but would be able to power it off if nothing's plugged in
and save a bit of power, it also copes with the case where individual
slots have separately controllable power.  I'm not sure how this plays
more generally but since the slots are a physical thing you can point to
on the board it doesn't seem unreasonable.  Every time I see these
supplies attached to the controller for a bus it always feels a bit
wrong, especially in interactions with soldered down devices.

That feels cleaner than saying that the controller has a couple of
optional supplies, it plays a bit better with soldered down devices and
with slots having distinct supplies and it generally feels a bit more
consistent.  I'm not super sure I'm *happy* with this approach though,
it feels a bit awkward with the device model.

> > When I say users shouldn't be using this API I mean _get_optional()
> > specifically.

> True, even if the regulator is specified as optional in the bindings,
> using regulator_get() would effectively make it optional for the driver
> given that it returns the dummy.

Unless the hardware can genuinely cope with there being literally no
power supply there (as opposed to some fixed voltage thing) it probably
shouldn't be specifying the supply as optional in the bindings either :/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/5] PCI: exynos: Properly handle optional PHYs

[Bjorn Helgaas]

This could use a [0/n] cover letter to hold this together as a series.

In my mind, "Properly" adds nothing (why would we merge something done
"improperly"?) and takes space that could be better used for more
specific details.

On Wed, Aug 28, 2019 at 06:36:32PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> devm_of_phy_get() can fail for a number of resides besides probe
> deferral. It can for example return -ENOMEM if it runs out of memory as
> it tries to allocate devres structures. Propagating only -EPROBE_DEFER
> is problematic because it results in these legitimately fatal errors
> being treated as "PHY not specified in DT".

s/resides/reasons/?  (Also in other patches, I think)

> What we really want is to ignore the optional PHYs only if they have not
> been specified in DT. devm_of_phy_get() 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.
> 
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Kukjin Kim <kgene@kernel.org>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/pci/controller/dwc/pci-exynos.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
> index cee5f2f590e2..14a6ba4067fb 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c
> @@ -465,7 +465,7 @@ static int __init exynos_pcie_probe(struct platform_device *pdev)
>  
>  	ep->phy = devm_of_phy_get(dev, np, NULL);
>  	if (IS_ERR(ep->phy)) {
> -		if (PTR_ERR(ep->phy) == -EPROBE_DEFER)
> +		if (PTR_ERR(ep->phy) != -ENODEV)
>  			return PTR_ERR(ep->phy);
>  
>  		ep->phy = NULL;
> -- 
> 2.22.0
> 

Re: [PATCH 1/5] PCI: exynos: Properly handle optional PHYs

[Andrew Murray]

On Wed, Aug 28, 2019 at 06:36:32PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> devm_of_phy_get() can fail for a number of resides besides probe
> deferral. It can for example return -ENOMEM if it runs out of memory as
> it tries to allocate devres structures. Propagating only -EPROBE_DEFER
> is problematic because it results in these legitimately fatal errors
> being treated as "PHY not specified in DT".
> 
> What we really want is to ignore the optional PHYs only if they have not
> been specified in DT. devm_of_phy_get() 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.
> 
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Kukjin Kim <kgene@kernel.org>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/pci/controller/dwc/pci-exynos.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
> index cee5f2f590e2..14a6ba4067fb 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c
> @@ -465,7 +465,7 @@ static int __init exynos_pcie_probe(struct platform_device *pdev)
>  
>  	ep->phy = devm_of_phy_get(dev, np, NULL);
>  	if (IS_ERR(ep->phy)) {
> -		if (PTR_ERR(ep->phy) == -EPROBE_DEFER)
> +		if (PTR_ERR(ep->phy) != -ENODEV)
>  			return PTR_ERR(ep->phy);
>  
>  		ep->phy = NULL;

Once you've applied Bjorn's feedback you can add:

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

> -- 
> 2.22.0
>