driver core: don't hold dev's parent lock when using async probe

SOC have internal I/O buses that can't be probed for devices. The devices on the buses can be accessed directly without additinal configuration required. This type of bus is represented as "simple-bus". In some platforms, we name "soc" with "simple-bus" attribute and many devices are hooked under and desribe them in DT (device tree).

In commit 'bf74ad5bc417 introduce ("[PATCH] Hold the device's parent's lock during probe and remove")' to solve USB subsystem lock sequence since usb device's characteristic. Thus "soc" needs to be locked whenever a device and driver's probing happen under "soc" bus. During this period, an async driver tries to probe a device which is under the "soc" bus would be blocked until previous driver finish the probing and release "soc" lock. And the next probing under the "soc" bus need to wait for async finish. Because of that, driver's async probe for init time improvement will be shadowed.

Since many devices don't have USB devices' characteristic, they actually don't need parent's lock. However, in order to control the risk and minimize the impact, we don't request parent's lock only when a driver requests async probe.

Async probe could have more benefit after we have this patch.

Signed-off-by martin_liu <[email protected]>

---
This RFC is asked to get some feedback since it involed driver core and USB subsystem. I'm not familiar with USB subsystem and not sure if we still need 'bf74ad5bc417 ("[PATCH] Hold the device's parent's lock during probe and remove")' since it has been there over 10 years. If we still need it and hard to fix it , the simple way is to find a place not to allow USB subsystem drivers to have async probe capability. Any suggestion is welcome.

Quoted 5 lines · martin_liu

not sure if we still need 'bf74ad5bc417 ("[PATCH] Hold the device's parent's lock during probe and remove")' since it has been there over 10 years. If we still need it and hard to fix it , the simple way is to find a place not to allow USB subsystem drivers to have async probe capability. Any suggestion is welcome.

I don't think the "allows_async_probing" attribute is the best way to attack this. Some other approach, like a special-purpose flag, might be better.

Yes, USB still needs to have parent's locks held during probing. Here's the reason. A USB device can have multiple interfaces, each bound to its own driver. A driver may sometimes need to issue a reset, but in USB there's no way to reset a single interface. Only the entire device can be reset, and of course this affects all the interfaces. Therefore a driver needs to acquire the device lock before it can issue a reset.

The problem is that the driver's thread may already hold the device lock. During a normal probe sequence, for example, the interfaces get probed by the hub driver while it owns the device lock. But for probes under other circumstances (for example, if the user writes to the driver's "bind" attribute in sysfs), the device lock might not be held.

A driver cannot tell these two cases apart. The only way to make it work all the time is to have the caller _always_ hold the device lock while the driver is probed (or the removed, for that matter).

Alan Stern

Quoted 30 lines · Alan Stern

Quoted 5 lines · martin_liu

not sure if we still need 'bf74ad5bc417 ("[PATCH] Hold the device's parent's lock during probe and remove")' since it has been there over 10 years. If we still need it and hard to fix it , the simple way is to find a place not to allow USB subsystem drivers to have async probe capability. Any suggestion is welcome.

I don't think the "allows_async_probing" attribute is the best way to attack this. Some other approach, like a special-purpose flag, might be better.

Yes, USB still needs to have parent's locks held during probing. Here's the reason. A USB device can have multiple interfaces, each bound to its own driver. A driver may sometimes need to issue a reset, but in USB there's no way to reset a single interface. Only the entire device can be reset, and of course this affects all the interfaces. Therefore a driver needs to acquire the device lock before it can issue a reset.

The problem is that the driver's thread may already hold the device lock. During a normal probe sequence, for example, the interfaces get probed by the hub driver while it owns the device lock. But for probes under other circumstances (for example, if the user writes to the driver's "bind" attribute in sysfs), the device lock might not be held.

A driver cannot tell these two cases apart. The only way to make it work all the time is to have the caller _always_ hold the device lock while the driver is probed (or the removed, for that matter).

Quoted 1 line

Alan Stern

Thanks for the reply and more detail about the backgroud. I'd like to have a conclusion about it. Please kindly correct me if my understanding is wrong. Regarding to the "special-purpose flag", do you mean we could find a place in USB subsystem to have the flag set (not sure if it's easy to find it). Driver core would be base on the flag to decide if we need to hold the device's parent's lock.

