[PATCH v2 1/5] driver: refactor probe return value handling into switch statement

[PATCH v2 1/5] driver: refactor probe return value handling into switch statement

[Ahmad Fatoum]

The return values of the bus probe function called inside device_probe()
are classified into 4 categories and they are checked by if statement
distributed across device_probe().

For clarity and easier changes, collect all of them into a switch
statement and while at it, use helpers to make use of %pe, list_move and
list_del_init instead of opencoding them.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - new patch
---
 drivers/base/driver.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 6548aec9b27b..4884e8fda8ef 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -129,12 +129,14 @@ int device_probe(struct device *dev)
 	list_add(&dev->active, &active_device_list);
 
 	ret = dev->bus->probe(dev);
-	if (ret == 0)
-		goto out;
 
-	if (ret == -EPROBE_DEFER) {
-		list_del(&dev->active);
-		list_add(&dev->active, &deferred);
+	depth--;
+
+	switch (ret) {
+	case 0:
+		return 0;
+	case -EPROBE_DEFER:
+		list_move(&dev->active, &deferred);
 
 		/*
 		 * -EPROBE_DEFER should never appear on a deep-probe machine so
@@ -144,19 +146,20 @@ int device_probe(struct device *dev)
 			dev_err(dev, "probe deferred\n");
 		else
 			dev_dbg(dev, "probe deferred\n");
-		goto out;
+
+		return -EPROBE_DEFER;
+	case -ENODEV:
+	case -ENXIO:
+		dev_dbg(dev, "probe failed: %pe\n", ERR_PTR(ret));
+		break;
+	default:
+		dev_err(dev, "probe failed: %pe\n", ERR_PTR(ret));
+		break;
+
 	}
 
-	list_del(&dev->active);
-	INIT_LIST_HEAD(&dev->active);
+	list_del_init(&dev->active);
 
-	if (ret == -ENODEV || ret == -ENXIO)
-		dev_dbg(dev, "probe failed: %s\n", strerror(-ret));
-	else
-		dev_err(dev, "probe failed: %s\n", strerror(-ret));
-
-out:
-	depth--;
 	return ret;
 }
 
-- 
2.39.2

[PATCH v2 2/5] deep-probe: treat any probe deferral as permanent

[Ahmad Fatoum]

As the comment notes, "-EPROBE_DEFER should never appear on a deep-probe
machine so inform the user immediately.". Yet, we still add the device
to the deferred probe list to try it again later, which should only make
a difference if there's a bug with the deep probe mechanism itself.

Therefore, never use the deferred probe list on deep probe system and
directly report any probe deferral as permanent.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - new patch
---
 drivers/base/driver.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 4884e8fda8ef..bb9699ee3795 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -106,6 +106,15 @@ int get_free_deviceid(const char *name_template)
 	};
 }
 
+static void dev_report_permanent_probe_deferral(struct device *dev)
+{
+	if (dev->deferred_probe_reason)
+		dev_err(dev, "probe permanently deferred (%s)\n",
+			dev->deferred_probe_reason);
+	else
+		dev_err(dev, "probe permanently deferred\n");
+}
+
 int device_probe(struct device *dev)
 {
 	static int depth = 0;
@@ -136,17 +145,18 @@ int device_probe(struct device *dev)
 	case 0:
 		return 0;
 	case -EPROBE_DEFER:
-		list_move(&dev->active, &deferred);
-
 		/*
 		 * -EPROBE_DEFER should never appear on a deep-probe machine so
 		 * inform the user immediately.
 		 */
-		if (deep_probe_is_supported())
-			dev_err(dev, "probe deferred\n");
-		else
-			dev_dbg(dev, "probe deferred\n");
+		if (deep_probe_is_supported()) {
+			dev_report_permanent_probe_deferral(dev);
+			break;
+		}
 
+		list_move(&dev->active, &deferred);
+
+		dev_dbg(dev, "probe deferred\n");
 		return -EPROBE_DEFER;
 	case -ENODEV:
 	case -ENXIO:
@@ -155,7 +165,6 @@ int device_probe(struct device *dev)
 	default:
 		dev_err(dev, "probe failed: %pe\n", ERR_PTR(ret));
 		break;
-
 	}
 
 	list_del_init(&dev->active);
@@ -382,13 +391,9 @@ static int device_probe_deferred(void)
 		}
 	} while (success);
 
