Re: [RFC v1 0/3] driver-core: add asynch module loading support

From: David Herrmann
Date: Sun Aug 31 2014 - 07:25:10 EST


Hi

On Sun, Aug 31, 2014 at 1:02 PM, Tejun Heo <tj@xxxxxxxxxx> wrote:
> So, something like the following. A couple things to note
>
> * driver_attach() can never fail but is marked with __must_check. We
> prolly should change it to void.
>
> * Old/weird userspace which depends on insmod to wait for device
> probing might choke and the new behavior might need to be switched
> somehow (sysctl, insmod param or whatever).
>
> Lightly tested and seems to work fine.
>
> Thanks.
> ---
> drivers/base/bus.c | 39 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 83e910a..e4fc9bc 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -23,6 +23,7 @@
>
> /* /sys/devices/system */
> static struct kset *system_kset;
> +static struct workqueue_struct *driver_attach_wq;
>
> #define to_bus_attr(_attr) container_of(_attr, struct bus_attribute, attr)
>
> @@ -657,6 +658,20 @@ static ssize_t uevent_store(struct device_driver *drv, const char *buf,
> }
> static DRIVER_ATTR_WO(uevent);
>
> +struct driver_attach_work {
> + struct work_struct work;
> + struct device_driver *driver;
> +};
> +
> +static void driver_attach_workfn(struct work_struct *work)
> +{
> + struct driver_attach_work *daw =
> + container_of(work, struct driver_attach_work, work);
> +
> + driver_attach(daw->driver);
> + kfree(daw);
> +}
> +
> /**
> * bus_add_driver - Add a driver to the bus.
> * @drv: driver.
> @@ -689,9 +704,23 @@ 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;
> + struct driver_attach_work *daw;
> +
> + if (drv->owner) {
> + daw = kzalloc(sizeof(*daw), GFP_KERNEL);
> + if (!daw) {
> + error = -ENOMEM;
> + goto out_unregister;
> + }
> +
> + INIT_WORK(&daw->work, driver_attach_workfn);
> + daw->driver = drv;
> + queue_work(driver_attach_wq, &daw->work);

Doesn't this break on-demand cdev initialization? We currently call
request_module() on open() for unclaimed major/minor combinations. If
driver_attach() is no longer part of module_init(), there is no
guarantee the driver created the cdev before request_module() returns.

I actually like this "deferred attach" approach, so this is not meant
as counter-argument. We just need to make sure to have a notion of
"settled modules" so we know how long to wait after loading a module.

Thanks
David

> + } else {
> + error = driver_attach(drv);
> + if (error)
> + goto out_unregister;
> + }
> }
> module_add_driver(drv->owner, drv);
>
> @@ -1268,5 +1297,9 @@ int __init buses_init(void)
> if (!system_kset)
> return -ENOMEM;
>
> + driver_attach_wq = alloc_ordered_workqueue("driver_attach", 0);
> + if (!driver_attach_wq)
> + return -ENOMEM;
> +
> return 0;
> }
> --
> 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/