Quoted 31 lines · Martin Liu

Quoted 30 lines · Alan Stern

Quoted 5 lines · martin_liu

not sure if we still need 'bf74ad5bc417 ("[PATCH] Hold the device's parent's lock during probe and remove")' since it has been there over 10 years. If we still need it and hard to fix it , the simple way is to find a place not to allow USB subsystem drivers to have async probe capability. Any suggestion is welcome.

I don't think the "allows_async_probing" attribute is the best way to attack this. Some other approach, like a special-purpose flag, might be better.

Yes, USB still needs to have parent's locks held during probing. Here's the reason. A USB device can have multiple interfaces, each bound to its own driver. A driver may sometimes need to issue a reset, but in USB there's no way to reset a single interface. Only the entire device can be reset, and of course this affects all the interfaces. Therefore a driver needs to acquire the device lock before it can issue a reset.

The problem is that the driver's thread may already hold the device lock. During a normal probe sequence, for example, the interfaces get probed by the hub driver while it owns the device lock. But for probes under other circumstances (for example, if the user writes to the driver's "bind" attribute in sysfs), the device lock might not be held.

A driver cannot tell these two cases apart. The only way to make it work all the time is to have the caller _always_ hold the device lock while the driver is probed (or the removed, for that matter).

Quoted 8 lines

Quoted 1 line

Alan Stern

Thanks for the reply and more detail about the backgroud. I'd like to have a conclusion about it. Please kindly correct me if my understanding is wrong. Regarding to the "special-purpose flag", do you mean we could find a place in USB subsystem to have the flag set (not sure if it's easy to find it). Driver core would be base on the flag to decide if we need to hold the device's parent's lock.

Yes, except that the flag would not be in the USB subsystem. It would be in the device, device_type, or bus_type structure, so that the driver core could access it.

Alan Stern

SOC have internal I/O buses that can't be proved for devices. The devices on the buses can be accessed directly without additinal configuration required. This type of bus is represented as "simple-bus". In some platforms, we name "soc" with "simple-bus" attribute and many devices are hooked under it desribed in DT (device tree).

In commit 'bf74ad5bc417 introduce ("[PATCH] Hold the device's parent's lock during probe and remove")' to solve USB subsystem lock sequence since usb device's characteristic. Thus "soc" needs to be locked whenever a device and driver's probing happen under "soc" bus. During this period, an async driver tries to probe a device which is under the "soc" bus would be blocked until previous driver finish the probing and release "soc" lock. And the next probing under the "soc" bus need to wait for async finish. Because of that, driver's async probe for init time improvement will be shadowed.

Since many devices don't have USB devices' characteristic, they actually don't need parent's lock. Thus, we introduce a lock flag in device struct and driver core would lock the parent lock base on the flag. For usbsystem, we set this flag when its device and driver is matched and to keep original lock behavior in driver core.

Async probe could have more benefit after this patch.

Signed-off-by martin_liu <[email protected]>

Suggested-by Alan Stern <[email protected]>

SOC have internal I/O buses that can't be proved for devices. The devices on the buses can be accessed directly without additinal configuration required. This type of bus is represented as "simple-bus". In some platforms, we name "soc" with "simple-bus" attribute and many devices are hooked under it desribed in DT (device tree).

In commit 'bf74ad5bc417 introduce ("[PATCH] Hold the device's parent's lock during probe and remove")' to solve USB subsystem lock sequence since usb device's characteristic. Thus "soc" needs to be locked whenever a device and driver's probing happen under "soc" bus. During this period, an async driver tries to probe a device which is under the "soc" bus would be blocked until previous driver finish the probing and release "soc" lock. And the next probing under the "soc" bus need to wait for async finish. Because of that, driver's async probe for init time improvement will be shadowed.

Since many devices don't have USB devices' characteristic, they actually don't need parent's lock. Thus, we introduce a lock flag in device struct and driver core would lock the parent lock base on the flag. For usbsystem, we set this flag when its device and driver is matched and to keep original lock behavior in driver core.

Async probe could have more benefit after this patch.

