[PATCH v2 0/6] PCI: Propagate errors for optional resources

[PATCH v2 0/6] PCI: Propagate errors for optional resources

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

A common pattern exists among several PCI host controller drivers. Some
of the resources that they support are optional, and the way that the
drivers handle these resources is by propagating -EPROBE_DEFER and keep
going without the resource otherwise. However, there can be several
reasons for failing to obtain a resource (e.g. out of memory). Currently
all of these reasons will cause the drivers to consider the optional
resource to not be there. However, if the resource was in fact required
in the specific case and requesting it failed because of some other
reason, the drivers would still happily continue and cause potentially
hard to find problems.

Instead of rolling all error codes into one, reverse the check and only
handle -ENODEV as meaning "resource was not specified". Fatal errors in
that case will cause the driver to fail to probe rather than continuing
as if nothing had happened.

Changes in v2:
- add Rockchip PCI patch which was previously separate
- addressed Bjorn's comments regarding commit message
- collected Reviewed-by, Tested-by and Acked-by tags

Thierry

Thierry Reding (6):
  PCI: rockchip: Propagate errors for optional regulators
  PCI: exynos: Propagate errors for optional PHYs
  PCI: imx6: Propagate errors for optional regulators
  PCI: armada8x: Propagate errors for optional PHYs
  PCI: histb: Propagate errors for optional regulators
  PCI: iproc: Propagate errors for optional PHYs

 drivers/pci/controller/dwc/pci-exynos.c      |  2 +-
 drivers/pci/controller/dwc/pci-imx6.c        |  4 ++--
 drivers/pci/controller/dwc/pcie-armada8k.c   |  7 +++----
 drivers/pci/controller/dwc/pcie-histb.c      |  4 ++--
 drivers/pci/controller/pcie-iproc-platform.c |  9 +++------
 drivers/pci/controller/pcie-rockchip-host.c  | 16 ++++++++--------
 6 files changed, 19 insertions(+), 23 deletions(-)

-- 
2.22.0

[PATCH v2 1/6] PCI: rockchip: Propagate errors for 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 Lin <shawn.lin@rock-chips.com>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: linux-rockchip@lists.infradead.org
Reviewed-by: Andrew Murray <andrew.murray@arm.com>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Shawn Lin <shawn.lin@rock-chips.com>
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");
 	}
 
 	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

[PATCH v2 2/6] PCI: exynos: Propagate errors for optional PHYs

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

devm_of_phy_get() 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 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>
Reviewed-by: Andrew Murray <andrew.murray@arm.com>
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 v2 3/6] PCI: imx6: Propagate errors for 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
Reviewed-by: Andrew Murray <andrew.murray@arm.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

[PATCH v2 4/6] PCI: armada8x: Propagate errors for optional PHYs

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

devm_of_phy_get_by_index() 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 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>
Reviewed-by: Andrew Murray <andrew.murray@arm.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

[PATCH v2 5/6] PCI: histb: Propagate errors for 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>
Reviewed-by: Andrew Murray <andrew.murray@arm.com>
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

[PATCH v2 6/6] PCI: iproc: Propagate errors for optional PHYs

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

devm_phy_get() 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 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
Reviewed-by: Andrew Murray <andrew.murray@arm.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 v2 0/6] PCI: Propagate errors for optional resources

[Lorenzo Pieralisi]

On Thu, Aug 29, 2019 at 12:53:13PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> A common pattern exists among several PCI host controller drivers. Some
> of the resources that they support are optional, and the way that the
> drivers handle these resources is by propagating -EPROBE_DEFER and keep
> going without the resource otherwise. However, there can be several
> reasons for failing to obtain a resource (e.g. out of memory). Currently
> all of these reasons will cause the drivers to consider the optional
> resource to not be there. However, if the resource was in fact required
> in the specific case and requesting it failed because of some other
> reason, the drivers would still happily continue and cause potentially
> hard to find problems.
> 
> Instead of rolling all error codes into one, reverse the check and only
> handle -ENODEV as meaning "resource was not specified". Fatal errors in
> that case will cause the driver to fail to probe rather than continuing
> as if nothing had happened.
> 
> Changes in v2:
> - add Rockchip PCI patch which was previously separate
> - addressed Bjorn's comments regarding commit message
> - collected Reviewed-by, Tested-by and Acked-by tags
> 
> Thierry
> 
> Thierry Reding (6):
>   PCI: rockchip: Propagate errors for optional regulators
>   PCI: exynos: Propagate errors for optional PHYs
>   PCI: imx6: Propagate errors for optional regulators
>   PCI: armada8x: Propagate errors for optional PHYs
>   PCI: histb: Propagate errors for optional regulators
>   PCI: iproc: Propagate errors for optional PHYs
> 
>  drivers/pci/controller/dwc/pci-exynos.c      |  2 +-
>  drivers/pci/controller/dwc/pci-imx6.c        |  4 ++--
>  drivers/pci/controller/dwc/pcie-armada8k.c   |  7 +++----
>  drivers/pci/controller/dwc/pcie-histb.c      |  4 ++--
>  drivers/pci/controller/pcie-iproc-platform.c |  9 +++------
>  drivers/pci/controller/pcie-rockchip-host.c  | 16 ++++++++--------
>  6 files changed, 19 insertions(+), 23 deletions(-)

Applied to pci/misc for v5.4, thanks !

Lorenzo