[PATCH v2] driver: core: Allow subsystems to continue deferring probe

[PATCH v2] driver: core: Allow subsystems to continue deferring probe

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Some subsystems, such as pinctrl, allow continuing to defer probe
indefinitely. This is useful for devices that depend on resources
provided by devices that are only probed after the init stage.

One example of this can be seen on Tegra, where the DPAUX hardware
contains pinmuxing controls for pins that it shares with an I2C
controller. The I2C controller is typically used for communication
with a monitor over HDMI (DDC). However, other instances of the I2C
controller are used to access system critical components, such as a
PMIC. The I2C controller driver will therefore usually be a builtin
driver, whereas the DPAUX driver is part of the display driver that
is loaded from a module to avoid bloating the kernel image with all
of the DRM/KMS subsystem.

In this particular case the pins used by this I2C/DDC controller
become accessible very late in the boot process. However, since the
controller is only used in conjunction with display, that's not an
issue.

Unfortunately the driver core currently outputs a warning message
when a device fails to get the pinctrl before the end of the init
stage. That can be confusing for the user because it may sound like
an unwanted error occurred, whereas it's really an expected and
harmless situation.

In order to eliminate this warning, this patch allows callers of the
driver_deferred_probe_check_state() helper to specify that they want
to continue deferring probe, regardless of whether we're past the
init stage or not. All of the callers of that function are updated
for the new signature, but only the pinctrl subsystem passes a true
value in the new persist parameter if appropriate.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- pass persist flag via flags parameter to make the function call easier
  to understand

 drivers/base/dd.c            | 19 ++++++++++++++-----
 drivers/base/power/domain.c  |  2 +-
 drivers/iommu/of_iommu.c     |  2 +-
 drivers/pinctrl/devicetree.c |  9 +++++----
 include/linux/device.h       | 18 +++++++++++++++++-
 5 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 0df9b4461766..0399a6f6c479 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -238,23 +238,32 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
 /**
  * driver_deferred_probe_check_state() - Check deferred probe state
  * @dev: device to check
+ * @flags: Flags used to control the behavior of this function. Drivers can
+ *   set the DRIVER_DEFER_PROBE_PERSIST flag to indicate that they want to
+ *   keep trying to probe after built-in drivers have had a chance to probe.
+ *   This is useful for built-in drivers that rely on resources provided by
+ *   modular drivers.
  *
  * Returns -ENODEV if init is done and all built-in drivers have had a chance
- * to probe (i.e. initcalls are done), -ETIMEDOUT if deferred probe debug
- * timeout has expired, or -EPROBE_DEFER if none of those conditions are met.
+ * to probe (i.e. initcalls are done) and unless the DRIVER_DEFER_PROBE_PERSIST
+ * flag is set, -ETIMEDOUT if deferred probe debug timeout has expired, or
+ * -EPROBE_DEFER if none of those conditions are met.
  *
  * Drivers or subsystems can opt-in to calling this function instead of directly
  * returning -EPROBE_DEFER.
  */
-int driver_deferred_probe_check_state(struct device *dev)
+int driver_deferred_probe_check_state(struct device *dev, unsigned long flags)
 {
 	if (initcalls_done) {
 		if (!deferred_probe_timeout) {
 			dev_WARN(dev, "deferred probe timeout, ignoring dependency");
 			return -ETIMEDOUT;
 		}
-		dev_warn(dev, "ignoring dependency for device, assuming no driver");
-		return -ENODEV;
+
+		if ((flags & DRIVER_DEFER_PROBE_PERSIST) == 0) {
+			dev_warn(dev, "ignoring dependency for device, assuming no driver");
+			return -ENODEV;
+		}
 	}
 	return -EPROBE_DEFER;
 }
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 33c30c1e6a30..6198c6a30fe2 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2423,7 +2423,7 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
 		mutex_unlock(&gpd_list_lock);
 		dev_dbg(dev, "%s() failed to find PM domain: %ld\n",
 			__func__, PTR_ERR(pd));
