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

[PATCH RESEND v2 0/2] 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 improperly used for 2 incompatible scenarios as
explained below:

Scenario A: scan drivers for a single device user specify
 - user may care about precise synchronous scanning result, so the
   function can not collapse error codes.

Scenario B: scan drivers for all devices of a bus
 - user may need to scan drivers for a bus's devices as many as
   possible, so the function needs to ignore inconsequential error
   codes for a device in order to continue to scan for next device.

Fixed by implementing bus_rescan_single_device() for scenario A
and correcting the function for scenario B.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Changes in v2:
- Temporarily drop the change related to bus_type's match() return value
- For this 1/2 change, transfer internal -EPROBE_DEFER to user known
  -EAGAIN, correct title, commit message, inline comments.
- For this 2/2 change, Add an extra -EBUSY to ignore, correct title
  commit message, inline comments.
- Link to v1: https://lore.kernel.org/r/20240904-bus_match_unlikely-v1-0-122318285261@quicinc.com

---
Zijun Hu (2):
      driver core: bus: Fix drivers_probe_store() giving user wrong scanning result
      driver core: bus: Correct API bus_rescan_devices() behavior

 drivers/base/bus.c | 78 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 64 insertions(+), 14 deletions(-)
---
base-commit: fea64fa04c31426eae512751e0c5342345c5741c
change-id: 20240830-bus_match_unlikely-abe9334bcfd2

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

[PATCH RESEND v2 1/2] driver core: bus: Fix drivers_probe_store() giving user wrong scanning result

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

drivers_probe_store(), as store() of bus attribute drivers_probe, may give
user wrong scanning result as explained below when user scans drivers for
a single device synchronously via the attribute:

- It wrongly collapses bus_rescan_devices_helper()'s various error
  return values as -EINVAL, that is not expected since user may want
  precise scanning result.

- It wrongly regards bus_rescan_devices_helper()'s return value 0 as
  success since the following 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 all its drivers.

Fixed by giving user precise scanning result or right prompt via
static bus_rescan_single_device() implemented for scanning drivers
for a single device.

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

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 657c93c38b0d..4b5958c5ee7d 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 rescans 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);
 
@@ -310,13 +328,33 @@ static ssize_t drivers_probe_store(const struct bus_type *bus,
 				   const char *buf, size_t count)
 {
 	struct device *dev;
-	int err = -EINVAL;
+	int err;
+	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) {
+		/*
+		 * Give user known -EAGAIN in case of internal
+		 * -EPROBE_DEFER, and propagate error code upwards
+		 * as precise as possible.
+		 */
+		err = res == -EPROBE_DEFER ? -EAGAIN : res;
+	} else if (res > 0) {
 		err = count;
+	} else {
+		/*
+		 * Which error code to return for such synchronous
+		 * probing result ?
+		 */
+		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 RESEND v2 2/2] 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 will really stop scanning for
remaining devices even if a device encounters inconsequential errors
such as -EPROBE_DEFER, -ENODEV and -EBUSY, fixed by ignoring such
inconsequential errors and continue to scan drivers for next device.

By the way, in order to eliminate risk:
 - the API's return value is not changed by recording the first
   error code during scanning which is returned.
 - API device_reprobe()'s existing logic is not changed as well.

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

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 4b5958c5ee7d..fa68acd55bf8 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
  *
