Message-Id: <201203051915.45197.arnd@arndb.de>
Date:	Mon, 5 Mar 2012 19:15:44 +0000
From:	Arnd Bergmann <arnd@...db.de>
To:	Grant Likely <grant.likely@...retlab.ca>
Cc:	linux-kernel@...r.kernel.org, Tony Lindgren <tony@...mide.com>,
	"Greg Kroah-Hartman" <greg@...ah.com>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Dilan Lee <dilee@...dia.com>,
	Manjunath GKondaiah <manjunath.gkondaiah@...aro.org>,
	Alan Stern <stern@...land.harvard.edu>
Subject: Re: [PATCH] drivercore: Add driver probe deferral mechanism

On Monday 05 March 2012, Grant Likely wrote:
> Allow drivers to report at probe time that they cannot get all the resources
> required by the device, and should be retried at a later time.
> 
> This should completely solve the problem of getting devices
> initialized in the right order.  Right now this is mostly handled by
> mucking about with initcall ordering which is a complete hack, and
> doesn't even remotely handle the case where device drivers are in
> modules.  This approach completely sidesteps the issues by allowing
> driver registration to occur in any order, and any driver can request
> to be retried after a few more other drivers get probed.

Hi Grant,

Looks great! I thought I had found two bugs but it turned out to
all be correct on second look.  What remains in my review is basically
bike-shedding, but I'll send it anyway since I took the time to
write it before I noticed I was wrong on the other points ;-)

Anyway, I'm happy for this to go in in the current way,

Reviewed-by: Arnd Bergmann <arnd@...db.de>

> @@ -28,6 +28,133 @@
>  #include "base.h"
>  #include "power/power.h"
>  
> +/*
> + * Deferred Probe infrastructure.
> + *
> + * Sometimes driver probe order matters, but the kernel doesn't always have
> + * dependency information which means some drivers will get probed before a
> + * resource it depends on is available.  For example, an SDHCI driver may
> + * first need a GPIO line from an i2c GPIO controller before it can be
> + * initialized.  If a required resource is not available yet, a driver can
> + * request probing to be deferred by returning -EPROBE_DEFER from its probe hook
> + *
> + * Deferred probe maintains two lists of devices, a pending list and an active
> + * list.  A driver returning -EPROBE_DEFER causes the device to be added to the
> + * pending list.  A successful driver probe will trigger moving all devices
> + * from the pending to the active list so that the workqueue will eventually
> + * retry them.
> + *
> + * The deferred_probe_mutex must be held any time the deferred_probe_*_list
> + * of the (struct device*)->deferred_probe pointers are manipulated
> + */
> +static DEFINE_MUTEX(deferred_probe_mutex);
> +static LIST_HEAD(deferred_probe_pending_list);
> +static LIST_HEAD(deferred_probe_active_list);
> +static struct workqueue_struct *deferred_wq;

I don't understand why you want both lists to be global, it seems to
complicate things.

> +/**
> + * deferred_probe_work_func() - Retry probing devices in the active list.
> + */
> +static void deferred_probe_work_func(struct work_struct *work)
> +{
> +	struct device *dev;
> +	/*
> +	 * This block processes every device in the deferred 'active' list.
> +	 * Each device is removed from the active list and passed to
> +	 * bus_probe_device() to re-attempt the probe.  The loop continues
> +	 * until every device in the active list is removed and retried.
> +	 *
> +	 * Note: Once the device is removed from the list and the mutex is
> +	 * released, it is possible for the device get freed by another thread
> +	 * and cause a illegal pointer dereference.  This code uses
> +	 * get/put_device() to ensure the device structure cannot disappear
> +	 * from under our feet.
> +	 */
> +	mutex_lock(&deferred_probe_mutex);
> +	while (!list_empty(&deferred_probe_active_list)) {
> +		dev = list_first_entry(&deferred_probe_active_list,
> +					typeof(*dev), deferred_probe);
> +		list_del_init(&dev->deferred_probe);
> +
> +		get_device(dev);
> +
> +		/* Drop the mutex while probing each device; the probe path
> +		 * may manipulate the deferred list */
> +		mutex_unlock(&deferred_probe_mutex);
> +		dev_dbg(dev, "Retrying from deferred list\n");
> +		bus_probe_device(dev);
> +		mutex_lock(&deferred_probe_mutex);
> +
> +		put_device(dev);
> +	}
> +	mutex_unlock(&deferred_probe_mutex);


If you make the deferred_probe_active_list local to this function, and do
the splice inside of it, you only need to hold the mutex for the
splice, and the loop can become a simpler

	LIST_HEAD(list);

	mutex_lock(&deferred_probe_mutex);
	list_splice_tail_init(&deferred_probe_pending_list, &list);
	mutex_unlock(&deferred_probe_mutex);

	list_for_each_entry_safe(...) {
		list_del_init(&dev->deferred_probe);
		bus_probe_device(dev);
		put_device(dev);
	}

Also, What protects the device from going away between being put on the list
and taken off of it? Don't you have to do the device_get during
driver_deferred_probe_add()?

> +static bool driver_deferred_probe_enable = false;
> +/**
> + * driver_deferred_probe_trigger() - Kick off re-probing deferred devices
> + *
> + * This functions moves all devices from the pending list to the active
> + * list and schedules the deferred probe workqueue to process them.  It
> + * should be called anytime a driver is successfully bound to a device.
> + */
> +static void driver_deferred_probe_trigger(void)
> +{
> +	if (!driver_deferred_probe_enable)
> +		return;

I tried to understand whether you need to have locking around
driver_deferred_probe_enable, but I think you don't even need this
variable at all:

> +
> +	/* A successful probe means that all the devices in the pending list
> +	 * should be triggered to be reprobed.  Move all the deferred devices
> +	 * into the active list so they can be retried by the workqueue */
> +	mutex_lock(&deferred_probe_mutex);
> +	list_splice_tail_init(&deferred_probe_pending_list,
> +			      &deferred_probe_active_list);
> +	mutex_unlock(&deferred_probe_mutex);
> +
> +	/* Kick the re-probe thread.  It may already be scheduled, but
> +	 * it is safe to kick it again. */
> +	queue_work(deferred_wq, &deferred_probe_work);
> +}

You can simply check whether deferred_wq is non-NULL here before you call it,
because it never goes away after it has been created.

> +static int deferred_probe_initcall(void)
> +{
> +	deferred_wq = create_singlethread_workqueue("deferwq");
> +	if (WARN_ON(!deferred_wq))
> +		return -ENOMEM;

I think "deferwq" is not a good name for a global thread: all work queues are
there for deferring somehting. How about "deferredprobe"?

	Arnd
--
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