Re: [PATCH 2/8] driver-core: add asynchronous probing support for drivers

[Tomeu Vizoso]

On 31 March 2015 at 01:20, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:
> Some devices take a long time when initializing, and not all drivers are
> suited to initialize their devices when they are open. For example,
> input drivers need to interrogate their devices in order to publish
> device's capabilities before userspace will open them. When such drivers
> are compiled into kernel they may stall entire kernel initialization.
>
> This change allows drivers request for their probe functions to be
> called asynchronously during driver and device registration (manual
> binding is still synchronous). Because async_schedule is used to perform
> asynchronous calls module loading will still wait for the probing to
> complete.

But what about parents? Don't we need to make sure that before probing
a device its parent has finished probing already? This backtrace
illustrates the problem:

[<c0014818>] (__dabt_svc) from [<c03737ac>] (host1x_syncpt_alloc+0x14/0x134)
[<c03737ac>] (host1x_syncpt_alloc) from [<c03738f4>]
(host1x_syncpt_request+0x28/0x2c)
[<c03738f4>] (host1x_syncpt_request) from [<c03b55ec>]
(tegra_dc_probe+0x198/0x35c)
[<c03b55ec>] (tegra_dc_probe) from [<c03cb5a0>] (platform_drv_probe+0x54/0xbc)
[<c03cb5a0>] (platform_drv_probe) from [<c03c96e0>]
(driver_probe_device+0x184/0x2c4)
[<c03c96e0>] (driver_probe_device) from [<c03c98bc>] (__driver_attach+0x9c/0xa0)
[<c03c98bc>] (__driver_attach) from [<c03c78d8>] (bus_for_each_dev+0x78/0xac)
[<c03c78d8>] (bus_for_each_dev) from [<c03c9070>] (driver_attach+0x2c/0x30)
[<c03c9070>] (driver_attach) from [<c03c7e10>] (driver_attach_async+0x18/0x1c)
[<c03c7e10>] (driver_attach_async) from [<c004afd0>]
(async_run_entry_fn+0x58/0x128)
[<c004afd0>] (async_run_entry_fn) from [<c0042470>]
(process_one_work+0x140/0x50c)
[<c0042470>] (process_one_work) from [<c0042890>] (worker_thread+0x54/0x52c)
[<c0042890>] (worker_thread) from [<c0048554>] (kthread+0xec/0x104)
[<c0048554>] (kthread) from [<c000fc28>] (ret_from_fork+0x14/0x2c)

host1x_syncpt_request() assumes that the parent of the DC (host1x) has
been probed already and its drvdata is available.

Thanks,

Tomeu

