[Patch 3/3] Driver core: Per-subsystem multithreaded probing.

[Patch 3/3] Driver core: Per-subsystem multithreaded probing.

[Cornelia Huck]

From: Cornelia Huck <cornelia.huck@de.ibm.com>

Make multithreaded probing work per subsystem instead of per driver.

It doesn't make much sense to probe the same device for multiple drivers in
parallel (after all, only one driver can bind to the device). Instead, create
a probing thread for each device that probes the drivers one after another.
Also make the decision to use multi-threaded probe per bus instead of per
device and adapt the pci code.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

---
 drivers/base/dd.c        |   54 +++++++++++++++++++++++------------------------
 drivers/pci/pci-driver.c |    6 -----
 include/linux/device.h   |    4 +--
 include/linux/pci.h      |    2 -
 4 files changed, 30 insertions(+), 36 deletions(-)

--- linux-2.6.orig/drivers/base/dd.c
+++ linux-2.6/drivers/base/dd.c
@@ -88,17 +88,9 @@ int device_bind_driver(struct device *de
 	return ret;
 }
 
-struct stupid_thread_structure {
-	struct device_driver *drv;
-	struct device *dev;
-};
-
 static atomic_t probe_count = ATOMIC_INIT(0);
-static int really_probe(void *void_data)
+static int really_probe(struct device *dev, struct device_driver *drv)
 {
-	struct stupid_thread_structure *data = void_data;
-	struct device_driver *drv = data->drv;
-	struct device *dev = data->dev;
 	int ret = 0;
 
 	atomic_inc(&probe_count);
@@ -138,7 +130,6 @@ probe_failed:
 	 */
 	ret = 0;
 done:
-	kfree(data);
 	atomic_dec(&probe_count);
 	return ret;
 }