Signed-off-by martin_liu <[email protected]>

Suggested-by Alan Stern <[email protected]>

---
Changes in v2:
- take Alan's suggestion to introudce a flag to guide driver core to hold device parent's lock.

[v1]: https://lkml.org/lkml/2018/5/22/545

Currently, I have the flag set in USB subsystem match function. Since I'm not familar with USB part, need some feedback to know if they cover all the cases that original case driver core protects.

 drivers/base/bus.c          | 16 ++++++++--------
 drivers/base/dd.c           |  8 ++++----
 drivers/usb/common/ulpi.c   | 16 ++++++++++++----
 drivers/usb/core/driver.c   |  9 +++++++--
 drivers/usb/core/usb-acpi.c |  6 +++++-
 drivers/usb/serial/bus.c    |  4 +++-
 include/linux/device.h      |  1 +
 7 files changed, 40 insertions(+), 20 deletions(-)

drivers/base/bus.c +0 -0

index ef6183306b40..18ea94caec02 100644

SOC have internal I/O buses that can't be proved for devices. The devices on the buses can be accessed directly without additinal configuration required. This type of bus is represented as "simple-bus". In some platforms, we name "soc" with "simple-bus" attribute and many devices are hooked under it desribed in DT (device tree).

SOC have internal I/O buses that can't be proved for devices. The

devices on the buses can be accessed directly without additinal configuration required. This type of bus is represented as "simple-bus". In some platforms, we name "soc" with "simple-bus" attribute and many devices are hooked under it desribed in DT (device tree).

In commit 'bf74ad5bc417 introduce ("[PATCH] Hold the device's parent's lock during probe and remove")'

to solve USB subsystem
lock sequence since usb device's characteristic. Thus "soc"

needs to be locked whenever a device and driver's probing happen under "soc" bus. During this period, an async driver tries to probe a device which is under the "soc" bus would be blocked until previous driver finish the probing and release "soc" lock. And the next probing under the "soc" bus need to wait for async finish. Because of that, driver's async probe for init time improvement will be shadowed.

Since many devices don't have USB devices' characteristic, they actually don't need parent's lock. Thus, we introduce a lock flag in bus_type struct and driver core would lock the parent lock base on the flag. For usbsystem, we set this flag in usb relatvie

+ bool need_parent_lock;

SOC have internal I/O buses that can't be proved for devices. The devices on the buses can be accessed directly without additinal configuration required. This type of bus is represented as "simple-bus". In some platforms, we name "soc" with "simple-bus" attribute and many devices are hooked under it desribed in DT (device tree).

In commit 'bf74ad5bc417 introduce ("[PATCH] Hold the device's parent's lock during probe and remove")' to solve USB subsystem lock sequence since usb device's characteristic. Thus "soc" needs to be locked whenever a device and driver's probing happen under "soc" bus. During this period, an async driver tries to probe a device which is under the "soc" bus would be blocked until previous driver finish the probing and release "soc" lock. And the next probing under the "soc" bus need to wait for async finish. Because of that, driver's async probe for init time improvement will be shadowed.

Since many devices don't have USB devices' characteristic, they actually don't need parent's lock. Thus, we introduce a lock flag in bus_type struct and driver core would lock the parent lock base on the flag. For usbsystem, we set this flag in usb relatvie bus_type struct to keep original lock behavior in driver core.

Async probe could have more benefit after this patch.

Signed-off-by Martin Liu <[email protected]>

---
Changes in v3:
-move lock flag to bus_type struct and set the flag in usb relative bus_type struct to keep original lock behavior. -fix sign name.

[v2]: https://lkml.org/lkml/2018/5/29/108
[v1]: https://lkml.org/lkml/2018/5/22/545

Currently, I have the flag set in USB relatvie bus_type struct. Since I'm not familar with USB part, need some feedback to know if they cover all the cases that original case driver core protects. Thanks.

drivers/usb/common/ulpi.c +1 -0

index 9a2ab6751a23..073954a1a1c5 100644
@@ -94,6 +94,7 @@ static struct bus_type ulpi_bus = {
 	.uevent = ulpi_uevent,
 	.probe = ulpi_probe,
 	.remove = ulpi_remove,
+	.need_parent_lock = 1,
 };
 
 /* -------------------------------------------------------------------------- */

