Re: [PATCH v2] driver: core: Allow subsystems to continue deferring probe
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 > 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 > > > > > > [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
Re: [PATCH v2] driver: core: Allow subsystems to continue deferring probe
On Fri, Jun 14, 2019 at 11:39 AM Thierry Reding 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 > > > [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
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 > > > > 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 > > --- > > 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(_list_lock); > > dev_dbg(dev, "%s() failed to find PM domain: %ld\n", > > __func__,
Re: [PATCH v2] driver: core: Allow subsystems to continue deferring probe
On Thu, Jun 13, 2019 at 07:00:11PM +0200, Thierry Reding wrote: > From: Thierry Reding > > 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 > --- > 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(_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
Re: [PATCH v2] driver: core: Allow subsystems to continue deferring probe
On Thu, Jun 13, 2019 at 7:00 PM Thierry Reding wrote: > > From: Thierry Reding > > 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 > --- > 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
Re: [PATCH v2] driver: core: Allow subsystems to continue deferring probe
On Thu, Jun 13, 2019 at 11:00 AM Thierry Reding wrote: > > From: Thierry Reding > > 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 > --- > 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