From: Ming Lei <[email protected]>
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 <[email protected]>
---
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
On Tue, 20 Jan 2009 21:34:08 +0800,
[email protected] wrote:
> From: Ming Lei <[email protected]>
>
> 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 <[email protected]>
> ---
> 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);
>
> /*
Thanks a lot for your reply.
I'll resend a patch with your suggestions.
2009/1/20 Cornelia Huck <[email protected]>:
> On Tue, 20 Jan 2009 21:34:08 +0800,
> [email protected] wrote:
>
>> From: Ming Lei <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
Hi,
this patch breaks soc-camera because of this change in behaviour:
On Tue, 20 Jan 2009, [email protected] wrote:
> From: Ming Lei <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 [email protected]
> 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
2009/3/25 Guennadi Liakhovetski <[email protected]>:
> Hi,
>
> this patch breaks soc-camera because of this change in behaviour:
>
> On Tue, 20 Jan 2009, [email protected] wrote:
>
>> From: Ming Lei <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> ?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 [email protected]
>> 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
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
>> > 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
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
2009/3/25 Guennadi Liakhovetski <[email protected]>:
> 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
2009/3/25 Ming Lei <[email protected]>:
> 2009/3/25 Guennadi Liakhovetski <[email protected]>:
>> 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