drivers/usb/core/driver.c +1 -0

index 9792cedfc351..209ee5d8a92d 100644
@@ -1922,4 +1922,5 @@ struct bus_type usb_bus_type = {
 	.name =		"usb",
 	.match =	usb_device_match,
 	.uevent =	usb_uevent,
+	.need_parent_lock =	1,
 };

drivers/usb/serial/bus.c +1 -0

index 9e265eb92611..55b2636b2804 100644
@@ -166,6 +166,7 @@ struct bus_type usb_serial_bus_type = {
 	.probe =	usb_serial_device_probe,
 	.remove =	usb_serial_device_remove,
 	.drv_groups = 	usb_serial_drv_groups,
+	.need_parent_lock =	1,
 };
 
 int usb_serial_bus_register(struct usb_serial_driver *driver)

include/linux/device.h +0 -0

index 477956990f5e..019b193aeb24 100644

SoC have internal I/O buses that can't be proved for devices. The devices on the buses can be accessed directly without additinal configuration required. This type of bus is represented as "simple-bus". In some platforms, we name "soc" with "simple-bus" attribute and many devices are hooked under it described in DT (device tree).

In commit bf74ad5bc417 ("Hold the device's parent's lock during probe and remove") to solve USB subsystem lock sequence since USB device's characteristic. Thus "soc" needs to be locked whenever a device and driver's probing happen under "soc" bus. During this period, an async driver tries to probe a device which is under the "soc" bus would be blocked until previous driver finish the probing and release "soc" lock. And the next probing under the "soc" bus need to wait for async finish. Because of that, driver's async probe for init time improvement will be shadowed.

Since many devices don't have USB devices' characteristic, they actually don't need parent's lock. Thus, we introduce a lock flag in bus_type struct and driver core would lock the parent lock base on the flag. For USB, we set this flag in usb_bus_type to keep original lock behavior in driver core.

Async probe could have more benefit after this patch.

Signed-off-by Martin Liu <[email protected]>

---
Changes in v4:
-fix comment and wording.
-follow the suggestion.

[v3]: https://lkml.org/lkml/2018/5/29/876
[v2]: https://lkml.org/lkml/2018/5/29/108
[v1]: https://lkml.org/lkml/2018/5/22/545

 drivers/base/bus.c        | 16 ++++++++--------
 drivers/base/dd.c         |  8 ++++----
 drivers/usb/core/driver.c |  1 +
 include/linux/device.h    |  3 +++
 4 files changed, 16 insertions(+), 12 deletions(-)

drivers/base/bus.c +8 -8

index ef6183306b40..8bfd27ec73d6 100644
@@ -184,10 +184,10 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
 
 	dev = bus_find_device_by_name(bus, NULL, buf);
 	if (dev && dev->driver == drv) {
-		if (dev->parent)	/* Needed for USB */
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_lock(dev->parent);
 		device_release_driver(dev);
-		if (dev->parent)
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_unlock(dev->parent);
 		err = count;
 	}

