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

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

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

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

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