[PATCH 0/3] driver core: bus: Fix issues related to bus_rescan_devices_helper()

[PATCH 0/3] driver core: bus: Fix issues related to bus_rescan_devices_helper()

[Zijun Hu]

This patch series is to fix issues related to bus_rescan_devices_helper().

The function is currently used to scan drivers for both single and
all devices, but its return value can not cover expectations for both
scenarios as explained below:

for single device, user may care about precise scanning result, so
should not collapse error codes.

for all devices, user may want to scan drivers for devices as many as
possible, so need to ignore inconsequential error codes for a device to
continue to scan drivers for remaining devices.

Fixed by implementing bus_rescan_single_device() for single device and
correcting bus_rescan_devices_helper() for all devices.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Zijun Hu (3):
      driver core: Mark impossible return values of bus_type's match() with unlikely()
      driver core: bus: Give error prompt for storing bus attribute drivers_probe failure
      driver core: bus: Correct API bus_rescan_devices() behavior

 drivers/base/bus.c         | 64 ++++++++++++++++++++++++++++++++++++----------
 drivers/base/dd.c          | 16 +++++++++---
 include/linux/device/bus.h |  9 +++----
 3 files changed, 67 insertions(+), 22 deletions(-)
---
base-commit: 888f67e621dda5c2804a696524e28d0ca4cf0a80
change-id: 20240830-bus_match_unlikely-abe9334bcfd2

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

[PATCH 1/3] driver core: Mark impossible return values of bus_type's match() with unlikely()

[Zijun Hu]

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.1

Re: [PATCH 1/3] driver core: Mark impossible return values of bus_type's match() with unlikely()

[Greg Kroah-Hartman]

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.

> 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

Re: [PATCH 1/3] driver core: Mark impossible return values of bus_type's match() with unlikely()

[Zijun Hu]

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

[PATCH 2/3] driver core: bus: Give error prompt for storing bus attribute drivers_probe failure

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

drivers_probe_store() regards bus_rescan_devices_helper()'s returned
value 0 as success when scan drivers for a single device user specify
that is wrong since the following 3 failed cases also return 0:

(1) the device is dead
(2) bus has no driver which match() the device
(3) bus fails to probe() the device with any its driver

Solved by giving error prompt via dev_err() for above failed cases.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/base/bus.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index abf090ace833..6b5ea82a44c1 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -40,6 +40,24 @@ static struct kset *bus_kset;
 	struct driver_attribute driver_attr_##_name =		\
 		__ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
 
+/*
+ *  Bus scans drivers for a single device, and derives from
+ *  bus_rescan_devices_helper(), but returns scanning result
+ *  as precise as possible.
+ */
+static int __must_check bus_rescan_single_device(struct device *dev)
+{
+	int ret;
+
+	if (dev->parent && dev->bus->need_parent_lock)
+		device_lock(dev->parent);
+	ret = device_attach(dev);
+	if (dev->parent && dev->bus->need_parent_lock)
+		device_unlock(dev->parent);
+
+	return ret;
+}
+
 static int __must_check bus_rescan_devices_helper(struct device *dev,
 						void *data);
 
@@ -311,12 +329,25 @@ static ssize_t drivers_probe_store(const struct bus_type *bus,
 {
 	struct device *dev;
 	int err = -EINVAL;
+	int res;
 
 	dev = bus_find_device_by_name(bus, NULL, buf);
 	if (!dev)
 		return -ENODEV;
-	if (bus_rescan_devices_helper(dev, NULL) == 0)
+
+	res = bus_rescan_single_device(dev);
+	if (res < 0) {
+		/* Propagate error code upwards as precise as possible */
+		err = res;
+	} else if (res > 0) {
 		err = count;
+	} else {
+		/* Which error code to return for this case ? */
+		dev_err(dev, "device '%s' fails to attach a driver\n",
+			dev_name(dev));
+		err = count;
+	}
+
 	put_device(dev);
 	return err;
 }

-- 
2.34.1

[PATCH 3/3] driver core: bus: Correct API bus_rescan_devices() behavior

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

API bus_rescan_devices() should ideally scan drivers for a bus's devices
as many as possible, but it really stops scanning for remaining devices
even if a device encounters inconsequential errors such as -EPROBE_DEFER
and -ENODEV, fixed by ignoring such inconsequential errors during scanning.

By the way, Neither the API's return value nor device_reprobe()'s existing
logic are changed.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/base/bus.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 6b5ea82a44c1..31d9d5d08934 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -58,9 +58,6 @@ static int __must_check bus_rescan_single_device(struct device *dev)
 	return ret;
 }
 
