From: Zijun Hu <quic_zijuhu@quicinc.com>
Bus_type's match() should return bool type compatible integer 0 or 1
ideally since its main operations are lookup and comparison normally
actually, this rule is followed by ALL bus_types but @amba_bustype within
current v6.10 kernel tree, for @amba_bustype, ONLY extra -EPROBE_DEFER
may be returned, so mark those impossible or rare return values with
unlikely() to help readers understand device and driver binding logic.
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
drivers/base/dd.c | 16 ++++++++++++----
include/linux/device/bus.h | 9 ++++-----
2 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9b745ba54de1..288e19c9854b 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -928,7 +928,11 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
if (ret == 0) {
/* no match */
return 0;
- } else if (ret == -EPROBE_DEFER) {
+ } else if (unlikely(ret == -EPROBE_DEFER)) {
+ /*
+ * Only match() of @amba_bustype may return this error
+ * in current v6.10 tree, so also give unlikely() here.
+ */
dev_dbg(dev, "Device match requests probe deferral\n");
dev->can_match = true;
driver_deferred_probe_add(dev);
@@ -937,7 +941,7 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
* to match or bind with other drivers on the bus.
*/
return ret;
- } else if (ret < 0) {
+ } else if (unlikely(ret < 0)) {
dev_dbg(dev, "Bus failed to match device: %d\n", ret);
return ret;
} /* ret > 0 means positive match */
@@ -1172,7 +1176,11 @@ static int __driver_attach(struct device *dev, void *data)
if (ret == 0) {
/* no match */
return 0;
- } else if (ret == -EPROBE_DEFER) {
+ } else if (unlikely(ret == -EPROBE_DEFER)) {
+ /*
+ * Only match() of @amba_bustype may return this error
+ * in current v6.10 tree, so also give unlikely() here.
+ */
dev_dbg(dev, "Device match requests probe deferral\n");
dev->can_match = true;
driver_deferred_probe_add(dev);
@@ -1181,7 +1189,7 @@ static int __driver_attach(struct device *dev, void *data)
* another device on the bus.
*/
return 0;
- } else if (ret < 0) {
+ } else if (unlikely(ret < 0)) {
dev_dbg(dev, "Bus failed to match device: %d\n", ret);
/*
* Driver could not match with device, but may match with
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
index 807831d6bf0f..1766b555da11 100644
--- a/include/linux/device/bus.h
+++ b/include/linux/device/bus.h
@@ -29,12 +29,11 @@ struct fwnode_handle;
* @bus_groups: Default attributes of the bus.
* @dev_groups: Default attributes of the devices on the bus.
* @drv_groups: Default attributes of the device drivers on the bus.
- * @match: Called, perhaps multiple times, whenever a new device or driver
- * is added for this bus. It should return a positive value if the
+ * @match: Called, perhaps multiple times, whenever a new device or
+ * driver is added for this bus. It should return one if the
* given device can be handled by the given driver and zero
- * otherwise. It may also return error code if determining that
- * the driver supports the device is not possible. In case of
- * -EPROBE_DEFER it will queue the device for deferred probing.
+ * otherwise. It may also return -EPROBE_DEFER to queue the
+ * device for deferred probing.
* @uevent: Called when a device is added, removed, or a few other things
* that generate uevents to add the environment variables.
* @probe: Called when a new device or driver add to this bus, and callback
--
2.34.1On Wed, Sep 04, 2024 at 08:56:42PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
>
> Bus_type's match() should return bool type compatible integer 0 or 1
> ideally since its main operations are lookup and comparison normally
> actually, this rule is followed by ALL bus_types but @amba_bustype within
> current v6.10 kernel tree, for @amba_bustype, ONLY extra -EPROBE_DEFER
> may be returned, so mark those impossible or rare return values with
> unlikely() to help readers understand device and driver binding logic.
unlikely() and likely() should ONLY be used when you can measure the
performance impact. And any "scan all devices in the bus" function is
NOT performance critical at all. So this is not the place for that,
sorry.
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
> drivers/base/dd.c | 16 ++++++++++++----
> include/linux/device/bus.h | 9 ++++-----
> 2 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 9b745ba54de1..288e19c9854b 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -928,7 +928,11 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
> if (ret == 0) {
> /* no match */
> return 0;
> - } else if (ret == -EPROBE_DEFER) {
> + } else if (unlikely(ret == -EPROBE_DEFER)) {
> + /*
> + * Only match() of @amba_bustype may return this error
> + * in current v6.10 tree, so also give unlikely() here.
version numbers in the kernel source mean nothing, and they age
horribly. This is not going to work out at all.
let's fix up the one user that is doing this wrong and then we don't
have to worry about it, right? We have the source for everything, let's
use it :)
thanks,
greg k-h
On 2024/9/4 21:53, Greg Kroah-Hartman wrote:
> On Wed, Sep 04, 2024 at 08:56:42PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> Bus_type's match() should return bool type compatible integer 0 or 1
>> ideally since its main operations are lookup and comparison normally
>> actually, this rule is followed by ALL bus_types but @amba_bustype within
>> current v6.10 kernel tree, for @amba_bustype, ONLY extra -EPROBE_DEFER
>> may be returned, so mark those impossible or rare return values with
>> unlikely() to help readers understand device and driver binding logic.
>
> unlikely() and likely() should ONLY be used when you can measure the
> performance impact. And any "scan all devices in the bus" function is
> NOT performance critical at all. So this is not the place for that,
> sorry.
>
make sense
thank you for these explanation.
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>> drivers/base/dd.c | 16 ++++++++++++----
>> include/linux/device/bus.h | 9 ++++-----
>> 2 files changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 9b745ba54de1..288e19c9854b 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -928,7 +928,11 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
>> if (ret == 0) {
>> /* no match */
>> return 0;
>> - } else if (ret == -EPROBE_DEFER) {
>> + } else if (unlikely(ret == -EPROBE_DEFER)) {
>> + /*
>> + * Only match() of @amba_bustype may return this error
>> + * in current v6.10 tree, so also give unlikely() here.
>
> version numbers in the kernel source mean nothing, and they age
> horribly. This is not going to work out at all.
>
> let's fix up the one user that is doing this wrong and then we don't
> have to worry about it, right? We have the source for everything, let's
> use it :)
>
yes, you are right.
my original motivation is to see if it is possible change bus_type's
match() return value type to bool type based on below reasons:
(1)
it is not good time for bus_type's match() to operate device such as
I/O since device may be not ready to operate at that time.
(2)
match() is called without holding device lock firstly by driver_attach().
(3)
all bus_type's match() only do lookup and comparison and return bool
type but @amba_bustype which operate device and maybe return extra
-EPROBE_DEFER
> thanks,
>
> greg k-h
© 2016 - 2025 Red Hat, Inc.