| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAAObsKDpXs_feWTCZAMEJe+-FEmepPofjSKcWzexY6sywCxw6g@mail.gmail.com>
Date: Fri, 29 May 2015 12:48:37 +0200
From: Tomeu Vizoso <tomeu.vizoso@...il.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Luis R . Rodriguez" <mcgrof@...e.com>, Tejun Heo <tj@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Arjan van de Ven <arjan@...ux.intel.com>,
Rusty Russell <rusty@...tcorp.com.au>,
Olof Johansson <olof@...om.net>,
Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Subject: Re: [PATCH 2/8] driver-core: add asynchronous probing support for drivers
On 31 March 2015 at 01:20, Dmitry Torokhov <dmitry.torokhov@...il.com> 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@...e.com>
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> ---
> 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@...r.kernel.org
> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists