[PATCH] driver core: check bus->match without holding device lock

[PATCH] driver core: check bus->match without holding device lock

[tom.leiming]

From: Ming Lei <tom.leiming@gmail.com>

This patch moves bus->match out from driver_probe_device and
does not hold device lock to check the match between a device
and a driver.

The idea has been verified by the commit 6cd495860901,
which leads to a faster boot. But the commit 6cd495860901 has
the following drawbacks: 1),only does the quick check in
the path of __driver_attach->driver_probe_device, not in other
paths; 2),for a matched device and driver, check the same match
twice. It is a waste of cpu ,especially for some drivers with long
device id table (eg. usb-storage driver).

This patch adds a helper of driver_match_device to check the match
in all paths, and testes the match only once.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/base/bus.c     |    8 +++++++-
 drivers/base/dd.c      |   19 +++++++------------
 include/linux/device.h |    6 ++++++
 3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 83f32b8..c570d16 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -199,6 +199,12 @@ static ssize_t driver_bind(struct device_driver *drv,
 
 	dev = bus_find_device_by_name(bus, NULL, buf);
 	if (dev && dev->driver == NULL) {
+
+		if (!driver_match_device(drv, dev)) {
+			err = 0;
+			goto not_match;
+		}
+
 		if (dev->parent)	/* Needed for USB */
 			down(&dev->parent->sem);
 		down(&dev->sem);
@@ -206,7 +212,7 @@ static ssize_t driver_bind(struct device_driver *drv,
 		up(&dev->sem);
 		if (dev->parent)
 			up(&dev->parent->sem);
-
+not_match:
 		if (err > 0) {
 			/* success */
 			err = count;
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 315bed8..61f32db 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -172,14 +172,8 @@ int driver_probe_done(void)
  * @drv: driver to bind a device to
  * @dev: device to try to bind to the driver
  *
- * First, we call the bus's match function, if one present, which should
- * compare the device IDs the driver supports with the device IDs of the
- * device. Note we don't do this ourselves because we don't know the
- * format of the ID structures, nor what is to be considered a match and
- * what is not.
- *
- * This function returns 1 if a match is found, -ENODEV if the device is
- * not registered, and 0 otherwise.
+ * This function returns -ENODEV if the device is not registered,
+ * and 0 otherwise.
  *
  * This function must be called with @dev->sem held.  When called for a
  * USB interface, @dev->parent->sem must be held as well.
@@ -190,21 +184,22 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
 
 	if (!device_is_registered(dev))
 		return -ENODEV;
-	if (drv->bus->match && !drv->bus->match(dev, drv))
-		goto done;
 
 	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
 		 drv->bus->name, __func__, dev_name(dev), drv->name);
 
 	ret = really_probe(dev, drv);
 
-done:
 	return ret;
 }
 
 static int __device_attach(struct device_driver *drv, void *data)
 {
 	struct device *dev = data;
+
+	if (!driver_match_device(drv, dev))
+		return 0;
+
 	return driver_probe_device(drv, dev);
 }
 
@@ -257,7 +252,7 @@ static int __driver_attach(struct device *dev, void *data)
 	 * is an error.
 	 */
 
-	if (drv->bus->match && !drv->bus->match(dev, drv))
+	if (!driver_match_device(drv, dev))
 		return 0;
 
 	if (dev->parent)	/* Needed for USB */
diff --git a/include/linux/device.h b/include/linux/device.h
index 45e5b19..3c61315 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -466,6 +466,12 @@ static inline int device_is_registered(struct device *dev)
 	return dev->kobj.state_in_sysfs;
 }
 
+static inline int driver_match_device(struct device_driver *drv,
+				      struct device *dev)
+{
+	return drv->bus->match && drv->bus->match(dev, drv);
+}
+
 void driver_init(void);
 
 /*
-- 
1.6.0

Re: [PATCH] driver core: check bus->match without holding device lock

[Cornelia Huck]

On Tue, 20 Jan 2009 21:34:08 +0800,
tom.leiming@gmail.com wrote:

> From: Ming Lei <tom.leiming@gmail.com>
> 
> This patch moves bus->match out from driver_probe_device and
> does not hold device lock to check the match between a device
> and a driver.
> 
> The idea has been verified by the commit 6cd495860901,
> which leads to a faster boot. But the commit 6cd495860901 has
> the following drawbacks: 1),only does the quick check in
> the path of __driver_attach->driver_probe_device, not in other
> paths; 2),for a matched device and driver, check the same match
> twice. It is a waste of cpu ,especially for some drivers with long
> device id table (eg. usb-storage driver).

I agree with the goal of that patch, as it essentially cleanly
seperates the quick check (->match) from the deeper probing (->probe).

> 
> This patch adds a helper of driver_match_device to check the match
> in all paths, and testes the match only once.
> 
> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
> ---
>  drivers/base/bus.c     |    8 +++++++-
>  drivers/base/dd.c      |   19 +++++++------------
>  include/linux/device.h |    6 ++++++
>  3 files changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 83f32b8..c570d16 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -199,6 +199,12 @@ static ssize_t driver_bind(struct device_driver *drv,
> 
>  	dev = bus_find_device_by_name(bus, NULL, buf);
>  	if (dev && dev->driver == NULL) {
> +
> +		if (!driver_match_device(drv, dev)) {
> +			err = 0;
> +			goto not_match;
> +		}
> +

This looks too complicated, I'd just change the if to
	if (dev && dev->driver == NULL && driver_match_device(drv,dev))

>  		if (dev->parent)	/* Needed for USB */
>  			down(&dev->parent->sem);
>  		down(&dev->sem);
> @@ -206,7 +212,7 @@ static ssize_t driver_bind(struct device_driver *drv,
>  		up(&dev->sem);
>  		if (dev->parent)
>  			up(&dev->parent->sem);
> -
> +not_match:
>  		if (err > 0) {
>  			/* success */
>  			err = count;
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 315bed8..61f32db 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -172,14 +172,8 @@ int driver_probe_done(void)
>   * @drv: driver to bind a device to
>   * @dev: device to try to bind to the driver
>   *
> - * First, we call the bus's match function, if one present, which should
> - * compare the device IDs the driver supports with the device IDs of the
> - * device. Note we don't do this ourselves because we don't know the
> - * format of the ID structures, nor what is to be considered a match and
> - * what is not.
> - *
> - * This function returns 1 if a match is found, -ENODEV if the device is
> - * not registered, and 0 otherwise.
> + * This function returns -ENODEV if the device is not registered,
> + * and 0 otherwise.

This is incorrect, really_probe() still returns 1 if the device could
be bound.

>   *
>   * This function must be called with @dev->sem held.  When called for a
>   * USB interface, @dev->parent->sem must be held as well.
> @@ -190,21 +184,22 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
> 
>  	if (!device_is_registered(dev))
>  		return -ENODEV;
> -	if (drv->bus->match && !drv->bus->match(dev, drv))
> -		goto done;
> 
>  	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
>  		 drv->bus->name, __func__, dev_name(dev), drv->name);
> 
>  	ret = really_probe(dev, drv);
> 
> -done:
>  	return ret;
>  }
> 
>  static int __device_attach(struct device_driver *drv, void *data)
>  {
>  	struct device *dev = data;
> +
> +	if (!driver_match_device(drv, dev))
> +		return 0;
> +
>  	return driver_probe_device(drv, dev);
>  }
> 
> @@ -257,7 +252,7 @@ static int __driver_attach(struct device *dev, void *data)
>  	 * is an error.
>  	 */
> 
> -	if (drv->bus->match && !drv->bus->match(dev, drv))
> +	if (!driver_match_device(drv, dev))
>  		return 0;
> 
>  	if (dev->parent)	/* Needed for USB */
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 45e5b19..3c61315 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -466,6 +466,12 @@ static inline int device_is_registered(struct device *dev)
>  	return dev->kobj.state_in_sysfs;
>  }
> 
> +static inline int driver_match_device(struct device_driver *drv,
> +				      struct device *dev)
> +{
> +	return drv->bus->match && drv->bus->match(dev, drv);
> +}
> +

This should go into drivers/base/base.h instead, as no code outside the
driver core should use it.

>  void driver_init(void);
> 
>  /*

Re: [PATCH] driver core: check bus->match without holding device lock

[Ming Lei]

Thanks a lot for your reply.
I'll resend a patch with your suggestions.

2009/1/20 Cornelia Huck <cornelia.huck@de.ibm.com>:
> On Tue, 20 Jan 2009 21:34:08 +0800,
> tom.leiming@gmail.com wrote:
>
>> From: Ming Lei <tom.leiming@gmail.com>
>>
>> This patch moves bus->match out from driver_probe_device and
>> does not hold device lock to check the match between a device
>> and a driver.
>>
>> The idea has been verified by the commit 6cd495860901,
>> which leads to a faster boot. But the commit 6cd495860901 has
>> the following drawbacks: 1),only does the quick check in
>> the path of __driver_attach->driver_probe_device, not in other
>> paths; 2),for a matched device and driver, check the same match
>> twice. It is a waste of cpu ,especially for some drivers with long
>> device id table (eg. usb-storage driver).
>
> I agree with the goal of that patch, as it essentially cleanly
> seperates the quick check (->match) from the deeper probing (->probe).
>
>>
>> This patch adds a helper of driver_match_device to check the match
>> in all paths, and testes the match only once.
>>
>> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
>> ---
>>  drivers/base/bus.c     |    8 +++++++-
>>  drivers/base/dd.c      |   19 +++++++------------
>>  include/linux/device.h |    6 ++++++
>>  3 files changed, 20 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
>> index 83f32b8..c570d16 100644
>> --- a/drivers/base/bus.c
>> +++ b/drivers/base/bus.c
>> @@ -199,6 +199,12 @@ static ssize_t driver_bind(struct device_driver *drv,
>>
>>       dev = bus_find_device_by_name(bus, NULL, buf);
>>       if (dev && dev->driver == NULL) {
>> +
>> +             if (!driver_match_device(drv, dev)) {
>> +                     err = 0;
>> +                     goto not_match;
>> +             }
>> +
>
> This looks too complicated, I'd just change the if to
>        if (dev && dev->driver == NULL && driver_match_device(drv,dev))
>
>>               if (dev->parent)        /* Needed for USB */
>>                       down(&dev->parent->sem);
>>               down(&dev->sem);
>> @@ -206,7 +212,7 @@ static ssize_t driver_bind(struct device_driver *drv,
>>               up(&dev->sem);
>>               if (dev->parent)
>>                       up(&dev->parent->sem);
>> -
>> +not_match:
>>               if (err > 0) {
>>                       /* success */
>>                       err = count;
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 315bed8..61f32db 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -172,14 +172,8 @@ int driver_probe_done(void)
>>   * @drv: driver to bind a device to
>>   * @dev: device to try to bind to the driver
>>   *
>> - * First, we call the bus's match function, if one present, which should
>> - * compare the device IDs the driver supports with the device IDs of the
>> - * device. Note we don't do this ourselves because we don't know the
>> - * format of the ID structures, nor what is to be considered a match and
>> - * what is not.
>> - *
>> - * This function returns 1 if a match is found, -ENODEV if the device is
>> - * not registered, and 0 otherwise.
>> + * This function returns -ENODEV if the device is not registered,
>> + * and 0 otherwise.
>
> This is incorrect, really_probe() still returns 1 if the device could
> be bound.
>
>>   *
>>   * This function must be called with @dev->sem held.  When called for a
>>   * USB interface, @dev->parent->sem must be held as well.
>> @@ -190,21 +184,22 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
>>
>>       if (!device_is_registered(dev))
>>               return -ENODEV;
>> -     if (drv->bus->match && !drv->bus->match(dev, drv))
>> -             goto done;
>>
>>       pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
>>                drv->bus->name, __func__, dev_name(dev), drv->name);
>>
>>       ret = really_probe(dev, drv);
>>
>> -done:
>>       return ret;
>>  }
>>
>>  static int __device_attach(struct device_driver *drv, void *data)
>>  {
>>       struct device *dev = data;
>> +
>> +     if (!driver_match_device(drv, dev))
>> +             return 0;
>> +
>>       return driver_probe_device(drv, dev);
>>  }
>>
>> @@ -257,7 +252,7 @@ static int __driver_attach(struct device *dev, void *data)
>>        * is an error.
>>        */
>>
>> -     if (drv->bus->match && !drv->bus->match(dev, drv))
>> +     if (!driver_match_device(drv, dev))
>>               return 0;
>>
>>       if (dev->parent)        /* Needed for USB */
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 45e5b19..3c61315 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -466,6 +466,12 @@ static inline int device_is_registered(struct device *dev)
>>       return dev->kobj.state_in_sysfs;
>>  }
>>
>> +static inline int driver_match_device(struct device_driver *drv,
>> +                                   struct device *dev)
>> +{
>> +     return drv->bus->match && drv->bus->match(dev, drv);
>> +}
>> +
>
> This should go into drivers/base/base.h instead, as no code outside the
> driver core should use it.
>
>>  void driver_init(void);
>>
>>  /*
>



-- 
Lei Ming

Re: [PATCH] driver core: check bus->match without holding device lock

[Guennadi Liakhovetski]

Hi,

this patch breaks soc-camera because of this change in behaviour:

On Tue, 20 Jan 2009, tom.leiming@gmail.com wrote:

> From: Ming Lei <tom.leiming@gmail.com>
> 
> This patch moves bus->match out from driver_probe_device and
> does not hold device lock to check the match between a device
> and a driver.
> 
> The idea has been verified by the commit 6cd495860901,
> which leads to a faster boot. But the commit 6cd495860901 has
> the following drawbacks: 1),only does the quick check in
> the path of __driver_attach->driver_probe_device, not in other
> paths; 2),for a matched device and driver, check the same match
> twice. It is a waste of cpu ,especially for some drivers with long
> device id table (eg. usb-storage driver).
> 
> This patch adds a helper of driver_match_device to check the match
> in all paths, and testes the match only once.
> 
> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
> ---
>  drivers/base/bus.c     |    8 +++++++-
>  drivers/base/dd.c      |   19 +++++++------------
>  include/linux/device.h |    6 ++++++
>  3 files changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 83f32b8..c570d16 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -199,6 +199,12 @@ static ssize_t driver_bind(struct device_driver *drv,
>  
>  	dev = bus_find_device_by_name(bus, NULL, buf);
>  	if (dev && dev->driver == NULL) {
> +
> +		if (!driver_match_device(drv, dev)) {
> +			err = 0;
> +			goto not_match;
> +		}
> +
>  		if (dev->parent)	/* Needed for USB */
>  			down(&dev->parent->sem);
>  		down(&dev->sem);
> @@ -206,7 +212,7 @@ static ssize_t driver_bind(struct device_driver *drv,
>  		up(&dev->sem);
>  		if (dev->parent)
>  			up(&dev->parent->sem);
> -
> +not_match:
>  		if (err > 0) {
>  			/* success */
>  			err = count;
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 315bed8..61f32db 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -172,14 +172,8 @@ int driver_probe_done(void)
>   * @drv: driver to bind a device to
>   * @dev: device to try to bind to the driver
>   *
> - * First, we call the bus's match function, if one present, which should
> - * compare the device IDs the driver supports with the device IDs of the
> - * device. Note we don't do this ourselves because we don't know the
> - * format of the ID structures, nor what is to be considered a match and
> - * what is not.
> - *
> - * This function returns 1 if a match is found, -ENODEV if the device is
> - * not registered, and 0 otherwise.
> + * This function returns -ENODEV if the device is not registered,
> + * and 0 otherwise.
>   *
>   * This function must be called with @dev->sem held.  When called for a
>   * USB interface, @dev->parent->sem must be held as well.
> @@ -190,21 +184,22 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
>  
>  	if (!device_is_registered(dev))
>  		return -ENODEV;
> -	if (drv->bus->match && !drv->bus->match(dev, drv))
> -		goto done;

Previously, if no .match() was specified, the normal probing has been 
called.

>  
>  	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
>  		 drv->bus->name, __func__, dev_name(dev), drv->name);
>  
>  	ret = really_probe(dev, drv);
>  
> -done:
>  	return ret;
>  }
>  
>  static int __device_attach(struct device_driver *drv, void *data)
>  {
>  	struct device *dev = data;
> +
> +	if (!driver_match_device(drv, dev))
> +		return 0;
> +

Now, without .match() no probing is done. Is this an intended change and 
soc-camera has to be fixed or is this a bug?

>  	return driver_probe_device(drv, dev);
>  }
>  
> @@ -257,7 +252,7 @@ static int __driver_attach(struct device *dev, void *data)
>  	 * is an error.
>  	 */
>  
> -	if (drv->bus->match && !drv->bus->match(dev, drv))
> +	if (!driver_match_device(drv, dev))
>  		return 0;
>  
>  	if (dev->parent)	/* Needed for USB */
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 45e5b19..3c61315 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -466,6 +466,12 @@ static inline int device_is_registered(struct device *dev)
>  	return dev->kobj.state_in_sysfs;
>  }
>  
> +static inline int driver_match_device(struct device_driver *drv,
> +				      struct device *dev)
> +{
> +	return drv->bus->match && drv->bus->match(dev, drv);
> +}
> +
>  void driver_init(void);
>  
>  /*
> -- 
> 1.6.0
> 
> --
> 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/
> 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH] driver core: check bus->match without holding device lock

[Ming Lei]

2009/3/25 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> Hi,
>
> this patch breaks soc-camera because of this change in behaviour:
>
> On Tue, 20 Jan 2009, tom.leiming@gmail.com wrote:
>
>> From: Ming Lei <tom.leiming@gmail.com>
>>
>> This patch moves bus->match out from driver_probe_device and
>> does not hold device lock to check the match between a device
>> and a driver.
>>
>> The idea has been verified by the commit 6cd495860901,
>> which leads to a faster boot. But the commit 6cd495860901 has
>> the following drawbacks: 1),only does the quick check in
>> the path of __driver_attach->driver_probe_device, not in other
>> paths; 2),for a matched device and driver, check the same match
>> twice. It is a waste of cpu ,especially for some drivers with long
>> device id table (eg. usb-storage driver).
>>
>> This patch adds a helper of driver_match_device to check the match
>> in all paths, and testes the match only once.
>>
>> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
>> ---
>>  drivers/base/bus.c     |    8 +++++++-
>>  drivers/base/dd.c      |   19 +++++++------------
>>  include/linux/device.h |    6 ++++++
>>  3 files changed, 20 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
>> index 83f32b8..c570d16 100644
>> --- a/drivers/base/bus.c
>> +++ b/drivers/base/bus.c
>> @@ -199,6 +199,12 @@ static ssize_t driver_bind(struct device_driver *drv,
>>
>>       dev = bus_find_device_by_name(bus, NULL, buf);
>>       if (dev && dev->driver == NULL) {
>> +
>> +             if (!driver_match_device(drv, dev)) {
>> +                     err = 0;
>> +                     goto not_match;
>> +             }
>> +
>>               if (dev->parent)        /* Needed for USB */
>>                       down(&dev->parent->sem);
>>               down(&dev->sem);
>> @@ -206,7 +212,7 @@ static ssize_t driver_bind(struct device_driver *drv,
>>               up(&dev->sem);
>>               if (dev->parent)
>>                       up(&dev->parent->sem);
>> -
>> +not_match:
>>               if (err > 0) {
>>                       /* success */
>>                       err = count;
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 315bed8..61f32db 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -172,14 +172,8 @@ int driver_probe_done(void)
>>   * @drv: driver to bind a device to
>>   * @dev: device to try to bind to the driver
>>   *
>> - * First, we call the bus's match function, if one present, which should
>> - * compare the device IDs the driver supports with the device IDs of the
>> - * device. Note we don't do this ourselves because we don't know the
>> - * format of the ID structures, nor what is to be considered a match and
>> - * what is not.
>> - *
>> - * This function returns 1 if a match is found, -ENODEV if the device is
>> - * not registered, and 0 otherwise.
>> + * This function returns -ENODEV if the device is not registered,
>> + * and 0 otherwise.
>>   *
>>   * This function must be called with @dev->sem held.  When called for a
>>   * USB interface, @dev->parent->sem must be held as well.
>> @@ -190,21 +184,22 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
>>
>>       if (!device_is_registered(dev))
>>               return -ENODEV;
>> -     if (drv->bus->match && !drv->bus->match(dev, drv))
>> -             goto done;
>
> Previously, if no .match() was specified, the normal probing has been
> called.
>
>>
>>       pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
>>                drv->bus->name, __func__, dev_name(dev), drv->name);
>>
>>       ret = really_probe(dev, drv);
>>
>> -done:
>>       return ret;
>>  }
>>
>>  static int __device_attach(struct device_driver *drv, void *data)
>>  {
>>       struct device *dev = data;
>> +
>> +     if (!driver_match_device(drv, dev))
>> +             return 0;
>> +
>
> Now, without .match() no probing is done. Is this an intended change and
> soc-camera has to be fixed or is this a bug?

It is not a driver-core bug, and soc-camera should be fixed.

>
>>       return driver_probe_device(drv, dev);
>>  }
>>
>> @@ -257,7 +252,7 @@ static int __driver_attach(struct device *dev, void *data)
>>        * is an error.
>>        */
>>
>> -     if (drv->bus->match && !drv->bus->match(dev, drv))
>> +     if (!driver_match_device(drv, dev))
>>               return 0;
>>
>>       if (dev->parent)        /* Needed for USB */
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 45e5b19..3c61315 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -466,6 +466,12 @@ static inline int device_is_registered(struct device *dev)
>>       return dev->kobj.state_in_sysfs;
>>  }
>>
>> +static inline int driver_match_device(struct device_driver *drv,
>> +                                   struct device *dev)
>> +{
>> +     return drv->bus->match && drv->bus->match(dev, drv);
>> +}
>> +
>>  void driver_init(void);
>>
>>  /*
>> --
>> 1.6.0
>>
>> --
>> 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/
>>
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
>



-- 
Lei Ming

Re: [PATCH] driver core: check bus->match without holding device lock

[Guennadi Liakhovetski]

On Wed, 25 Mar 2009, Ming Lei wrote:

> >> @@ -190,21 +184,22 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
> >>
> >>       if (!device_is_registered(dev))
> >>               return -ENODEV;
> >> -     if (drv->bus->match && !drv->bus->match(dev, drv))
> >> -             goto done;
> >
> > Previously, if no .match() was specified, the normal probing has been
> > called.
> >
> >>
> >>       pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
> >>                drv->bus->name, __func__, dev_name(dev), drv->name);
> >>
> >>       ret = really_probe(dev, drv);
> >>
> >> -done:
> >>       return ret;
> >>  }
> >>
> >>  static int __device_attach(struct device_driver *drv, void *data)
> >>  {
> >>       struct device *dev = data;
> >> +
> >> +     if (!driver_match_device(drv, dev))
> >> +             return 0;
> >> +
> >
> > Now, without .match() no probing is done. Is this an intended change and
> > soc-camera has to be fixed or is this a bug?
> 
> It is not a driver-core bug, and soc-camera should be fixed.

So, you're saying this used to be a bug and it has been fixed by this 
patch? Then why isn't this mentioned in the commit message? The commit 
text seems to suggest, that this patch shouldn't introduce any change in 
behaviour, but it does. So, before .match == NULL lead to .probe() being 
called, and now it doesn't anymore?

And in which way should soc-camera be fixed? Just provide a dummy match 
with just "return 1;" in it?

> >> diff --git a/include/linux/device.h b/include/linux/device.h
> >> index 45e5b19..3c61315 100644
> >> --- a/include/linux/device.h
> >> +++ b/include/linux/device.h
> >> @@ -466,6 +466,12 @@ static inline int device_is_registered(struct device *dev)
> >>       return dev->kobj.state_in_sysfs;
> >>  }
> >>
> >> +static inline int driver_match_device(struct device_driver *drv,
> >> +                                   struct device *dev)
> >> +{
> >> +     return drv->bus->match && drv->bus->match(dev, drv);
> >> +}
> >> +
> >>  void driver_init(void);

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH] driver core: check bus->match without holding device lock

[Ming Lei]

>> > Now, without .match() no probing is done. Is this an intended change and
>> > soc-camera has to be fixed or is this a bug?
>>
>> It is not a driver-core bug, and soc-camera should be fixed.
>
> So, you're saying this used to be a bug and it has been fixed by this
> patch? Then why isn't this mentioned in the commit message? The commit
> text seems to suggest, that this patch shouldn't introduce any change in
> behaviour, but it does. So, before .match == NULL lead to .probe() being
> called, and now it doesn't anymore?

Where is soc-camera  driver in kernel tree?  Which bus  is soc-camera
device (driver) attached to ?
Why doesn't soc-camera  driver  have a match method?

Thanks!


-- 
Lei Ming

Re: [PATCH] driver core: check bus->match without holding device lock

[Guennadi Liakhovetski]

On Wed, 25 Mar 2009, Ming Lei wrote:

> >> > Now, without .match() no probing is done. Is this an intended change and
> >> > soc-camera has to be fixed or is this a bug?
> >>
> >> It is not a driver-core bug, and soc-camera should be fixed.
> >
> > So, you're saying this used to be a bug and it has been fixed by this
> > patch? Then why isn't this mentioned in the commit message? The commit
> > text seems to suggest, that this patch shouldn't introduce any change in
> > behaviour, but it does. So, before .match == NULL lead to .probe() being
> > called, and now it doesn't anymore?
> 
> Where is soc-camera  driver in kernel tree?

drivers/media/video/soc_camera.c

> Which bus  is soc-camera device (driver) attached to ?

camera bus.

> Why doesn't soc-camera  driver  have a match method?

Why should it? Because there is only one driver on this bus by definition 
(and I only register a device on the bus when I find a match between a 
device and its parent / driver).

What I in any case see wrong with this patch, is that it _silently_ 
changes kernel behaviour without even mentioning it in the commit log!

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH] driver core: check bus->match without holding device lock

[Ming Lei]

2009/3/25 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> On Wed, 25 Mar 2009, Ming Lei wrote:
>
>> >> > Now, without .match() no probing is done. Is this an intended change and
>> >> > soc-camera has to be fixed or is this a bug?
>> >>
>> >> It is not a driver-core bug, and soc-camera should be fixed.
>> >
>> > So, you're saying this used to be a bug and it has been fixed by this
>> > patch? Then why isn't this mentioned in the commit message? The commit
>> > text seems to suggest, that this patch shouldn't introduce any change in
>> > behaviour, but it does. So, before .match == NULL lead to .probe() being
>> > called, and now it doesn't anymore?
>>
>> Where is soc-camera  driver in kernel tree?
>
> drivers/media/video/soc_camera.c
>
>> Which bus  is soc-camera device (driver) attached to ?
>
> camera bus.
>
>> Why doesn't soc-camera  driver  have a match method?
>
> Why should it? Because there is only one driver on this bus by definition
> (and I only register a device on the bus when I find a match between a
> device and its parent / driver).

I grep the drivers directory ( grep -r -n -I -A 5 -w "struct bus_type"
./* ) and
find soc-camera is the __only__ bus-type which have not implemented  match
method, so it is better to define a match method(always return true)
in soc-camera
to solve the  problem, OK?

Also, I will submit a patch to warn absence of match (maybe should
return failed)
in bus_register().

>
> What I in any case see wrong with this patch, is that it _silently_
> changes kernel behaviour without even mentioning it in the commit log!

IMHO, the change should be reasonable, but it is missed carelessly in
commit log.

Thanks

>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
>



-- 
Lei Ming

Re: [PATCH] driver core: check bus->match without holding device lock

[Ming Lei]

2009/3/25 Ming Lei <tom.leiming@gmail.com>:
> 2009/3/25 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
>> On Wed, 25 Mar 2009, Ming Lei wrote:
>>
>>> >> > Now, without .match() no probing is done. Is this an intended change and
>>> >> > soc-camera has to be fixed or is this a bug?
>>> >>
>>> >> It is not a driver-core bug, and soc-camera should be fixed.
>>> >
>>> > So, you're saying this used to be a bug and it has been fixed by this
>>> > patch? Then why isn't this mentioned in the commit message? The commit
>>> > text seems to suggest, that this patch shouldn't introduce any change in
>>> > behaviour, but it does. So, before .match == NULL lead to .probe() being
>>> > called, and now it doesn't anymore?
>>>
>>> Where is soc-camera  driver in kernel tree?
>>
>> drivers/media/video/soc_camera.c
>>
>>> Which bus  is soc-camera device (driver) attached to ?
>>
>> camera bus.
>>
>>> Why doesn't soc-camera  driver  have a match method?
>>
>> Why should it? Because there is only one driver on this bus by definition
>> (and I only register a device on the bus when I find a match between a
>> device and its parent / driver).
>
> I grep the drivers directory ( grep -r -n -I -A 5 -w "struct bus_type"
> ./* ) and
> find soc-camera is the __only__ bus-type which have not implemented  match

Sorry, under arch and sound, there are some instances of bus_type defined, and
few of them ( two or three ) do not have a .match method. So it is better to
not change previous driver core behaviour and allow .probe called if bus->match
not defined.

I'll submit a patch to fix it ,and you need not to fix your soc-camera.

Thanks.

> method, so it is better to define a match method(always return true)
> in soc-camera
> to solve the  problem, OK?
>
> Also, I will submit a patch to warn absence of match (maybe should
> return failed)
> in bus_register().
>
>>
>> What I in any case see wrong with this patch, is that it _silently_
>> changes kernel behaviour without even mentioning it in the commit log!
>
> IMHO, the change should be reasonable, but it is missed carelessly in
> commit log.
>
> Thanks
>
>>
>> Thanks
>> Guennadi
>> ---
>> Guennadi Liakhovetski, Ph.D.
>> Freelance Open-Source Software Developer
>>
>
>
>
> --
> Lei Ming
>



-- 
Lei Ming