@@ -797,15 +794,23 @@ 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 inconsequential errors and continue to
+	 * scan drivers for next device.
+	 */
+	if (ret == -EPROBE_DEFER || ret == -ENODEV || ret == -EBUSY)
+		return 0;
+
+	return ret;
 }
 
 /**
@@ -818,7 +823,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);
 
@@ -833,9 +841,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 RESEND v2 0/2] driver core: bus: Fix issues related to bus_rescan_devices_helper()

[Greg Kroah-Hartman]

On Tue, Oct 22, 2024 at 07:18:00PM +0800, Zijun Hu wrote:
> This patch series is to fix issues related to bus_rescan_devices_helper().
> 
> The function is improperly used for 2 incompatible scenarios as
> explained below:
> 
> Scenario A: scan drivers for a single device user specify
>  - user may care about precise synchronous scanning result, so the
>    function can not collapse error codes.

I do not understand this, what is wrong that this is fixing?

> Scenario B: scan drivers for all devices of a bus
>  - user may need to scan drivers for a bus's devices as many as
>    possible, so the function needs to ignore inconsequential error
>    codes for a device in order to continue to scan for next device.

How often is that needed?  And why can't that still work with the
existing code?

thanks,

greg k-h

Re: [PATCH RESEND v2 0/2] driver core: bus: Fix issues related to bus_rescan_devices_helper()

[Zijun Hu]

On 2024/11/12 19:48, Greg Kroah-Hartman wrote:
> On Tue, Oct 22, 2024 at 07:18:00PM +0800, Zijun Hu wrote:
>> This patch series is to fix issues related to bus_rescan_devices_helper().
>>
>> The function is improperly used for 2 incompatible scenarios as
>> explained below:
>>
>> Scenario A: scan drivers for a single device user specify
>>  - user may care about precise synchronous scanning result, so the
>>    function can not collapse error codes.
> 
> I do not understand this, what is wrong that this is fixing?
> 

for scanning drivers for single device, it gives user wrong scanning
result(success or failure).

patch 1/2 is a concrete example.

>> Scenario B: scan drivers for all devices of a bus
>>  - user may need to scan drivers for a bus's devices as many as
>>    possible, so the function needs to ignore inconsequential error
>>    codes for a device in order to continue to scan for next device.
> 
> How often is that needed?  And why can't that still work with the
> existing code?
>

1) API bus_rescan_devices() invokes it and can't achieve its design
purpose due to error described above.

2) as shown by recent mainlined commit, usage of API
bus_rescan_devices() have been dropped due to some bugs.
Commit: 3d6ebf16438d ("cxl/port: Fix cxl_bus_rescan() vs
bus_rescan_devices()")

3) there are only 2 usages for the API now.
// does not do what the comments say
https://elixir.bootlin.com/linux/v6.11/source/drivers/pcmcia/ds.c#L725
// do multi repeated iterating, can be simplified if fix the API bugs
https://elixir.bootlin.com/linux/v6.11/source/drivers/hid/hid-core.c#L2967

4) i have more reply in below link about the API's bugs.
https://lore.kernel.org/all/20240913-bus_match_unlikely-v2-2-5c0c3bfda2f6@quicinc.com/

> thanks,
> 
> greg k-h

Re: [PATCH RESEND v2 0/2] driver core: bus: Fix issues related to bus_rescan_devices_helper()

[Zijun Hu]

On 2024/11/12 19:48, Greg Kroah-Hartman wrote:
> On Tue, Oct 22, 2024 at 07:18:00PM +0800, Zijun Hu wrote:
>> This patch series is to fix issues related to bus_rescan_devices_helper().
>>
>> The function is improperly used for 2 incompatible scenarios as
>> explained below:
>>
>> Scenario A: scan drivers for a single device user specify
>>  - user may care about precise synchronous scanning result, so the
>>    function can not collapse error codes.
> 
> I do not understand this, what is wrong that this is fixing?
> 

for scanning drivers for single device, it gives user wrong scanning
result(success or failure).

patch 1/2 is a concrete example.

>> Scenario B: scan drivers for all devices of a bus
>>  - user may need to scan drivers for a bus's devices as many as
>>    possible, so the function needs to ignore inconsequential error
>>    codes for a device in order to continue to scan for next device.
> 
> How often is that needed?  And why can't that still work with the
> existing code?
>

1) API bus_rescan_devices() invokes it and can't achieve its design
purpose due to error described above.

2) as shown by recent mainlined commit, usage of API
bus_rescan_devices() have been dropped due to some bugs.
Commit: 3d6ebf16438d ("cxl/port: Fix cxl_bus_rescan() vs
bus_rescan_devices()")

3) there are only 2 usages for the API now.
// does not do what the comments say
https://elixir.bootlin.com/linux/v6.11/source/drivers/pcmcia/ds.c#L725
// do multi repeated iterating, can be simplified if fix the API bugs
https://elixir.bootlin.com/linux/v6.11/source/drivers/hid/hid-core.c#L2967

4) i have more reply in below link about the API's bugs.
https://lore.kernel.org/all/20240913-bus_match_unlikely-v2-2-5c0c3bfda2f6@quicinc.com/

we may go to patch 2/2 to continue discussion.

> thanks,
> 
> greg k-h