-	list_for_each_entry(dev, &deferred, active) {
-		if (dev->deferred_probe_reason)
-			dev_err(dev, "probe permanently deferred (%s)\n",
-				dev->deferred_probe_reason);
-		else
-			dev_err(dev, "probe permanently deferred\n");
-	}
+	list_for_each_entry(dev, &deferred, active)
+		dev_report_permanent_probe_deferral(dev);
+
 	return 0;
 }
 late_initcall(device_probe_deferred);
-- 
2.39.2

[PATCH v2 3/5] deep-probe: use IS_ERR_OR_NULL() instead of opencoding

[Ahmad Fatoum]

of_device_create_on_demand either returns a valid pointer, -ENODEV or NULL.
of_device_create_on_demand is a recursive function, which either returns
a valid pointer, ERR_PTR(-ENODEV) or NULL.

Retuning either ERR_PTR(-ENODEV) or NULL is needed for its proper
operation, but of_device_ensure_proped treats both the same, so use
IS_ERR_OR_NULL() to make this apparent.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - new patch
---
 drivers/of/platform.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 060fa3458bd2..0d1dea2db377 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -491,10 +491,8 @@ int of_device_ensure_probed(struct device_node *np)
 		return 0;
 
 	dev = of_device_create_on_demand(np);
-	if (!dev)
+	if (IS_ERR_OR_NULL(dev))
 		return -ENODEV;
-	if (IS_ERR(dev))
-		return PTR_ERR(dev);
 
 	/*
 	 * The deep-probe mechanism relies on the fact that all necessary
-- 
2.39.2

[PATCH v2 4/5] deep-probe: return -EPROBE_DEFER when ensuring probe fails

[Ahmad Fatoum]

-ENODEV is a bad choice for an error code for of_device_ensure_probed.

The function is either called from board code or from driver frameworks,
which usually just propagate the error code with unintended
consequences:

  - A board driver probe function returning -ENODEV is silently skipped

  - A driver framework function returning -ENODEV is often interpreted
    to mean that an optional resource is not specified (e.g. in DT).

In both cases, the user isn't provided an error message and wrong
behavior can crop up later. In my case, the XHCI driver would time out,
because phy_get propagated of_device_ensure_probed's -ENODEV, which was
understood to mean that no PHY is needed and not that the PHY is
specified in the DT, but no driver was bound to it.

Instead of -ENODEV, let's thus return -EPROBE_DEFER. This can be
propagated up to the driver core, which on a deep probe system (the only
ones where of_device_ensure_probed is not a no-op) will print a fat red
error message. We could achieve the same with some other error code, but
-EPROBE_DEFER is what a non-deep-probe system would return when probes
are deferred indefinitely, so symmetry in the deep probe case fits well.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - adapt function kerneldoc documentation (Marco)
  - replace all -ENODEV return values with -EPROBE_DEFER
---
 drivers/of/platform.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 0d1dea2db377..ec1482b27797 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -468,7 +468,7 @@ static struct device *of_device_create_on_demand(struct device_node *np)
 	else
 		dev = of_platform_device_create(np, parent_dev);
 
-	return dev ? : ERR_PTR(-ENODEV);
+	return dev ? : ERR_PTR(-EPROBE_DEFER);
 }
 
 /**
@@ -480,7 +480,7 @@ static struct device *of_device_create_on_demand(struct device_node *np)
  * it.
  *
  * Return: %0 on success
- *	   %-ENODEV if either the device can't be populated, the driver is
+ *	   %-EPROBE_DEFER if either the device can't be populated, the driver is
  *	     missing or the driver probe returns an error.
  */
 int of_device_ensure_probed(struct device_node *np)
@@ -492,7 +492,7 @@ int of_device_ensure_probed(struct device_node *np)
 
 	dev = of_device_create_on_demand(np);
 	if (IS_ERR_OR_NULL(dev))
-		return -ENODEV;
+		return -EPROBE_DEFER;
 
 	/*
 	 * The deep-probe mechanism relies on the fact that all necessary
@@ -502,7 +502,7 @@ int of_device_ensure_probed(struct device_node *np)
 	 * requirements are fulfilled if 'dev->driver' is not NULL.
 	 */
 	if (!dev->driver)