> Note that the end goal is to make the probing asynchronous by default,
> so annotating drivers with PROBE_PREFER_ASYNCHRONOUS is a temporary
> measure that allows us to speed up boot process while we validating and
> fixing the rest of the drivers and preparing userspace.
>
> This change is based on earlier patch by "Luis R. Rodriguez"
> <mcgrof@xxxxxxxx>
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> ---
>  drivers/base/base.h    |   1 +
>  drivers/base/bus.c     |  31 +++++++---
>  drivers/base/dd.c      | 149 ++++++++++++++++++++++++++++++++++++++++++-------
>  include/linux/device.h |  28 ++++++++++
>  4 files changed, 182 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 251c5d3..fd3347d 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -116,6 +116,7 @@ static inline int driver_match_device(struct device_driver *drv,
>  {
>         return drv->bus->match ? drv->bus->match(dev, drv) : 1;
>  }
> +extern bool driver_allows_async_probing(struct device_driver *drv);
>
>  extern int driver_add_groups(struct device_driver *drv,
>                              const struct attribute_group **groups);
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 79bc203..5005924 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -10,6 +10,7 @@
>   *
>   */
>
> +#include <linux/async.h>
>  #include <linux/device.h>
>  #include <linux/module.h>
>  #include <linux/errno.h>
> @@ -549,15 +550,12 @@ void bus_probe_device(struct device *dev)
>  {
>         struct bus_type *bus = dev->bus;
>         struct subsys_interface *sif;
> -       int ret;
>
>         if (!bus)
>                 return;
>
> -       if (bus->p->drivers_autoprobe) {
> -               ret = device_attach(dev);
> -               WARN_ON(ret < 0);
> -       }
> +       if (bus->p->drivers_autoprobe)
> +               device_initial_probe(dev);
>
>         mutex_lock(&bus->p->mutex);
>         list_for_each_entry(sif, &bus->p->interfaces, node)
> @@ -659,6 +657,17 @@ static ssize_t uevent_store(struct device_driver *drv, const char *buf,
>  }
>  static DRIVER_ATTR_WO(uevent);
>
> +static void driver_attach_async(void *_drv, async_cookie_t cookie)
> +{
> +       struct device_driver *drv = _drv;
> +       int ret;
> +
> +       ret = driver_attach(drv);
> +
> +       pr_debug("bus: '%s': driver %s async attach completed: %d\n",
> +                drv->bus->name, drv->name, ret);
> +}
> +
>  /**
>   * bus_add_driver - Add a driver to the bus.
>   * @drv: driver.
> @@ -691,9 +700,15 @@ int bus_add_driver(struct device_driver *drv)
>
>         klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers);
>         if (drv->bus->p->drivers_autoprobe) {
> -               error = driver_attach(drv);
> -               if (error)
> -                       goto out_unregister;
> +               if (driver_allows_async_probing(drv)) {
> +                       pr_debug("bus: '%s': probing driver %s asynchronously\n",
> +                               drv->bus->name, drv->name);
> +                       async_schedule(driver_attach_async, drv);
> +               } else {
> +                       error = driver_attach(drv);
> +                       if (error)
> +                               goto out_unregister;
> +               }
>         }
>         module_add_driver(drv->owner, drv);
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index e843fdb..2ad33b2 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -417,31 +417,95 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
>         return ret;
>  }
>
> -static int __device_attach(struct device_driver *drv, void *data)
> +bool driver_allows_async_probing(struct device_driver *drv)
>  {
> -       struct device *dev = data;
> +       return drv->probe_type == PROBE_PREFER_ASYNCHRONOUS;
> +}
> +
> +struct device_attach_data {
> +       struct device *dev;
> +
> +       /*
> +        * Indicates whether we are are considering asynchronous probing or
> +        * not. Only initial binding after device or driver registration
> +        * (including deferral processing) may be done asynchronously, the
> +        * rest is always synchronous, as we expect it is being done by
> +        * request from userspace.
> +        */
> +       bool check_async;
> +
> +       /*
> +        * Indicates if we are binding synchronous or asynchronous drivers.
> +        * When asynchronous probing is enabled we'll execute 2 passes
> +        * over drivers: first pass doing synchronous probing and second
> +        * doing asynchronous probing (if synchronous did not succeed -
> +        * most likely because there was no driver requiring synchronous
> +        * probing - and we found asynchronous driver during first pass).
> +        * The 2 passes are done because we can't shoot asynchronous
> +        * probe for given device and driver from bus_for_each_drv() since
> +        * driver pointer is not guaranteed to stay valid once
> +        * bus_for_each_drv() iterates to the next driver on the bus.
> +        */
> +       bool want_async;
> +
> +       /*
> +        * We'll set have_async to 'true' if, while scanning for matching
> +        * driver, we'll encounter one that requests asynchronous probing.
> +        */
> +       bool have_async;
> +};
> +
> +static int __device_attach_driver(struct device_driver *drv, void *_data)
> +{
> +       struct device_attach_data *data = _data;
> +       struct device *dev = data->dev;
> +       bool async_allowed;
> +
> +       /*
> +        * Check if device has already been claimed. This may
> +        * happen with driver loading, device discovery/registration,
> +        * and deferred probe processing happens all at once with
> +        * multiple threads.
> +        */
> +       if (dev->driver)
> +               return -EBUSY;
>
>         if (!driver_match_device(drv, dev))
>                 return 0;
>
> +       async_allowed = driver_allows_async_probing(drv);
> +
> +       if (async_allowed)
> +               data->have_async = true;
> +
> +       if (data->check_async && async_allowed != data->want_async)
> +               return 0;
> +
>         return driver_probe_device(drv, dev);
>  }
>
> -/**
> - * device_attach - try to attach device to a driver.
> - * @dev: device.
> - *
> - * Walk the list of drivers that the bus has and call
> - * driver_probe_device() for each pair. If a compatible
> - * pair is found, break out and return.
> - *
> - * Returns 1 if the device was bound to a driver;
> - * 0 if no matching driver was found;
> - * -ENODEV if the device is not registered.
> - *
> - * When called for a USB interface, @dev->parent lock must be held.
> - */
> -int device_attach(struct device *dev)
> +static void __device_attach_async_helper(void *_dev, async_cookie_t cookie)
> +{
> +       struct device *dev = _dev;
> +       struct device_attach_data data = {
> +               .dev            = dev,
> +               .check_async    = true,
> +               .want_async     = true,
> +       };
> +
> +       device_lock(dev);
> +
> +       bus_for_each_drv(dev->bus, NULL, &data, __device_attach_driver);
> +       dev_dbg(dev, "async probe completed\n");
> +
> +       pm_request_idle(dev);
> +
> +       device_unlock(dev);
> +
> +       put_device(dev);
> +}
> +
> +int __device_attach(struct device *dev, bool allow_async)
>  {
>         int ret = 0;
>
> @@ -459,15 +523,59 @@ int device_attach(struct device *dev)
>                         ret = 0;
>                 }
>         } else {
> -               ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach);
> -               pm_request_idle(dev);
> +               struct device_attach_data data = {
> +                       .dev = dev,
> +                       .check_async = allow_async,
> +                       .want_async = false,
> +               };
> +
> +               ret = bus_for_each_drv(dev->bus, NULL, &data,
> +                                       __device_attach_driver);
> +               if (!ret && allow_async && data.have_async) {
> +                       /*
> +                        * If we could not find appropriate driver
> +                        * synchronously and we are allowed to do
> +                        * async probes and there are drivers that
> +                        * want to probe asynchronously, we'll
> +                        * try them.
> +                        */
> +                       dev_dbg(dev, "scheduling asynchronous probe\n");
> +                       get_device(dev);
> +                       async_schedule(__device_attach_async_helper, dev);
> +               } else {
> +                       pm_request_idle(dev);
> +               }
>         }
>  out_unlock:
>         device_unlock(dev);
>         return ret;
>  }
> +
> +/**
> + * device_attach - try to attach device to a driver.
> + * @dev: device.
> + *
> + * Walk the list of drivers that the bus has and call
> + * driver_probe_device() for each pair. If a compatible
> + * pair is found, break out and return.
> + *
> + * Returns 1 if the device was bound to a driver;
> + * 0 if no matching driver was found;
> + * -ENODEV if the device is not registered.
> + *
> + * When called for a USB interface, @dev->parent lock must be held.
> + */
> +int device_attach(struct device *dev)
> +{
> +       return __device_attach(dev, false);
> +}
>  EXPORT_SYMBOL_GPL(device_attach);
>
> +void device_initial_probe(struct device *dev)
> +{
> +       __device_attach(dev, true);
> +}
> +
>  static int __driver_attach(struct device *dev, void *data)
>  {
>         struct device_driver *drv = data;
> @@ -522,6 +630,9 @@ static void __device_release_driver(struct device *dev)
>
>         drv = dev->driver;
>         if (drv) {
> +               if (driver_allows_async_probing(drv))
> +                       async_synchronize_full();
> +
>                 pm_runtime_get_sync(dev);
>
>                 driver_sysfs_remove(dev);
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 884aa6e..400cacd 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -196,12 +196,38 @@ extern struct kset *bus_get_kset(struct bus_type *bus);
>  extern struct klist *bus_get_device_klist(struct bus_type *bus);
>
>  /**
> + * enum probe_type - device driver probe type to try
> + *     Device drivers may opt in for special handling of their
> + *     respective probe routines. This tells the core what to
> + *     expect and prefer.
> + *
> + * @PROBE_SYNCHRONOUS: Default. Drivers expect their probe routines
> + *     to run synchronously with driver and device registration
> + *     (with the exception of -EPROBE_DEFER handling - re-probing
> + *     always ends up being done asynchronously).
> + * @PROBE_PREFER_ASYNCHRONOUS: Drivers for "slow" devices which
> + *     probing order is not essential for booting the system may
> + *     opt into executing their probes asynchronously.
> + *
> + * Note that the end goal is to switch the kernel to use asynchronous
> + * probing by default, so annotating drivers with
> + * %PROBE_PREFER_ASYNCHRONOUS is a temporary measure that allows us
> + * to speed up boot process while we are validating the rest of the
> + * drivers.
> + */
> +enum probe_type {
> +       PROBE_SYNCHRONOUS,
> +       PROBE_PREFER_ASYNCHRONOUS,
> +};
> +
> +/**
>   * struct device_driver - The basic device driver structure
>   * @name:      Name of the device driver.
>   * @bus:       The bus which the device of this driver belongs to.
>   * @owner:     The module owner.
>   * @mod_name:  Used for built-in modules.
>   * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> + * @probe_type:        Type of the probe (synchronous or asynchronous) to use.
>   * @of_match_table: The open firmware table.
>   * @acpi_match_table: The ACPI match table.
>   * @probe:     Called to query the existence of a specific device,
> @@ -235,6 +261,7 @@ struct device_driver {
>         const char              *mod_name;      /* used for built-in modules */
>
>         bool suppress_bind_attrs;       /* disables bind/unbind via sysfs */
> +       enum probe_type probe_type;
>
>         const struct of_device_id       *of_match_table;
>         const struct acpi_device_id     *acpi_match_table;
> @@ -972,6 +999,7 @@ extern int __must_check device_bind_driver(struct device *dev);
>  extern void device_release_driver(struct device *dev);
>  extern int  __must_check device_attach(struct device *dev);
>  extern int __must_check driver_attach(struct device_driver *drv);
> +extern void device_initial_probe(struct device *dev);
>  extern int __must_check device_reprobe(struct device *dev);
>
>  /*
> --
> 2.2.0.rc0.207.ga3a616c
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/