-static int __must_check bus_rescan_devices_helper(struct device *dev,
-						void *data);
-
 /**
  * bus_to_subsys - Turn a struct bus_type into a struct subsys_private
  *
@@ -790,15 +787,18 @@ static int __must_check bus_rescan_devices_helper(struct device *dev,
 						  void *data)
 {
 	int ret = 0;
+	int *first_error = data;
 
-	if (!dev->driver) {
-		if (dev->parent && dev->bus->need_parent_lock)
-			device_lock(dev->parent);
-		ret = device_attach(dev);
-		if (dev->parent && dev->bus->need_parent_lock)
-			device_unlock(dev->parent);
-	}
-	return ret < 0 ? ret : 0;
+	ret = bus_rescan_single_device(dev);
+
+	if (ret >= 0)
+		return 0;
+	if (!*first_error)
+		*first_error = ret;
+	/* Ignore these errors to scan drivers for next device */
+	if (ret == -EPROBE_DEFER || ret == -ENODEV)
+		return 0;
+	return ret;
 }
 
 /**
@@ -811,7 +811,10 @@ static int __must_check bus_rescan_devices_helper(struct device *dev,
  */
 int bus_rescan_devices(const struct bus_type *bus)
 {
-	return bus_for_each_dev(bus, NULL, NULL, bus_rescan_devices_helper);
+	int err = 0;
+
+	bus_for_each_dev(bus, NULL, &err, bus_rescan_devices_helper);
+	return err;
 }
 EXPORT_SYMBOL_GPL(bus_rescan_devices);
 
@@ -826,9 +829,13 @@ EXPORT_SYMBOL_GPL(bus_rescan_devices);
  */
 int device_reprobe(struct device *dev)
 {
+	int ret;
+
 	if (dev->driver)
 		device_driver_detach(dev);
-	return bus_rescan_devices_helper(dev, NULL);
+
+	ret = bus_rescan_single_device(dev);
+	return ret < 0 ? ret : 0;
 }
 EXPORT_SYMBOL_GPL(device_reprobe);
 

-- 
2.34.1

Re: [PATCH 3/3] driver core: bus: Correct API bus_rescan_devices() behavior

[Greg Kroah-Hartman]

On Wed, Sep 04, 2024 at 08:56:44PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> API bus_rescan_devices() should ideally scan drivers for a bus's devices
> as many as possible, but it really stops scanning for remaining devices
> even if a device encounters inconsequential errors such as -EPROBE_DEFER

-EPROBE_DEFER should not be an issue for scanning the bus, that's only
for probe errors, so who is returning that mess today?  Let's fix that
up please.

thanks,

greg k-h

Re: [PATCH 3/3] driver core: bus: Correct API bus_rescan_devices() behavior

[Zijun Hu]

On 2024/9/4 21:54, Greg Kroah-Hartman wrote:
> On Wed, Sep 04, 2024 at 08:56:44PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> API bus_rescan_devices() should ideally scan drivers for a bus's devices
>> as many as possible, but it really stops scanning for remaining devices
>> even if a device encounters inconsequential errors such as -EPROBE_DEFER
> 
> -EPROBE_DEFER should not be an issue for scanning the bus, that's only
> for probe errors, so who is returning that mess today?  Let's fix that
> up please.
> 

drivers/amba/bus.c:
struct bus_type amba_bustype = {
...
        .match          = amba_match,
...
};
amba_match() may return -EPROBE_DEFER.