-		return -ENODEV;
+		return -EPROBE_DEFER;
 
 	return 0;
 }
@@ -517,7 +517,7 @@ EXPORT_SYMBOL_GPL(of_device_ensure_probed);
  * populated and probed if found.
  *
  * Return: %0 on success
- *	   %-ENODEV if either the device can't be populated, the driver is
+ *	   %-EPROBE_DEFER if either the device can't be populated, the driver is
  *	     missing or the driver probe returns an error
  *	   %-EINVAL if alias can't be found
  */
@@ -545,7 +545,7 @@ EXPORT_SYMBOL_GPL(of_device_ensure_probed_by_alias);
  * probes devices which match @ids.
  *
  * Return: %0 on success
- *	   %-ENODEV if either the device wasn't found, can't be populated,
+ *	   %-EPROBE_DEFER if either the device wasn't found, can't be populated,
  *	     the driver is missing or the driver probe returns an error
  */
 int of_devices_ensure_probed_by_dev_id(const struct of_device_id *ids)
@@ -578,7 +578,7 @@ EXPORT_SYMBOL_GPL(of_devices_ensure_probed_by_dev_id);
  * devices which matches @property_name.
  *
  * Return: %0 on success
- *	   %-ENODEV if either the device wasn't found, can't be populated,
+ *	   %-EPROBE_DEFER if either the device wasn't found, can't be populated,
  *	     the driver is missing or the driver probe returns an error
  */
 int of_devices_ensure_probed_by_property(const char *property_name)
-- 
2.39.2

[PATCH v2 5/5] phy: freescale: imx8mq-usb: make i.MX8MP support more explicit

[Ahmad Fatoum]

The driver was originally written for use with the i.MX8MQ's DWC3, but
has been extended to also cover the i.MX8MP's DWC3.

While we won't change the config symbol name, because that could break
existing users, we can and should change the symbol prompt and help
text as well as the dependency to reflect that the driver also targets
the i.MX8MP.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - no change
---
 drivers/phy/freescale/Kconfig | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
index 4eb1f9a55c3b..04e8bcf18833 100644
--- a/drivers/phy/freescale/Kconfig
+++ b/drivers/phy/freescale/Kconfig
@@ -1,5 +1,9 @@
 # SPDX-License-Identifier: GPL-2.0-only
-config PHY_FSL_IMX8MQ_USB
-	bool "Freescale i.MX8M USB3 PHY"
-	default ARCH_IMX8MQ
+
+config PHY_FSL_IMX8MQ_USB
+	bool "Freescale i.MX8MQ/P USB3 PHY"
+	default ARCH_IMX8MQ || ARCH_IMX8MP
+	help
+	  Enable this to add support for the USB PHY found on
+	  the i.MX8M Quad and Plus.
 
-- 
2.39.2

Re: [PATCH v2 1/5] driver: refactor probe return value handling into switch statement

[Sascha Hauer]

On Mon, 19 Feb 2024 18:29:21 +0100, Ahmad Fatoum wrote:
> The return values of the bus probe function called inside device_probe()
> are classified into 4 categories and they are checked by if statement
> distributed across device_probe().
> 
> For clarity and easier changes, collect all of them into a switch
> statement and while at it, use helpers to make use of %pe, list_move and
> list_del_init instead of opencoding them.
> 
> [...]

Applied, thanks!

[1/5] driver: refactor probe return value handling into switch statement
      https://git.pengutronix.de/cgit/barebox/commit/?id=6e08b4ab5bec (link may not be stable)
[2/5] deep-probe: treat any probe deferral as permanent
      https://git.pengutronix.de/cgit/barebox/commit/?id=8a42a6bf42b0 (link may not be stable)
[3/5] deep-probe: use IS_ERR_OR_NULL() instead of opencoding
      https://git.pengutronix.de/cgit/barebox/commit/?id=05785e25af00 (link may not be stable)
[4/5] deep-probe: return -EPROBE_DEFER when ensuring probe fails
      https://git.pengutronix.de/cgit/barebox/commit/?id=1905fa7cb425 (link may not be stable)
[5/5] phy: freescale: imx8mq-usb: make i.MX8MP support more explicit
      https://git.pengutronix.de/cgit/barebox/commit/?id=e87ca6e235e4 (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>