@@ -211,12 +211,12 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
 
 	dev = bus_find_device_by_name(bus, NULL, buf);
 	if (dev && dev->driver == NULL && driver_match_device(drv, dev)) {
-		if (dev->parent)	/* Needed for USB */
+		if (dev->parent && bus->need_parent_lock)
 			device_lock(dev->parent);
 		device_lock(dev);
 		err = driver_probe_device(drv, dev);
 		device_unlock(dev);
-		if (dev->parent)
+		if (dev->parent && bus->need_parent_lock)
 			device_unlock(dev->parent);
 
 		if (err > 0) {

@@ -735,10 +735,10 @@ static int __must_check bus_rescan_devices_helper(struct device *dev,
 	int ret = 0;
 
 	if (!dev->driver) {
-		if (dev->parent)	/* Needed for USB */
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_lock(dev->parent);
 		ret = device_attach(dev);
-		if (dev->parent)
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_unlock(dev->parent);
 	}
 	return ret < 0 ? ret : 0;

@@ -770,10 +770,10 @@ EXPORT_SYMBOL_GPL(bus_rescan_devices);
 int device_reprobe(struct device *dev)
 {
 	if (dev->driver) {
-		if (dev->parent)        /* Needed for USB */
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_lock(dev->parent);
 		device_release_driver(dev);
-		if (dev->parent)
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_unlock(dev->parent);
 	}
 	return bus_rescan_devices_helper(dev, NULL);

drivers/base/dd.c +4 -4

index c9f54089429b..7c09f73b96f3 100644
@@ -817,13 +817,13 @@ static int __driver_attach(struct device *dev, void *data)
 		return ret;
 	} /* ret > 0 means positive match */
 
-	if (dev->parent)	/* Needed for USB */
+	if (dev->parent && dev->bus->need_parent_lock)
 		device_lock(dev->parent);
 	device_lock(dev);
 	if (!dev->driver)
 		driver_probe_device(drv, dev);
 	device_unlock(dev);
-	if (dev->parent)
+	if (dev->parent && dev->bus->need_parent_lock)
 		device_unlock(dev->parent);
 
 	return 0;

@@ -919,7 +919,7 @@ void device_release_driver_internal(struct device *dev,
 				    struct device_driver *drv,
 				    struct device *parent)
 {
-	if (parent)
+	if (parent && dev->bus->need_parent_lock)
 		device_lock(parent);
 
 	device_lock(dev);

@@ -927,7 +927,7 @@ void device_release_driver_internal(struct device *dev,
 		__device_release_driver(dev, parent);
 
 	device_unlock(dev);
-	if (parent)
+	if (parent && dev->bus->need_parent_lock)
 		device_unlock(parent);
 }
 

drivers/usb/core/driver.c +1 -0

index 9792cedfc351..e76e95f62f76 100644
@@ -1922,4 +1922,5 @@ struct bus_type usb_bus_type = {
 	.name =		"usb",
 	.match =	usb_device_match,
 	.uevent =	usb_uevent,
+	.need_parent_lock =	true,
 };

include/linux/device.h +2 -0

index 477956990f5e..beca424395dd 100644
@@ -98,6 +98,8 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
  * @lock_key:	Lock class key for use by the lock validator
  * @force_dma:	Assume devices on this bus should be set up by dma_configure()
  * 		even if DMA capability is not explicitly described by firmware.
+ * @need_parent_lock:	When probing or removing a device on this bus, the
+ *			device core should lock the device's parent.
  *
  * A bus is a channel between the processor and one or more devices. For the
  * purposes of the device model, all devices are connected via a bus, even if

Quoted 168 lines · Martin Liu

SoC have internal I/O buses that can't be proved for devices. The devices on the buses can be accessed directly without additinal configuration required. This type of bus is represented as "simple-bus". In some platforms, we name "soc" with "simple-bus" attribute and many devices are hooked under it described in DT (device tree).

In commit bf74ad5bc417 ("Hold the device's parent's lock during probe and remove") to solve USB subsystem lock sequence since USB device's characteristic. Thus "soc" needs to be locked whenever a device and driver's probing happen under "soc" bus. During this period, an async driver tries to probe a device which is under the "soc" bus would be blocked until previous driver finish the probing and release "soc" lock. And the next probing under the "soc" bus need to wait for async finish. Because of that, driver's async probe for init time improvement will be shadowed.

Since many devices don't have USB devices' characteristic, they actually don't need parent's lock. Thus, we introduce a lock flag in bus_type struct and driver core would lock the parent lock base on the flag. For USB, we set this flag in usb_bus_type to keep original lock behavior in driver core.

Async probe could have more benefit after this patch.

Signed-off-by Martin Liu <[email protected]>

---
Changes in v4:
-fix comment and wording.
-follow the suggestion.

[v3]: https://lkml.org/lkml/2018/5/29/876
[v2]: https://lkml.org/lkml/2018/5/29/108
[v1]: https://lkml.org/lkml/2018/5/22/545

 drivers/base/bus.c        | 16 ++++++++--------
 drivers/base/dd.c         |  8 ++++----
 drivers/usb/core/driver.c |  1 +
 include/linux/device.h    |  3 +++
 4 files changed, 16 insertions(+), 12 deletions(-)

drivers/base/bus.c +8 -8

index ef6183306b40..8bfd27ec73d6 100644
@@ -184,10 +184,10 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
 
 	dev = bus_find_device_by_name(bus, NULL, buf);
 	if (dev && dev->driver == drv) {
-		if (dev->parent)	/* Needed for USB */
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_lock(dev->parent);
 		device_release_driver(dev);
-		if (dev->parent)
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_unlock(dev->parent);
 		err = count;
 	}

@@ -211,12 +211,12 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
 
 	dev = bus_find_device_by_name(bus, NULL, buf);
 	if (dev && dev->driver == NULL && driver_match_device(drv, dev)) {
-		if (dev->parent)	/* Needed for USB */
+		if (dev->parent && bus->need_parent_lock)
 			device_lock(dev->parent);
 		device_lock(dev);
 		err = driver_probe_device(drv, dev);
 		device_unlock(dev);
-		if (dev->parent)
+		if (dev->parent && bus->need_parent_lock)
 			device_unlock(dev->parent);
 
 		if (err > 0) {

@@ -735,10 +735,10 @@ static int __must_check bus_rescan_devices_helper(struct device *dev,
 	int ret = 0;
 
 	if (!dev->driver) {
-		if (dev->parent)	/* Needed for USB */
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_lock(dev->parent);
 		ret = device_attach(dev);
-		if (dev->parent)
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_unlock(dev->parent);
 	}
 	return ret < 0 ? ret : 0;

@@ -770,10 +770,10 @@ EXPORT_SYMBOL_GPL(bus_rescan_devices);
 int device_reprobe(struct device *dev)
 {
 	if (dev->driver) {
-		if (dev->parent)        /* Needed for USB */
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_lock(dev->parent);
 		device_release_driver(dev);
-		if (dev->parent)
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_unlock(dev->parent);
 	}
 	return bus_rescan_devices_helper(dev, NULL);

drivers/base/dd.c +4 -4

index c9f54089429b..7c09f73b96f3 100644
@@ -817,13 +817,13 @@ static int __driver_attach(struct device *dev, void *data)
 		return ret;
 	} /* ret > 0 means positive match */
 
-	if (dev->parent)	/* Needed for USB */
+	if (dev->parent && dev->bus->need_parent_lock)
 		device_lock(dev->parent);
 	device_lock(dev);
 	if (!dev->driver)
 		driver_probe_device(drv, dev);
 	device_unlock(dev);
-	if (dev->parent)
+	if (dev->parent && dev->bus->need_parent_lock)
 		device_unlock(dev->parent);
 
 	return 0;

@@ -919,7 +919,7 @@ void device_release_driver_internal(struct device *dev,
 				    struct device_driver *drv,
 				    struct device *parent)
 {
-	if (parent)
+	if (parent && dev->bus->need_parent_lock)
 		device_lock(parent);
 
 	device_lock(dev);

@@ -927,7 +927,7 @@ void device_release_driver_internal(struct device *dev,
 		__device_release_driver(dev, parent);
 
 	device_unlock(dev);
-	if (parent)
+	if (parent && dev->bus->need_parent_lock)
 		device_unlock(parent);
 }
 

drivers/usb/core/driver.c +1 -0

index 9792cedfc351..e76e95f62f76 100644
@@ -1922,4 +1922,5 @@ struct bus_type usb_bus_type = {
 	.name =		"usb",
 	.match =	usb_device_match,
 	.uevent =	usb_uevent,
+	.need_parent_lock =	true,
 };

include/linux/device.h +3 -0

index 477956990f5e..beca424395dd 100644
@@ -98,6 +98,8 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
  * @lock_key:	Lock class key for use by the lock validator
  * @force_dma:	Assume devices on this bus should be set up by dma_configure()
  * 		even if DMA capability is not explicitly described by firmware.
+ * @need_parent_lock:	When probing or removing a device on this bus, the
+ *			device core should lock the device's parent.
  *
  * A bus is a channel between the processor and one or more devices. For the
  * purposes of the device model, all devices are connected via a bus, even if

@@ -138,6 +140,7 @@ struct bus_type {
 	struct lock_class_key lock_key;
 
 	bool force_dma;
+	bool need_parent_lock;
 };
 

include/linux/device.h struct bus_type { · line 146

Quoted 1 line

include/linux/device.h +0 -0

 extern int __must_check bus_register(struct bus_type *bus);

This looks okay to me.

Acked-by Alan Stern <[email protected]>

Greg, any more comments?

Quoted 174 lines · Alan Stern

Quoted 162 lines · Martin Liu

SoC have internal I/O buses that can't be proved for devices. The devices on the buses can be accessed directly without additinal configuration required. This type of bus is represented as "simple-bus". In some platforms, we name "soc" with "simple-bus" attribute and many devices are hooked under it described in DT (device tree).

In commit bf74ad5bc417 ("Hold the device's parent's lock during probe and remove") to solve USB subsystem lock sequence since USB device's characteristic. Thus "soc" needs to be locked whenever a device and driver's probing happen under "soc" bus. During this period, an async driver tries to probe a device which is under the "soc" bus would be blocked until previous driver finish the probing and release "soc" lock. And the next probing under the "soc" bus need to wait for async finish. Because of that, driver's async probe for init time improvement will be shadowed.

Since many devices don't have USB devices' characteristic, they actually don't need parent's lock. Thus, we introduce a lock flag in bus_type struct and driver core would lock the parent lock base on the flag. For USB, we set this flag in usb_bus_type to keep original lock behavior in driver core.

Async probe could have more benefit after this patch.

Signed-off-by Martin Liu <[email protected]>

---
Changes in v4:
-fix comment and wording.
-follow the suggestion.

[v3]: https://lkml.org/lkml/2018/5/29/876
[v2]: https://lkml.org/lkml/2018/5/29/108
[v1]: https://lkml.org/lkml/2018/5/22/545

 drivers/base/bus.c        | 16 ++++++++--------
 drivers/base/dd.c         |  8 ++++----
 drivers/usb/core/driver.c |  1 +
 include/linux/device.h    |  3 +++
 4 files changed, 16 insertions(+), 12 deletions(-)

drivers/base/bus.c +8 -8

index ef6183306b40..8bfd27ec73d6 100644
@@ -184,10 +184,10 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
 
 	dev = bus_find_device_by_name(bus, NULL, buf);
 	if (dev && dev->driver == drv) {
-		if (dev->parent)	/* Needed for USB */
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_lock(dev->parent);
 		device_release_driver(dev);
-		if (dev->parent)
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_unlock(dev->parent);
 		err = count;
 	}

@@ -211,12 +211,12 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
 
 	dev = bus_find_device_by_name(bus, NULL, buf);
 	if (dev && dev->driver == NULL && driver_match_device(drv, dev)) {
-		if (dev->parent)	/* Needed for USB */
+		if (dev->parent && bus->need_parent_lock)
 			device_lock(dev->parent);
 		device_lock(dev);
 		err = driver_probe_device(drv, dev);
 		device_unlock(dev);
-		if (dev->parent)
+		if (dev->parent && bus->need_parent_lock)
 			device_unlock(dev->parent);
 
 		if (err > 0) {

@@ -735,10 +735,10 @@ static int __must_check bus_rescan_devices_helper(struct device *dev,
 	int ret = 0;
 
 	if (!dev->driver) {
-		if (dev->parent)	/* Needed for USB */
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_lock(dev->parent);
 		ret = device_attach(dev);
-		if (dev->parent)
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_unlock(dev->parent);
 	}
 	return ret < 0 ? ret : 0;

@@ -770,10 +770,10 @@ EXPORT_SYMBOL_GPL(bus_rescan_devices);
 int device_reprobe(struct device *dev)
 {
 	if (dev->driver) {
-		if (dev->parent)        /* Needed for USB */
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_lock(dev->parent);
 		device_release_driver(dev);
-		if (dev->parent)
+		if (dev->parent && dev->bus->need_parent_lock)
 			device_unlock(dev->parent);
 	}
 	return bus_rescan_devices_helper(dev, NULL);

drivers/base/dd.c +4 -4

index c9f54089429b..7c09f73b96f3 100644
@@ -817,13 +817,13 @@ static int __driver_attach(struct device *dev, void *data)
 		return ret;
 	} /* ret > 0 means positive match */
 
-	if (dev->parent)	/* Needed for USB */
+	if (dev->parent && dev->bus->need_parent_lock)
 		device_lock(dev->parent);
 	device_lock(dev);
 	if (!dev->driver)
 		driver_probe_device(drv, dev);
 	device_unlock(dev);
-	if (dev->parent)
+	if (dev->parent && dev->bus->need_parent_lock)
 		device_unlock(dev->parent);
 
 	return 0;

@@ -919,7 +919,7 @@ void device_release_driver_internal(struct device *dev,
 				    struct device_driver *drv,
 				    struct device *parent)
 {
-	if (parent)
+	if (parent && dev->bus->need_parent_lock)
 		device_lock(parent);
 
 	device_lock(dev);

@@ -927,7 +927,7 @@ void device_release_driver_internal(struct device *dev,
 		__device_release_driver(dev, parent);
 
 	device_unlock(dev);
-	if (parent)
+	if (parent && dev->bus->need_parent_lock)
 		device_unlock(parent);
 }
 

drivers/usb/core/driver.c +1 -0

index 9792cedfc351..e76e95f62f76 100644
@@ -1922,4 +1922,5 @@ struct bus_type usb_bus_type = {
 	.name =		"usb",
 	.match =	usb_device_match,
 	.uevent =	usb_uevent,
+	.need_parent_lock =	true,
 };

include/linux/device.h +2 -0

index 477956990f5e..beca424395dd 100644
@@ -98,6 +98,8 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
  * @lock_key:	Lock class key for use by the lock validator
  * @force_dma:	Assume devices on this bus should be set up by dma_configure()
  * 		even if DMA capability is not explicitly described by firmware.
+ * @need_parent_lock:	When probing or removing a device on this bus, the
+ *			device core should lock the device's parent.
  *
  * A bus is a channel between the processor and one or more devices. For the
  * purposes of the device model, all devices are connected via a bus, even if

include/linux/device.h struct bus_type { · line 146

Quoted 7 lines

include/linux/device.h +1 -0

@@ -138,6 +140,7 @@ struct bus_type {
 	struct lock_class_key lock_key;
 
 	bool force_dma;
+	bool need_parent_lock;
 };
 
 extern int __must_check bus_register(struct bus_type *bus);

This looks okay to me.

Quoted 3 lines

Acked-by Alan Stern <[email protected]>

Greg, any more comments?

Looks sane to me. Martin, thanks for all of the changes.

greg k-h

Quoted 5 lines · Greg KH

Quoted 4 lines · Alan Stern

On Thu, 31 May 2018, Martin Liu wrote:

This looks okay to me.

Quoted 5 lines

Quoted 3 lines

Acked-by Alan Stern <[email protected]>

Greg, any more comments?

Looks sane to me. Martin, thanks for all of the changes.

Great thank Alan, Greg and Andy for helpful hints and feedback. Many Thanks!

SOC have internal I/O buses that can't be proved for devices. The devices on the buses can be accessed directly without additinal configuration required. This type of bus is represented as "simple-bus". In some platforms, we name "soc" with "simple-bus" attribute and many devices are hooked under it desribed in DT (device tree).

In commit 'bf74ad5bc417 introduce ("[PATCH] Hold the device's parent's lock during probe and remove")' to solve USB subsystem lock sequence since usb device's characteristic. Thus "soc" needs to be locked whenever a device and driver's probing happen under "soc" bus. During this period, an async driver tries to probe a device which is under the "soc" bus would be blocked until previous driver finish the probing and release "soc" lock. And the next probing under the "soc" bus need to wait for async finish. Because of that, driver's async probe for init time improvement will be shadowed.

Since many devices don't have USB devices' characteristic, they actually don't need parent's lock. Thus, we introduce a lock flag in device struct and driver core would lock the parent lock base on the flag. For usbsystem, we set this flag when its device and driver is matched and to keep original lock behavior in driver core.

Async probe could have more benefit after this patch.

Signed-off-by martin_liu <[email protected]>

Suggested-by Alan Stern <[email protected]>

+ }