you maybe also look at below AMBA bus related commit.
Commit: 656b8035b0ee ("ARM: 8524/1: driver cohandle -EPROBE_DEFER from
bus_type.match()"

is it proper to change bus_type match()'s return value type to bool type
back if this issue is fixed?

> thanks,
> 
> greg k-h

Re: [PATCH 3/3] driver core: bus: Correct API bus_rescan_devices() behavior

[Greg Kroah-Hartman]

On Wed, Sep 04, 2024 at 10:26:39PM +0800, Zijun Hu wrote:
> On 2024/9/4 21:54, Greg Kroah-Hartman wrote:
> > On Wed, Sep 04, 2024 at 08:56:44PM +0800, Zijun Hu wrote:
> >> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>
> >> API bus_rescan_devices() should ideally scan drivers for a bus's devices
> >> as many as possible, but it really stops scanning for remaining devices
> >> even if a device encounters inconsequential errors such as -EPROBE_DEFER
> > 
> > -EPROBE_DEFER should not be an issue for scanning the bus, that's only
> > for probe errors, so who is returning that mess today?  Let's fix that
> > up please.
> > 
> 
> drivers/amba/bus.c:
> struct bus_type amba_bustype = {
> ...
>         .match          = amba_match,
> ...
> };
> amba_match() may return -EPROBE_DEFER.

Why?  That feels wrong.

> you maybe also look at below AMBA bus related commit.
> Commit: 656b8035b0ee ("ARM: 8524/1: driver cohandle -EPROBE_DEFER from
> bus_type.match()"

Ah, ick, clocks.  What a mess.

I don't think we need this anymore with the genric device link stuff
anymore, but I'm not quite sure.

> is it proper to change bus_type match()'s return value type to bool type
> back if this issue is fixed?

Yes, I would like to.  Care to look into it, odds are you can test this
better than I can :)

thanks,

greg k-h

Re: [PATCH 3/3] driver core: bus: Correct API bus_rescan_devices() behavior

[Zijun Hu]

On 2024/9/4 22:44, Greg Kroah-Hartman wrote:
> On Wed, Sep 04, 2024 at 10:26:39PM +0800, Zijun Hu wrote:
>> On 2024/9/4 21:54, Greg Kroah-Hartman wrote:
>>> On Wed, Sep 04, 2024 at 08:56:44PM +0800, Zijun Hu wrote:
>>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>
>>>> API bus_rescan_devices() should ideally scan drivers for a bus's devices
>>>> as many as possible, but it really stops scanning for remaining devices
>>>> even if a device encounters inconsequential errors such as -EPROBE_DEFER
>>>
>>> -EPROBE_DEFER should not be an issue for scanning the bus, that's only
>>> for probe errors, so who is returning that mess today?  Let's fix that
>>> up please.
>>>
>>
>> drivers/amba/bus.c:
>> struct bus_type amba_bustype = {
>> ...
>>         .match          = amba_match,
>> ...
>> };
>> amba_match() may return -EPROBE_DEFER.
> 
> Why?  That feels wrong.
> 

i have below understanding after reading drivers/amba/bus.c.

amba_device_add() needs to read ID from device to add device.
but resources such as clocks may be not ready to read ID at that time

so it registers a empty amba_driver @amba_proxy_drv which will trigger
reading ID within bus_type match() when add a device by amba_device_add()

bus_type match() will defer device probe when resources is not ready
during match().

at deferral probe() time, match() is able to read ID since resources is
ready at that time, and it notify device adding by uevent to user.

user loads matched driver for the device which has ID read out.

>> you maybe also look at below AMBA bus related commit.
>> Commit: 656b8035b0ee ("ARM: 8524/1: driver cohandle -EPROBE_DEFER from
>> bus_type.match()"
> 
> Ah, ick, clocks.  What a mess.
> 
> I don't think we need this anymore with the genric device link stuff
> anymore, but I'm not quite sure.
> 
yes, i agree with you.

>> is it proper to change bus_type match()'s return value type to bool type
>> back if this issue is fixed?
> 
> Yes, I would like to.  Care to look into it, odds are you can test this
> better than I can :)
> 
actually, i also does not have AMBA test environment. let me send a AMBA
patch to start a topic to check if AMBA maintainers have good ideas to
help us at spare time.

> thanks,
> 
> greg k-h