@@ -177,8 +168,6 @@ int driver_probe_done(void)
  */
 int driver_probe_device(struct device_driver * drv, struct device * dev)
 {
-	struct stupid_thread_structure *data;
-	struct task_struct *probe_task;
 	int ret = 0;
 
 	if (!device_is_registered(dev))
@@ -189,19 +178,7 @@ int driver_probe_device(struct device_dr
 	pr_debug("%s: Matched Device %s with Driver %s\n",
 		 drv->bus->name, dev->bus_id, drv->name);
 
-	data = kmalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-	data->drv = drv;
-	data->dev = dev;
-
-	if (drv->multithread_probe) {
-		probe_task = kthread_run(really_probe, data,
-					 "probe-%s", dev->bus_id);
-		if (IS_ERR(probe_task))
-			ret = really_probe(data);
-	} else
-		ret = really_probe(data);
+	ret = really_probe(dev, drv);
 
 done:
 	return ret;
@@ -213,6 +190,19 @@ static int __device_attach(struct device
 	return driver_probe_device(drv, dev);
 }
 
+static int device_probe_drivers(void *data)
+{
+	struct device *dev = data;
+	int ret = 0;
+
+	if (dev->bus) {
+		down(&dev->sem);
+		ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach);
+		up(&dev->sem);
+	}
+	return ret;
+}
+
 /**
  *	device_attach - try to attach device to a driver.
  *	@dev:	device.
@@ -229,14 +219,24 @@ static int __device_attach(struct device
 int device_attach(struct device * dev)
 {
 	int ret = 0;
+	struct task_struct *probe_task;
 
 	down(&dev->sem);
 	if (dev->driver) {
 		ret = device_bind_driver(dev);
 		if (ret == 0)
 			ret = 1;
-	} else
-		ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach);
+	} else {
+		if (dev->bus->multithread_probe) {
+			probe_task = kthread_run(device_probe_drivers, dev,
+						 "probe-%s", dev->bus_id);
+			if(IS_ERR(probe_task))
+				ret = bus_for_each_drv(dev->bus, NULL, dev,
+						       __device_attach);
+		} else
+			ret = bus_for_each_drv(dev->bus, NULL, dev,
+					       __device_attach);
+	}
 	up(&dev->sem);
 	return ret;
 }
--- linux-2.6.orig/drivers/pci/pci-driver.c
+++ linux-2.6/drivers/pci/pci-driver.c
@@ -422,11 +422,6 @@ int __pci_register_driver(struct pci_dri
 	drv->driver.owner = owner;
 	drv->driver.kobj.ktype = &pci_driver_kobj_type;
 
-	if (pci_multithread_probe)
-		drv->driver.multithread_probe = pci_multithread_probe;
-	else
-		drv->driver.multithread_probe = drv->multithread_probe;
-
 	spin_lock_init(&drv->dynids.lock);
 	INIT_LIST_HEAD(&drv->dynids.list);
 
@@ -559,6 +554,7 @@ struct bus_type pci_bus_type = {
 
 static int __init pci_driver_init(void)
 {
+	pci_bus_type.multithread_probe = pci_multithread_probe;
 	return bus_register(&pci_bus_type);
 }
 
--- linux-2.6.orig/include/linux/device.h
+++ linux-2.6/include/linux/device.h
@@ -57,6 +57,8 @@ struct bus_type {
 	int (*suspend_late)(struct device * dev, pm_message_t state);
 	int (*resume_early)(struct device * dev);
 	int (*resume)(struct device * dev);
+
+	unsigned int multithread_probe:1;
 };
 
 extern int __must_check bus_register(struct bus_type * bus);
@@ -106,8 +108,6 @@ struct device_driver {
 	void	(*shutdown)	(struct device * dev);
 	int	(*suspend)	(struct device * dev, pm_message_t state);
 	int	(*resume)	(struct device * dev);
-
-	unsigned int multithread_probe:1;
 };
 
 
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -356,8 +356,6 @@ struct pci_driver {
 	struct pci_error_handlers *err_handler;
 	struct device_driver	driver;
 	struct pci_dynids dynids;
-
-	int multithread_probe;
 };
 
 #define	to_pci_driver(drv) container_of(drv,struct pci_driver, driver)

Re: [Patch 3/3] Driver core: Per-subsystem multithreaded probing.

[Duncan Sands]

On Monday 16 October 2006 10:44, Cornelia Huck wrote:
> From: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> Make multithreaded probing work per subsystem instead of per driver.
> 
> It doesn't make much sense to probe the same device for multiple drivers in
> parallel (after all, only one driver can bind to the device). Instead, create
> a probing thread for each device that probes the drivers one after another.
> Also make the decision to use multi-threaded probe per bus instead of per
> device and adapt the pci code.

I see that you also fix another problem with the previous code: probe would
be called with dev->sem taken in the single-threaded case, but not necessarily
in the multi-threaded case (because the kthread might run after the original
thread dropped the semaphore).  There may have been a similar problem with
USB locking, since there too probe was expecting a lock to be held that might
not be held when called from the kthread:

	 * This function must be called with @dev->sem held.  When called for a
	 * USB interface, @dev->parent->sem must be held as well.
	 */
	int driver_probe_device(struct device_driver * drv, struct device * dev)

I guess the code should be reviewed to check that all such problems have now
been eliminated.

Also, what about device removal racing with probe?  Is it possible for someone to
attempt to remove a device in the gap between the call to device_attach and the
kthread actually running and doing the probe?  That would result in remove and
probe being called in the wrong order...

Best wishes,

Duncan.

> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> ---
>  drivers/base/dd.c        |   54 +++++++++++++++++++++++------------------------
>  drivers/pci/pci-driver.c |    6 -----
>  include/linux/device.h   |    4 +--
>  include/linux/pci.h      |    2 -
>  4 files changed, 30 insertions(+), 36 deletions(-)
> 
> --- linux-2.6.orig/drivers/base/dd.c
> +++ linux-2.6/drivers/base/dd.c
> @@ -88,17 +88,9 @@ int device_bind_driver(struct device *de
>  	return ret;
>  }
>  
> -struct stupid_thread_structure {
> -	struct device_driver *drv;
> -	struct device *dev;
> -};
> -
>  static atomic_t probe_count = ATOMIC_INIT(0);
> -static int really_probe(void *void_data)
> +static int really_probe(struct device *dev, struct device_driver *drv)
>  {
> -	struct stupid_thread_structure *data = void_data;
> -	struct device_driver *drv = data->drv;
> -	struct device *dev = data->dev;
>  	int ret = 0;
>  
>  	atomic_inc(&probe_count);
> @@ -138,7 +130,6 @@ probe_failed:
>  	 */
>  	ret = 0;
>  done:
> -	kfree(data);
>  	atomic_dec(&probe_count);
>  	return ret;
>  }
> @@ -177,8 +168,6 @@ int driver_probe_done(void)
>   */
>  int driver_probe_device(struct device_driver * drv, struct device * dev)
>  {
> -	struct stupid_thread_structure *data;
> -	struct task_struct *probe_task;
>  	int ret = 0;
>  
>  	if (!device_is_registered(dev))
> @@ -189,19 +178,7 @@ int driver_probe_device(struct device_dr
>  	pr_debug("%s: Matched Device %s with Driver %s\n",
>  		 drv->bus->name, dev->bus_id, drv->name);
>  
> -	data = kmalloc(sizeof(*data), GFP_KERNEL);
> -	if (!data)
> -		return -ENOMEM;
> -	data->drv = drv;
> -	data->dev = dev;
> -
> -	if (drv->multithread_probe) {
> -		probe_task = kthread_run(really_probe, data,
> -					 "probe-%s", dev->bus_id);
> -		if (IS_ERR(probe_task))
> -			ret = really_probe(data);
> -	} else
> -		ret = really_probe(data);
> +	ret = really_probe(dev, drv);
>  
>  done:
>  	return ret;
> @@ -213,6 +190,19 @@ static int __device_attach(struct device
>  	return driver_probe_device(drv, dev);
>  }
>  
> +static int device_probe_drivers(void *data)
> +{
> +	struct device *dev = data;
> +	int ret = 0;
> +
> +	if (dev->bus) {
> +		down(&dev->sem);
> +		ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach);
> +		up(&dev->sem);
> +	}
> +	return ret;
> +}
> +
>  /**
>   *	device_attach - try to attach device to a driver.
>   *	@dev:	device.
> @@ -229,14 +219,24 @@ static int __device_attach(struct device
>  int device_attach(struct device * dev)
>  {
>  	int ret = 0;
> +	struct task_struct *probe_task;
>  
>  	down(&dev->sem);
>  	if (dev->driver) {
>  		ret = device_bind_driver(dev);
>  		if (ret == 0)
>  			ret = 1;
> -	} else
> -		ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach);
> +	} else {
> +		if (dev->bus->multithread_probe) {
> +			probe_task = kthread_run(device_probe_drivers, dev,
> +						 "probe-%s", dev->bus_id);
> +			if(IS_ERR(probe_task))
> +				ret = bus_for_each_drv(dev->bus, NULL, dev,
> +						       __device_attach);
> +		} else
> +			ret = bus_for_each_drv(dev->bus, NULL, dev,
> +					       __device_attach);
> +	}
>  	up(&dev->sem);
>  	return ret;
>  }
> --- linux-2.6.orig/drivers/pci/pci-driver.c
> +++ linux-2.6/drivers/pci/pci-driver.c
> @@ -422,11 +422,6 @@ int __pci_register_driver(struct pci_dri
>  	drv->driver.owner = owner;
>  	drv->driver.kobj.ktype = &pci_driver_kobj_type;
>  
> -	if (pci_multithread_probe)
> -		drv->driver.multithread_probe = pci_multithread_probe;
> -	else
> -		drv->driver.multithread_probe = drv->multithread_probe;
> -
>  	spin_lock_init(&drv->dynids.lock);
>  	INIT_LIST_HEAD(&drv->dynids.list);
>  
> @@ -559,6 +554,7 @@ struct bus_type pci_bus_type = {
>  
>  static int __init pci_driver_init(void)
>  {
> +	pci_bus_type.multithread_probe = pci_multithread_probe;
>  	return bus_register(&pci_bus_type);
>  }
>  
> --- linux-2.6.orig/include/linux/device.h
> +++ linux-2.6/include/linux/device.h
> @@ -57,6 +57,8 @@ struct bus_type {
>  	int (*suspend_late)(struct device * dev, pm_message_t state);
>  	int (*resume_early)(struct device * dev);
>  	int (*resume)(struct device * dev);
> +
> +	unsigned int multithread_probe:1;
>  };
>  
>  extern int __must_check bus_register(struct bus_type * bus);
> @@ -106,8 +108,6 @@ struct device_driver {
>  	void	(*shutdown)	(struct device * dev);
>  	int	(*suspend)	(struct device * dev, pm_message_t state);
>  	int	(*resume)	(struct device * dev);
> -
> -	unsigned int multithread_probe:1;
>  };
>  
>  
> --- linux-2.6.orig/include/linux/pci.h
> +++ linux-2.6/include/linux/pci.h
> @@ -356,8 +356,6 @@ struct pci_driver {
>  	struct pci_error_handlers *err_handler;
>  	struct device_driver	driver;
>  	struct pci_dynids dynids;
> -
> -	int multithread_probe;
>  };
>  
>  #define	to_pci_driver(drv) container_of(drv,struct pci_driver, driver)
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [Patch 3/3] Driver core: Per-subsystem multithreaded probing.

[Cornelia Huck]

On Mon, 16 Oct 2006 11:13:30 +0200,
Duncan Sands <duncan.sands@math.u-psud.fr> wrote:

> There may have been a similar problem with
> USB locking, since there too probe was expecting a lock to be held that might
> not be held when called from the kthread:
> 
> 	 * This function must be called with @dev->sem held.  When called for a
> 	 * USB interface, @dev->parent->sem must be held as well.
> 	 */
> 	int driver_probe_device(struct device_driver * drv, struct device * dev)

But as we don't know we're probing an usb interface, we have no chance
of ensuring that dev->parent->sem is taken in the multithreaded case
(meaning we couldn't do multithreaded probe for usb). (Any idea why the
parent's sem must be taken for usb interfaces?)

> Also, what about device removal racing with probe?  Is it possible for someone to
> attempt to remove a device in the gap between the call to device_attach and the
> kthread actually running and doing the probe?  That would result in remove and
> probe being called in the wrong order...

->probe won't be called if the device is already being removed, but
that still results in bus->remove being called without a prior ->probe
(but not drv->probe since dev->driver is not set at that time).

Re: [Patch 3/3] Driver core: Per-subsystem multithreaded probing.

[Alan Stern]

On Mon, 16 Oct 2006, Cornelia Huck wrote:

> On Mon, 16 Oct 2006 11:13:30 +0200,
> Duncan Sands <duncan.sands@math.u-psud.fr> wrote:
> 
> > There may have been a similar problem with
> > USB locking, since there too probe was expecting a lock to be held that might
> > not be held when called from the kthread:
> > 
> > 	 * This function must be called with @dev->sem held.  When called for a
> > 	 * USB interface, @dev->parent->sem must be held as well.
> > 	 */
> > 	int driver_probe_device(struct device_driver * drv, struct device * dev)
> 
> But as we don't know we're probing an usb interface, we have no chance
> of ensuring that dev->parent->sem is taken in the multithreaded case
> (meaning we couldn't do multithreaded probe for usb).

That's not quite true.  You could acquire dev->parent->sem always, just to
be certain.  However USB shouldn't use this form of multithreaded probing
in any case; it should instead use multiple threads for khubd.

> (Any idea why the
> parent's sem must be taken for usb interfaces?)

It's a peculiarity of the way USB works that some commands (Set
Configuration and port reset) affect the entire device, even though they
may be needed only by a driver for a single interface.  Some interface
drivers do need to issue these commands when they are probed.  The only
safe way to do it is to insure that their probe routines are called with
both the device and the interface semaphores held.

> > Also, what about device removal racing with probe?  Is it possible for someone to
> > attempt to remove a device in the gap between the call to device_attach and the
> > kthread actually running and doing the probe?  That would result in remove and
> > probe being called in the wrong order...
> 
> ->probe won't be called if the device is already being removed,

Because driver_probe_device() checks device_is_registered(dev) before 
doing anything else.

>  but
> that still results in bus->remove being called without a prior ->probe
> (but not drv->probe since dev->driver is not set at that time).

How so?  We shouldn't call bus->remove if a driver isn't bound.

Some other things were left out of the patch.  Since we can no longer know 
whether any drivers will get bound at all, device_attach() should now 
return void.  This means that bus_attach_device() can be simplified and 
should also return void.

Alan Stern

Re: [Patch 3/3] Driver core: Per-subsystem multithreaded probing.

[Cornelia Huck]

On Mon, 16 Oct 2006 10:59:28 -0400 (EDT),
Alan Stern <stern@rowland.harvard.edu> wrote:

> That's not quite true.  You could acquire dev->parent->sem always, just to
> be certain.  

But dev->parent->sem wouldn't be taken in the non-multithreaded path,
so we would change the semantics.

> However USB shouldn't use this form of multithreaded probing
> in any case; it should instead use multiple threads for khubd.

OK, so usb shouldn't request multithreaded probe.

> >  but
> > that still results in bus->remove being called without a prior ->probe
> > (but not drv->probe since dev->driver is not set at that time).
> 
> How so?  We shouldn't call bus->remove if a driver isn't bound.

Eh, yes. I was confused :)

> Some other things were left out of the patch.  Since we can no longer know 
> whether any drivers will get bound at all, device_attach() should now 
> return void.

But device_bind_driver() may still return an error, if creating the
links failed.

-- 
Cornelia Huck
Linux for zSeries Developer
Tel.: +49-7031-16-4837, Mail: cornelia.huck@de.ibm.com

Re: [Patch 3/3] Driver core: Per-subsystem multithreaded probing.

[Alan Stern]

On Mon, 16 Oct 2006, Cornelia Huck wrote:

> On Mon, 16 Oct 2006 10:59:28 -0400 (EDT),
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > That's not quite true.  You could acquire dev->parent->sem always, just to
> > be certain.  
> 
> But dev->parent->sem wouldn't be taken in the non-multithreaded path,
> so we would change the semantics.

You would only be changing it for the multithreaded path, which itself is 
very new.  But this is all hypothetical anyway...


> > Some other things were left out of the patch.  Since we can no longer know 
> > whether any drivers will get bound at all, device_attach() should now 
> > return void.
> 
> But device_bind_driver() may still return an error, if creating the
> links failed.

So the question is what do do if someone calls device_register() or
device_add() with dev->driver set.  If everything succeeds except for
creation of the symlinks, should the device remain registered?  The driver
core has been vacillating about this recently.  I'm not sure what the 
right answer is.

However the kerneldoc for device_attach() should be updated to mention
that when multithreaded probing is used, a driver might end up getting
bound even though the return value is 0.

Alan Stern

P.S.: If you initialize probe_task to ERR_PTR(-ENOMEM) or something 
similar, then you could eliminate one of the calls to bus_for_each_drv() 
in device_attach().

Re: [Patch 3/3] Driver core: Per-subsystem multithreaded probing.

[Cornelia Huck]

On Mon, 16 Oct 2006 11:56:29 -0400 (EDT),
Alan Stern <stern@rowland.harvard.edu> wrote:

> You would only be changing it for the multithreaded path, which itself is 
> very new.  But this is all hypothetical anyway...

I think we're talking about the same thing :)

> So the question is what do do if someone calls device_register() or
> device_add() with dev->driver set.  If everything succeeds except for
> creation of the symlinks, should the device remain registered?  The driver
> core has been vacillating about this recently.  I'm not sure what the 
> right answer is.

If dev->driver has not been set and symlink creation failed, we'll end
up with an unbound but registered device. Maybe the same should happen
if dev->driver had been set before (register the device but reset
dev->driver to NULL)?

> However the kerneldoc for device_attach() should be updated to mention
> that when multithreaded probing is used, a driver might end up getting
> bound even though the return value is 0.

Agreed.
 
> P.S.: If you initialize probe_task to ERR_PTR(-ENOMEM) or something 
> similar, then you could eliminate one of the calls to bus_for_each_drv() 
> in device_attach().

I'll do a new patch series tomorrow.

-- 
Cornelia Huck
Linux for zSeries Developer
Tel.: +49-7031-16-4837, Mail: cornelia.huck@de.ibm.com

Re: [Patch 3/3] Driver core: Per-subsystem multithreaded probing.

[Alan Stern]

On Mon, 16 Oct 2006, Cornelia Huck wrote:

> > So the question is what do do if someone calls device_register() or
> > device_add() with dev->driver set.  If everything succeeds except for
> > creation of the symlinks, should the device remain registered?  The driver
> > core has been vacillating about this recently.  I'm not sure what the 
> > right answer is.
> 
> If dev->driver has not been set and symlink creation failed, we'll end
> up with an unbound but registered device. Maybe the same should happen
> if dev->driver had been set before (register the device but reset
> dev->driver to NULL)?

That _seems_ reasonable.  But what do subsystem drivers expect?  They
might assume that the binding succeeded whenever the registration 
succeeded.  Although it may not make any real difference in the end.

And of course, we always have the excuse that up until a week ago, failure
to create the symlinks would not cause device registration to fail...

Alan Stern

Re: [Patch 3/3] Driver core: Per-subsystem multithreaded probing.

[Duncan Sands]

> That's not quite true.  You could acquire dev->parent->sem always, just to
> be certain.  However USB shouldn't use this form of multithreaded probing
> in any case; it should instead use multiple threads for khubd.

Is anyone working on this (multiple threads for khubd)?

Ciao,

Duncan.

Re: [Patch 3/3] Driver core: Per-subsystem multithreaded probing.

[Alan Stern]

On Mon, 16 Oct 2006, Duncan Sands wrote:

> > That's not quite true.  You could acquire dev->parent->sem always, just to
> > be certain.  However USB shouldn't use this form of multithreaded probing
> > in any case; it should instead use multiple threads for khubd.
> 
> Is anyone working on this (multiple threads for khubd)?

Not as far as I know.  It should be trivial, though.  Just start several 
threads instead of only one during initialization, and stop them all 
during cleanup.  And have the threads use a wait queue instead of calling
wait_event_interruptible().

Alan Stern

Re: [Patch 3/3] Driver core: Per-subsystem multithreaded probing.

[Alan Stern]

On Mon, 16 Oct 2006, Cornelia Huck wrote:

> ->probe won't be called if the device is already being removed, but
> that still results in bus->remove being called without a prior ->probe
> (but not drv->probe since dev->driver is not set at that time).

One other thing I forgot to mention...  In device_attach(), if dev->driver 
is already set upon entry and the call to device_bind_driver() fails, 
dev->driver should be set to NULL before returning.  We don't want the 
device to appear to be bound when in fact it isn't.

Alan Stern