-		return driver_deferred_probe_check_state(base_dev);
+		return driver_deferred_probe_check_state(base_dev, 0);
 	}
 
 	dev_dbg(dev, "adding to PM domain %s\n", pd->name);
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 614a93aa5305..b95d4342e414 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -105,7 +105,7 @@ static int of_iommu_xlate(struct device *dev,
 	 * a proper probe-ordering dependency mechanism in future.
 	 */
 	if (!ops)
-		return driver_deferred_probe_check_state(dev);
+		return driver_deferred_probe_check_state(dev, 0);
 
 	return ops->of_xlate(dev, iommu_spec);
 }
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index f7e354f85518..43c0183fa23f 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -111,13 +111,14 @@ static int dt_to_map_one_config(struct pinctrl *p,
 
 		np_pctldev = of_get_next_parent(np_pctldev);
 		if (!np_pctldev || of_node_is_root(np_pctldev)) {
+			unsigned long flags = 0;
+
 			of_node_put(np_pctldev);
-			ret = driver_deferred_probe_check_state(p->dev);
 			/* keep deferring if modules are enabled unless we've timed out */
-			if (IS_ENABLED(CONFIG_MODULES) && !allow_default && ret == -ENODEV)
-				ret = -EPROBE_DEFER;
+			if (IS_ENABLED(CONFIG_MODULES) && !allow_default)
+				flags |= DRIVER_DEFER_PROBE_PERSIST;
 
-			return ret;
+			return driver_deferred_probe_check_state(p->dev, flags);
 		}
 		/* If we're creating a hog we can use the passed pctldev */
 		if (hog_pctldev && (np_pctldev == p->dev->of_node)) {
diff --git a/include/linux/device.h b/include/linux/device.h
index e0649f6adf2e..d364656a920c 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -340,7 +340,23 @@ struct device *driver_find_device(struct device_driver *drv,
 				  int (*match)(struct device *dev, void *data));
 
 void driver_deferred_probe_add(struct device *dev);
-int driver_deferred_probe_check_state(struct device *dev);
+
+/*
+ * This can be use to continue to defer probe after the init stage and after
+ * all the built-in drivers have had a chance to probe. This is useful if a
+ * built-in driver requires resources provided by a modular driver.
+ *
+ * One such example is the pinctrl subsystem, where for example the DPAUX
+ * hardware on Tegra provides pinmuxing controls for pins shared between DPAUX
+ * and I2C controllers. Only a subset of I2C controllers need the DPAUX
+ * pinmuxing, and some I2C controllers are used during early boot for critical
+ * tasks (such as communicating with the system PMIC). The I2C controllers
+ * that don't share pins with a DPAUX block will want to be driven by a built-
+ * in driver to make sure they are available early on.
+ */
+#define DRIVER_DEFER_PROBE_PERSIST (1 << 0)
+
+int driver_deferred_probe_check_state(struct device *dev, unsigned long flags);
 
 /**
  * struct subsys_interface - interfaces to device functions
-- 
2.21.0

Re: [PATCH v2] driver: core: Allow subsystems to continue deferring probe

[Rob Herring]

On Thu, Jun 13, 2019 at 11:00 AM Thierry Reding
<thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@nvidia.com>
>
> Some subsystems, such as pinctrl, allow continuing to defer probe
> indefinitely. This is useful for devices that depend on resources
> provided by devices that are only probed after the init stage.
>
> One example of this can be seen on Tegra, where the DPAUX hardware
> contains pinmuxing controls for pins that it shares with an I2C
> controller. The I2C controller is typically used for communication
> with a monitor over HDMI (DDC). However, other instances of the I2C
> controller are used to access system critical components, such as a
> PMIC. The I2C controller driver will therefore usually be a builtin
> driver, whereas the DPAUX driver is part of the display driver that
> is loaded from a module to avoid bloating the kernel image with all
> of the DRM/KMS subsystem.
>
> In this particular case the pins used by this I2C/DDC controller
> become accessible very late in the boot process. However, since the
> controller is only used in conjunction with display, that's not an
> issue.
>
> Unfortunately the driver core currently outputs a warning message
> when a device fails to get the pinctrl before the end of the init
> stage. That can be confusing for the user because it may sound like
> an unwanted error occurred, whereas it's really an expected and
> harmless situation.
>
> In order to eliminate this warning, this patch allows callers of the
> driver_deferred_probe_check_state() helper to specify that they want
> to continue deferring probe, regardless of whether we're past the
> init stage or not. All of the callers of that function are updated
> for the new signature, but only the pinctrl subsystem passes a true
> value in the new persist parameter if appropriate.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - pass persist flag via flags parameter to make the function call easier
>   to understand
>
>  drivers/base/dd.c            | 19 ++++++++++++++-----
>  drivers/base/power/domain.c  |  2 +-
>  drivers/iommu/of_iommu.c     |  2 +-
>  drivers/pinctrl/devicetree.c |  9 +++++----
>  include/linux/device.h       | 18 +++++++++++++++++-
>  5 files changed, 38 insertions(+), 12 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2] driver: core: Allow subsystems to continue deferring probe

[Rafael J. Wysocki]

On Thu, Jun 13, 2019 at 7:00 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@nvidia.com>
>
> Some subsystems, such as pinctrl, allow continuing to defer probe
> indefinitely. This is useful for devices that depend on resources
> provided by devices that are only probed after the init stage.
>
> One example of this can be seen on Tegra, where the DPAUX hardware
> contains pinmuxing controls for pins that it shares with an I2C
> controller. The I2C controller is typically used for communication
> with a monitor over HDMI (DDC). However, other instances of the I2C
> controller are used to access system critical components, such as a
> PMIC. The I2C controller driver will therefore usually be a builtin
> driver, whereas the DPAUX driver is part of the display driver that
> is loaded from a module to avoid bloating the kernel image with all
> of the DRM/KMS subsystem.
>
> In this particular case the pins used by this I2C/DDC controller
> become accessible very late in the boot process. However, since the
> controller is only used in conjunction with display, that's not an
> issue.
>
> Unfortunately the driver core currently outputs a warning message
> when a device fails to get the pinctrl before the end of the init
> stage. That can be confusing for the user because it may sound like
> an unwanted error occurred, whereas it's really an expected and
> harmless situation.
>
> In order to eliminate this warning, this patch allows callers of the
> driver_deferred_probe_check_state() helper to specify that they want
> to continue deferring probe, regardless of whether we're past the
> init stage or not. All of the callers of that function are updated
> for the new signature, but only the pinctrl subsystem passes a true
> value in the new persist parameter if appropriate.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - pass persist flag via flags parameter to make the function call easier
>   to understand
>
>  drivers/base/dd.c            | 19 ++++++++++++++-----
>  drivers/base/power/domain.c  |  2 +-
>  drivers/iommu/of_iommu.c     |  2 +-
>  drivers/pinctrl/devicetree.c |  9 +++++----
>  include/linux/device.h       | 18 +++++++++++++++++-
>  5 files changed, 38 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 0df9b4461766..0399a6f6c479 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -238,23 +238,32 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
>  /**
>   * driver_deferred_probe_check_state() - Check deferred probe state
>   * @dev: device to check
> + * @flags: Flags used to control the behavior of this function. Drivers can
> + *   set the DRIVER_DEFER_PROBE_PERSIST flag to indicate that they want to

What about calling this flag DRIVER_DEFER_PROBE_CONTINUE ?

Also, I would just say

@flags: Flags used to control the behavior of this function.

here and added the description of the flag below.

> + *   keep trying to probe after built-in drivers have had a chance to probe.
> + *   This is useful for built-in drivers that rely on resources provided by
> + *   modular drivers.
>   *
>   * Returns -ENODEV if init is done and all built-in drivers have had a chance
> - * to probe (i.e. initcalls are done), -ETIMEDOUT if deferred probe debug
> - * timeout has expired, or -EPROBE_DEFER if none of those conditions are met.
> + * to probe (i.e. initcalls are done) and unless the DRIVER_DEFER_PROBE_PERSIST

"unless DRIVER_DEFER_PROBE_CONTINUE is set in @flags"

> + * flag is set, -ETIMEDOUT if deferred probe debug timeout has expired, or
> + * -EPROBE_DEFER if none of those conditions are met.
>   *
>   * Drivers or subsystems can opt-in to calling this function instead of directly
>   * returning -EPROBE_DEFER.

And here:

"In that case, passing DRIVER_DEFER_PROBE_CONTINUE in @flags indicates
that the caller wants to keep trying to probe after built-in drivers
have had a chance to probe, which is useful for built-in drivers that
rely on resources provided by modular drivers."

>   */
> -int driver_deferred_probe_check_state(struct device *dev)
> +int driver_deferred_probe_check_state(struct device *dev, unsigned long flags)
>  {
>         if (initcalls_done) {
>                 if (!deferred_probe_timeout) {
>                         dev_WARN(dev, "deferred probe timeout, ignoring dependency");
>                         return -ETIMEDOUT;
>                 }
> -               dev_warn(dev, "ignoring dependency for device, assuming no driver");
> -               return -ENODEV;
> +
> +               if ((flags & DRIVER_DEFER_PROBE_PERSIST) == 0) {

What about

if (!(flags & DRIVER_DEFER_PROBE_PERSIST)) {

> +                       dev_warn(dev, "ignoring dependency for device, assuming no driver");
> +                       return -ENODEV;
> +               }
>         }
>         return -EPROBE_DEFER;
>  }
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 33c30c1e6a30..6198c6a30fe2 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2423,7 +2423,7 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>                 mutex_unlock(&gpd_list_lock);
>                 dev_dbg(dev, "%s() failed to find PM domain: %ld\n",
>                         __func__, PTR_ERR(pd));
> -               return driver_deferred_probe_check_state(base_dev);
> +               return driver_deferred_probe_check_state(base_dev, 0);
>         }
>
>         dev_dbg(dev, "adding to PM domain %s\n", pd->name);
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 614a93aa5305..b95d4342e414 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -105,7 +105,7 @@ static int of_iommu_xlate(struct device *dev,
>          * a proper probe-ordering dependency mechanism in future.
>          */
>         if (!ops)
> -               return driver_deferred_probe_check_state(dev);
> +               return driver_deferred_probe_check_state(dev, 0);
>
>         return ops->of_xlate(dev, iommu_spec);
>  }
> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
> index f7e354f85518..43c0183fa23f 100644
> --- a/drivers/pinctrl/devicetree.c
> +++ b/drivers/pinctrl/devicetree.c
> @@ -111,13 +111,14 @@ static int dt_to_map_one_config(struct pinctrl *p,
>
>                 np_pctldev = of_get_next_parent(np_pctldev);
>                 if (!np_pctldev || of_node_is_root(np_pctldev)) {
> +                       unsigned long flags = 0;
> +
>                         of_node_put(np_pctldev);
> -                       ret = driver_deferred_probe_check_state(p->dev);
>                         /* keep deferring if modules are enabled unless we've timed out */
> -                       if (IS_ENABLED(CONFIG_MODULES) && !allow_default && ret == -ENODEV)
> -                               ret = -EPROBE_DEFER;
> +                       if (IS_ENABLED(CONFIG_MODULES) && !allow_default)
> +                               flags |= DRIVER_DEFER_PROBE_PERSIST;
>
> -                       return ret;
> +                       return driver_deferred_probe_check_state(p->dev, flags);
>                 }
>                 /* If we're creating a hog we can use the passed pctldev */
>                 if (hog_pctldev && (np_pctldev == p->dev->of_node)) {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index e0649f6adf2e..d364656a920c 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -340,7 +340,23 @@ struct device *driver_find_device(struct device_driver *drv,
>                                   int (*match)(struct device *dev, void *data));
>
>  void driver_deferred_probe_add(struct device *dev);
> -int driver_deferred_probe_check_state(struct device *dev);
> +
> +/*
> + * This can be use to continue to defer probe after the init stage and after
> + * all the built-in drivers have had a chance to probe. This is useful if a
> + * built-in driver requires resources provided by a modular driver.
> + *
> + * One such example is the pinctrl subsystem, where for example the DPAUX
> + * hardware on Tegra provides pinmuxing controls for pins shared between DPAUX
> + * and I2C controllers. Only a subset of I2C controllers need the DPAUX
> + * pinmuxing, and some I2C controllers are used during early boot for critical
> + * tasks (such as communicating with the system PMIC). The I2C controllers
> + * that don't share pins with a DPAUX block will want to be driven by a built-
> + * in driver to make sure they are available early on.
> + */
> +#define DRIVER_DEFER_PROBE_PERSIST (1 << 0)

Use BIT(1) ?

> +
> +int driver_deferred_probe_check_state(struct device *dev, unsigned long flags);
>
>  /**
>   * struct subsys_interface - interfaces to device functions
> --

Re: [PATCH v2] driver: core: Allow subsystems to continue deferring probe

[Greg Kroah-Hartman]

On Thu, Jun 13, 2019 at 07:00:11PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Some subsystems, such as pinctrl, allow continuing to defer probe
> indefinitely. This is useful for devices that depend on resources
> provided by devices that are only probed after the init stage.
> 
> One example of this can be seen on Tegra, where the DPAUX hardware
> contains pinmuxing controls for pins that it shares with an I2C
> controller. The I2C controller is typically used for communication
> with a monitor over HDMI (DDC). However, other instances of the I2C
> controller are used to access system critical components, such as a
> PMIC. The I2C controller driver will therefore usually be a builtin
> driver, whereas the DPAUX driver is part of the display driver that
> is loaded from a module to avoid bloating the kernel image with all
> of the DRM/KMS subsystem.
> 
> In this particular case the pins used by this I2C/DDC controller
> become accessible very late in the boot process. However, since the
> controller is only used in conjunction with display, that's not an
> issue.
> 
> Unfortunately the driver core currently outputs a warning message
> when a device fails to get the pinctrl before the end of the init
> stage. That can be confusing for the user because it may sound like
> an unwanted error occurred, whereas it's really an expected and
> harmless situation.
> 
> In order to eliminate this warning, this patch allows callers of the
> driver_deferred_probe_check_state() helper to specify that they want
> to continue deferring probe, regardless of whether we're past the
> init stage or not. All of the callers of that function are updated
> for the new signature, but only the pinctrl subsystem passes a true
> value in the new persist parameter if appropriate.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - pass persist flag via flags parameter to make the function call easier
>   to understand
> 
>  drivers/base/dd.c            | 19 ++++++++++++++-----
>  drivers/base/power/domain.c  |  2 +-
>  drivers/iommu/of_iommu.c     |  2 +-
>  drivers/pinctrl/devicetree.c |  9 +++++----
>  include/linux/device.h       | 18 +++++++++++++++++-
>  5 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 0df9b4461766..0399a6f6c479 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -238,23 +238,32 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
>  /**
>   * driver_deferred_probe_check_state() - Check deferred probe state
>   * @dev: device to check
> + * @flags: Flags used to control the behavior of this function. Drivers can
> + *   set the DRIVER_DEFER_PROBE_PERSIST flag to indicate that they want to
> + *   keep trying to probe after built-in drivers have had a chance to probe.
> + *   This is useful for built-in drivers that rely on resources provided by
> + *   modular drivers.
>   *
>   * Returns -ENODEV if init is done and all built-in drivers have had a chance
> - * to probe (i.e. initcalls are done), -ETIMEDOUT if deferred probe debug
> - * timeout has expired, or -EPROBE_DEFER if none of those conditions are met.
> + * to probe (i.e. initcalls are done) and unless the DRIVER_DEFER_PROBE_PERSIST
> + * flag is set, -ETIMEDOUT if deferred probe debug timeout has expired, or
> + * -EPROBE_DEFER if none of those conditions are met.
>   *
>   * Drivers or subsystems can opt-in to calling this function instead of directly
>   * returning -EPROBE_DEFER.
>   */
> -int driver_deferred_probe_check_state(struct device *dev)
> +int driver_deferred_probe_check_state(struct device *dev, unsigned long flags)
>  {
>  	if (initcalls_done) {
>  		if (!deferred_probe_timeout) {
>  			dev_WARN(dev, "deferred probe timeout, ignoring dependency");
>  			return -ETIMEDOUT;
>  		}
> -		dev_warn(dev, "ignoring dependency for device, assuming no driver");
> -		return -ENODEV;
> +
> +		if ((flags & DRIVER_DEFER_PROBE_PERSIST) == 0) {
> +			dev_warn(dev, "ignoring dependency for device, assuming no driver");
> +			return -ENODEV;
> +		}
>  	}
>  	return -EPROBE_DEFER;
>  }
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 33c30c1e6a30..6198c6a30fe2 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2423,7 +2423,7 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>  		mutex_unlock(&gpd_list_lock);
>  		dev_dbg(dev, "%s() failed to find PM domain: %ld\n",
>  			__func__, PTR_ERR(pd));
> -		return driver_deferred_probe_check_state(base_dev);
> +		return driver_deferred_probe_check_state(base_dev, 0);

Again, I said no odd flags for functions, how is anyone supposed to know
what "0" means here?

You just swapped a boolean for a bitmapped flag, right?  That did not
make the api any easier to understand at all.

> +/*
> + * This can be use to continue to defer probe after the init stage and after
> + * all the built-in drivers have had a chance to probe. This is useful if a
> + * built-in driver requires resources provided by a modular driver.
> + *
> + * One such example is the pinctrl subsystem, where for example the DPAUX
> + * hardware on Tegra provides pinmuxing controls for pins shared between DPAUX
> + * and I2C controllers. Only a subset of I2C controllers need the DPAUX
> + * pinmuxing, and some I2C controllers are used during early boot for critical
> + * tasks (such as communicating with the system PMIC). The I2C controllers
> + * that don't share pins with a DPAUX block will want to be driven by a built-
> + * in driver to make sure they are available early on.
> + */
> +#define DRIVER_DEFER_PROBE_PERSIST (1 << 0)

In the future, please always use BIT() for stuff like this.

Anyway, this isn't ok, do it correctly please, like I asked for the
first time...

thanks,

greg k-h

Re: [PATCH v2] driver: core: Allow subsystems to continue deferring probe

[Thierry Reding]

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

On Fri, Jun 14, 2019 at 11:10:58AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jun 13, 2019 at 07:00:11PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Some subsystems, such as pinctrl, allow continuing to defer probe
> > indefinitely. This is useful for devices that depend on resources
> > provided by devices that are only probed after the init stage.
> > 
> > One example of this can be seen on Tegra, where the DPAUX hardware
> > contains pinmuxing controls for pins that it shares with an I2C
> > controller. The I2C controller is typically used for communication
> > with a monitor over HDMI (DDC). However, other instances of the I2C
> > controller are used to access system critical components, such as a
> > PMIC. The I2C controller driver will therefore usually be a builtin
> > driver, whereas the DPAUX driver is part of the display driver that
> > is loaded from a module to avoid bloating the kernel image with all
> > of the DRM/KMS subsystem.
> > 
> > In this particular case the pins used by this I2C/DDC controller
> > become accessible very late in the boot process. However, since the
> > controller is only used in conjunction with display, that's not an
> > issue.
> > 
> > Unfortunately the driver core currently outputs a warning message
> > when a device fails to get the pinctrl before the end of the init
> > stage. That can be confusing for the user because it may sound like
> > an unwanted error occurred, whereas it's really an expected and
> > harmless situation.
> > 
> > In order to eliminate this warning, this patch allows callers of the
> > driver_deferred_probe_check_state() helper to specify that they want
> > to continue deferring probe, regardless of whether we're past the
> > init stage or not. All of the callers of that function are updated
> > for the new signature, but only the pinctrl subsystem passes a true
> > value in the new persist parameter if appropriate.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v2:
> > - pass persist flag via flags parameter to make the function call easier
> >   to understand
> > 
> >  drivers/base/dd.c            | 19 ++++++++++++++-----
> >  drivers/base/power/domain.c  |  2 +-
> >  drivers/iommu/of_iommu.c     |  2 +-
> >  drivers/pinctrl/devicetree.c |  9 +++++----
> >  include/linux/device.h       | 18 +++++++++++++++++-
> >  5 files changed, 38 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index 0df9b4461766..0399a6f6c479 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -238,23 +238,32 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
> >  /**
> >   * driver_deferred_probe_check_state() - Check deferred probe state
> >   * @dev: device to check
> > + * @flags: Flags used to control the behavior of this function. Drivers can
> > + *   set the DRIVER_DEFER_PROBE_PERSIST flag to indicate that they want to
> > + *   keep trying to probe after built-in drivers have had a chance to probe.
> > + *   This is useful for built-in drivers that rely on resources provided by
> > + *   modular drivers.
> >   *
> >   * Returns -ENODEV if init is done and all built-in drivers have had a chance
> > - * to probe (i.e. initcalls are done), -ETIMEDOUT if deferred probe debug
> > - * timeout has expired, or -EPROBE_DEFER if none of those conditions are met.
> > + * to probe (i.e. initcalls are done) and unless the DRIVER_DEFER_PROBE_PERSIST
> > + * flag is set, -ETIMEDOUT if deferred probe debug timeout has expired, or
> > + * -EPROBE_DEFER if none of those conditions are met.
> >   *
> >   * Drivers or subsystems can opt-in to calling this function instead of directly
> >   * returning -EPROBE_DEFER.
> >   */
> > -int driver_deferred_probe_check_state(struct device *dev)
> > +int driver_deferred_probe_check_state(struct device *dev, unsigned long flags)
> >  {
> >  	if (initcalls_done) {
> >  		if (!deferred_probe_timeout) {
> >  			dev_WARN(dev, "deferred probe timeout, ignoring dependency");
> >  			return -ETIMEDOUT;
> >  		}
> > -		dev_warn(dev, "ignoring dependency for device, assuming no driver");
> > -		return -ENODEV;
> > +
> > +		if ((flags & DRIVER_DEFER_PROBE_PERSIST) == 0) {
> > +			dev_warn(dev, "ignoring dependency for device, assuming no driver");
> > +			return -ENODEV;
> > +		}
> >  	}
> >  	return -EPROBE_DEFER;
> >  }
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 33c30c1e6a30..6198c6a30fe2 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -2423,7 +2423,7 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
> >  		mutex_unlock(&gpd_list_lock);
> >  		dev_dbg(dev, "%s() failed to find PM domain: %ld\n",
> >  			__func__, PTR_ERR(pd));
> > -		return driver_deferred_probe_check_state(base_dev);
> > +		return driver_deferred_probe_check_state(base_dev, 0);
> 
> Again, I said no odd flags for functions, how is anyone supposed to know
> what "0" means here?

Technically you said "no boolean flags". And I think it's pretty common
to have this type of flag bitmask to change the behavior of functions. 0
is typically considered as the "run normally" case where you don't
really have to care.

> You just swapped a boolean for a bitmapped flag, right?  That did not
> make the api any easier to understand at all.

Granted, the normal case isn't made any easier with "0" than with
"false", but certainly the cases where we do pass one or more flags are
now much easier to understand.

> > +/*
> > + * This can be use to continue to defer probe after the init stage and after
> > + * all the built-in drivers have had a chance to probe. This is useful if a
> > + * built-in driver requires resources provided by a modular driver.
> > + *
> > + * One such example is the pinctrl subsystem, where for example the DPAUX
> > + * hardware on Tegra provides pinmuxing controls for pins shared between DPAUX
> > + * and I2C controllers. Only a subset of I2C controllers need the DPAUX
> > + * pinmuxing, and some I2C controllers are used during early boot for critical
> > + * tasks (such as communicating with the system PMIC). The I2C controllers
> > + * that don't share pins with a DPAUX block will want to be driven by a built-
> > + * in driver to make sure they are available early on.
> > + */
> > +#define DRIVER_DEFER_PROBE_PERSIST (1 << 0)
> 
> In the future, please always use BIT() for stuff like this.
> 
> Anyway, this isn't ok, do it correctly please, like I asked for the
> first time...

To avoid further back and forth, what exactly is it that you would have
me do? That is, what do you consider to be the correct way to do this?

Would you prefer me to add another function with a different name that
reimplements the functionality only with the exception? Something along
the lines of:

	int driver_deferred_probe_check_state_continue(struct device *dev)
	{
		int ret;

		ret = driver_deferred_probe_check_state(dev);
		if (ret == -ENODEV)
			return -EPROBE_DEFER;

		return ret;
	}

? I'd need to split that up some more to avoid the warning that the
inner function prints before returning -ENODEV, but that's a minor
detail. Would that API be more to your liking?

Thierry

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

Re: [PATCH v2] driver: core: Allow subsystems to continue deferring probe

[Rafael J. Wysocki]

On Fri, Jun 14, 2019 at 11:39 AM Thierry Reding
<thierry.reding@gmail.com> wrote:
>
> On Fri, Jun 14, 2019 at 11:10:58AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jun 13, 2019 at 07:00:11PM +0200, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > >

[cut]

>
> To avoid further back and forth, what exactly is it that you would have
> me do? That is, what do you consider to be the correct way to do this?
>
> Would you prefer me to add another function with a different name that
> reimplements the functionality only with the exception? Something along
> the lines of:
>
>         int driver_deferred_probe_check_state_continue(struct device *dev)
>         {
>                 int ret;
>
>                 ret = driver_deferred_probe_check_state(dev);
>                 if (ret == -ENODEV)
>                         return -EPROBE_DEFER;
>
>                 return ret;
>         }
>
> ? I'd need to split that up some more to avoid the warning that the
> inner function prints before returning -ENODEV, but that's a minor
> detail. Would that API be more to your liking?

Well, why don't you do

static int deferred_probe_check_state_internal(struct device *dev)
{
        if (!initcalls_done)
                return -EPROBE_DEFER;

        if (!deferred_probe_timeout) {
                dev_WARN(dev, "deferred probe timeout, ignoring dependency");
                return -ETIMEDOUT;
        }

        return 0;
}

int driver_deferred_probe_check_state(struct device *dev)
{
        int ret = deferred_probe_check_state_internal(dev);

        if (ret)
                 return ret;

        dev_warn(dev, "ignoring dependency for device, assuming no driver");
        return -ENODEV;
}

int driver_deferred_probe_check_state_continue(struct device *dev)
{
        int ret = deferred_probe_check_state_internal(dev);

        if (ret)
                 return ret;

        return -EPROBE_DEFER;
}

Re: [PATCH v2] driver: core: Allow subsystems to continue deferring probe

[Greg Kroah-Hartman]

On Fri, Jun 14, 2019 at 12:10:10PM +0200, Rafael J. Wysocki wrote:
> On Fri, Jun 14, 2019 at 11:39 AM Thierry Reding
> <thierry.reding@gmail.com> wrote:
> >
> > On Fri, Jun 14, 2019 at 11:10:58AM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Jun 13, 2019 at 07:00:11PM +0200, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > >
> 
> [cut]
> 
> >
> > To avoid further back and forth, what exactly is it that you would have
> > me do? That is, what do you consider to be the correct way to do this?
> >
> > Would you prefer me to add another function with a different name that
> > reimplements the functionality only with the exception? Something along
> > the lines of:
> >
> >         int driver_deferred_probe_check_state_continue(struct device *dev)
> >         {
> >                 int ret;
> >
> >                 ret = driver_deferred_probe_check_state(dev);
> >                 if (ret == -ENODEV)
> >                         return -EPROBE_DEFER;
> >
> >                 return ret;
> >         }
> >
> > ? I'd need to split that up some more to avoid the warning that the
> > inner function prints before returning -ENODEV, but that's a minor
> > detail. Would that API be more to your liking?
> 
> Well, why don't you do
> 
> static int deferred_probe_check_state_internal(struct device *dev)
> {
>         if (!initcalls_done)
>                 return -EPROBE_DEFER;
> 
>         if (!deferred_probe_timeout) {
>                 dev_WARN(dev, "deferred probe timeout, ignoring dependency");
>                 return -ETIMEDOUT;
>         }
> 
>         return 0;
> }
> 
> int driver_deferred_probe_check_state(struct device *dev)
> {
>         int ret = deferred_probe_check_state_internal(dev);
> 
>         if (ret)
>                  return ret;
> 
>         dev_warn(dev, "ignoring dependency for device, assuming no driver");
>         return -ENODEV;
> }
> 
> int driver_deferred_probe_check_state_continue(struct device *dev)
> {
>         int ret = deferred_probe_check_state_internal(dev);
> 
>         if (ret)
>                  return ret;
> 
>         return -EPROBE_DEFER;
> }

Yes, that's much more sane.  Self-decribing apis are the key here, I did
not want a boolean flag, or any other flag, as part of the public api as
they do not describe what the call does at all.

thanks,

greg k-h