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

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

[martin_liu]

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 <liumartin@google.com>
---
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.

 drivers/base/bus.c | 19 +++++++++++++------
 drivers/base/dd.c  | 23 ++++++++++++++++-------
 2 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index ef6183306b40..6434333995d4 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -181,13 +181,15 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
 	struct bus_type *bus = bus_get(drv->bus);
 	struct device *dev;
 	int err = -ENODEV;
+	bool allow_async;
 
+	allow_async = driver_allows_async_probing(drv);
 	dev = bus_find_device_by_name(bus, NULL, buf);
 	if (dev && dev->driver == drv) {
-		if (dev->parent)	/* Needed for USB */
+		if (dev->parent && !allow_async)/* Needed for USB */
 			device_lock(dev->parent);
 		device_release_driver(dev);
-		if (dev->parent)
+		if (dev->parent && !allow_async)
 			device_unlock(dev->parent);
 		err = count;
 	}
@@ -208,15 +210,17 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
 	struct bus_type *bus = bus_get(drv->bus);
 	struct device *dev;
 	int err = -ENODEV;
+	bool allow_async;
 
+	allow_async = driver_allows_async_probing(drv);
 	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 && !allow_async)/* Needed for USB */
 			device_lock(dev->parent);
 		device_lock(dev);
 		err = driver_probe_device(drv, dev);
 		device_unlock(dev);
-		if (dev->parent)
+		if (dev->parent && !allow_async)
 			device_unlock(dev->parent);
 
 		if (err > 0) {
@@ -769,11 +773,14 @@ EXPORT_SYMBOL_GPL(bus_rescan_devices);
  */
 int device_reprobe(struct device *dev)
 {
+	bool allow_async;
+
 	if (dev->driver) {
-		if (dev->parent)        /* Needed for USB */
+		allow_async = driver_allows_async_probing(dev->driver);
+		if (dev->parent && !allow_async)/* Needed for USB */
 			device_lock(dev->parent);
 		device_release_driver(dev);
-		if (dev->parent)
+		if (dev->parent && !allow_async)
 			device_unlock(dev->parent);
 	}
 	return bus_rescan_devices_helper(dev, NULL);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index c9f54089429b..36aed1576c58 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -794,6 +794,7 @@ static int __driver_attach(struct device *dev, void *data)
 {
 	struct device_driver *drv = data;
 	int ret;
+	bool allow_async;
 
 	/*
 	 * Lock device and try to bind to it. We drop the error
@@ -817,13 +818,14 @@ static int __driver_attach(struct device *dev, void *data)
 		return ret;
 	} /* ret > 0 means positive match */
 
-	if (dev->parent)	/* Needed for USB */
+	allow_async = driver_allows_async_probing(drv);
+	if (dev->parent && !allow_async)/* Needed for USB */
 		device_lock(dev->parent);
 	device_lock(dev);
 	if (!dev->driver)
 		driver_probe_device(drv, dev);
 	device_unlock(dev);
-	if (dev->parent)
+	if (dev->parent && !allow_async)
 		device_unlock(dev->parent);
 
 	return 0;
@@ -851,19 +853,21 @@ EXPORT_SYMBOL_GPL(driver_attach);
 static void __device_release_driver(struct device *dev, struct device *parent)
 {
 	struct device_driver *drv;
+	bool allow_async;
 
 	drv = dev->driver;
 	if (drv) {
-		if (driver_allows_async_probing(drv))
+		allow_async = driver_allows_async_probing(drv);
+		if (allow_async)
 			async_synchronize_full();
 
 		while (device_links_busy(dev)) {
 			device_unlock(dev);
-			if (parent)
+			if (parent && !allow_async)
 				device_unlock(parent);
 
 			device_links_unbind_consumers(dev);
-			if (parent)
+			if (parent && !allow_async)
 				device_lock(parent);
 
 			device_lock(dev);
@@ -919,7 +923,12 @@ void device_release_driver_internal(struct device *dev,
 				    struct device_driver *drv,
 				    struct device *parent)
 {
-	if (parent)
+	bool allow_async = false;
+
+	if (drv)
+		allow_async = driver_allows_async_probing(drv);
+
+	if (parent && !allow_async)
 		device_lock(parent);
 
 	device_lock(dev);
@@ -927,7 +936,7 @@ void device_release_driver_internal(struct device *dev,
 		__device_release_driver(dev, parent);
 
 	device_unlock(dev);
-	if (parent)
+	if (parent && !allow_async)
 		device_unlock(parent);
 }
 
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [RFC] driver core: don't hold dev's parent lock when using async probe

[Alan Stern]

On Tue, 22 May 2018, martin_liu wrote:

> 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 <liumartin@google.com>
> ---
> 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.

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

Re: [RFC] driver core: don't hold dev's parent lock when using async probe

[Martin Liu]

On Tue, May 22, 2018 at 01:09:44PM -0400, Alan Stern wrote:
> On Tue, 22 May 2018, martin_liu wrote:
> 
> > 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

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.

Martin

Re: [RFC] driver core: don't hold dev's parent lock when using async probe

[Alan Stern]

On Thu, 24 May 2018, Martin Liu wrote:

> On Tue, May 22, 2018 at 01:09:44PM -0400, Alan Stern wrote:
> > On Tue, 22 May 2018, martin_liu wrote:
> > 
> > > 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
> 
> 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

Re: [RFC] driver core: don't hold dev's parent lock when using async probe

[Martin Liu]

On Thu, May 24, 2018 at 11:02:57AM -0400, Alan Stern wrote:
> On Thu, 24 May 2018, Martin Liu wrote:
> 
> > On Tue, May 22, 2018 at 01:09:44PM -0400, Alan Stern wrote:
> > > On Tue, 22 May 2018, martin_liu wrote:
> > > 
> > > > 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
> > 
> > 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

Thanks for the quick feedback and the suggestion. will try to figure out how
it works.

Martin

[RFC PATCH v2] driver core: hold dev's parent lock when needed

[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 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 <liumartin@google.com>
Suggested-by: Alan Stern <stern@rowland.harvard.edu>
---
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(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index ef6183306b40..18ea94caec02 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -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->need_parent_lock)/* Needed for USB */
 			device_lock(dev->parent);
 		device_release_driver(dev);
-		if (dev->parent)
+		if (dev->parent && dev->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 && dev->need_parent_lock)/* Needed for USB */
 			device_lock(dev->parent);
 		device_lock(dev);
 		err = driver_probe_device(drv, dev);
 		device_unlock(dev);
-		if (dev->parent)
+		if (dev->parent && dev->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->need_parent_lock)/* Needed for USB */
 			device_lock(dev->parent);
 		ret = device_attach(dev);
-		if (dev->parent)
+		if (dev->parent && dev->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->need_parent_lock)/* Needed for USB */
 			device_lock(dev->parent);
 		device_release_driver(dev);
-		if (dev->parent)
+		if (dev->parent && dev->need_parent_lock)
 			device_unlock(dev->parent);
 	}
 	return bus_rescan_devices_helper(dev, NULL);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index c9f54089429b..c9a118568775 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -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->need_parent_lock)/* Needed for USB */
 		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->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->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->need_parent_lock)
 		device_unlock(parent);
 }
 
diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index 9a2ab6751a23..609566396bf8 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -40,13 +40,21 @@ static int ulpi_match(struct device *dev, struct device_driver *driver)
 	const struct ulpi_device_id *id;
 
 	/* Some ULPI devices don't have a vendor id so rely on OF match */
-	if (ulpi->id.vendor == 0)
-		return of_driver_match_device(dev, driver);
+	if (ulpi->id.vendor == 0) {
+		if (of_driver_match_device(dev, driver)) {
+			dev->need_parent_lock = 1;
+			return 1;
+		}
+		return o;
+	}
 
-	for (id = drv->id_table; id->vendor; id++)
+	for (id = drv->id_table; id->vendor; id++) {
 		if (id->vendor == ulpi->id.vendor &&
-		    id->product == ulpi->id.product)
+		    id->product == ulpi->id.product) {
+			dev->need_parent_lock = 1;
 			return 1;
+		}
+	}
 
 	return 0;
 }
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 9792cedfc351..3dbabf8d3cb8 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -808,6 +808,7 @@ static int usb_device_match(struct device *dev, struct device_driver *drv)
 			return 0;
 
 		/* TODO: Add real matching code */
+		dev->need_parent_lock = 1;
 		return 1;
 
 	} else if (is_usb_interface(dev)) {
@@ -823,12 +824,16 @@ static int usb_device_match(struct device *dev, struct device_driver *drv)
 		usb_drv = to_usb_driver(drv);
 
 		id = usb_match_id(intf, usb_drv->id_table);
-		if (id)
+		if (id) {
+			dev->need_parent_lock = 1;
 			return 1;
+		}
 
 		id = usb_match_dynamic_id(intf, usb_drv);
-		if (id)
+		if (id) {
+			dev->need_parent_lock = 1;
 			return 1;
+		}
 	}
 
 	return 0;
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index e221861b3187..03901fc86227 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -224,7 +224,11 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
 
 static bool usb_acpi_bus_match(struct device *dev)
 {
-	return is_usb_device(dev) || is_usb_port(dev);
+	if (is_usb_device(dev) || is_usb_port(dev)) {
+		dev->need_parent_lock = 1;
+		return 1;
+	}
+	return 0;
 }
 
 static struct acpi_bus_type usb_acpi_bus = {
diff --git a/drivers/usb/serial/bus.c b/drivers/usb/serial/bus.c
index 9e265eb92611..599ba0046f8e 100644
--- a/drivers/usb/serial/bus.c
+++ b/drivers/usb/serial/bus.c
@@ -29,8 +29,10 @@ static int usb_serial_device_match(struct device *dev,
 
 	driver = to_usb_serial_driver(drv);
 
-	if (driver == port->serial->type)
+	if (driver == port->serial->type) {
+		dev->need_parent_lock = 1;
 		return 1;
+	}
 
 	return 0;
 }
diff --git a/include/linux/device.h b/include/linux/device.h
index 477956990f5e..a96194f39dbe 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -992,6 +992,7 @@ struct device {
 	bool			offline_disabled:1;
 	bool			offline:1;
 	bool			of_node_reused:1;
+	bool			need_parent_lock:1;
 };
 
 static inline struct device *kobj_to_dev(struct kobject *kobj)
-- 
2.17.0.921.gf22659ad46-goog

Re: [RFC PATCH v2] driver core: hold dev's parent lock when needed

[Greg KH]

On Tue, May 29, 2018 at 03:07:20PM +0800, martin_liu wrote:
> 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 <liumartin@google.com>

Nit, your name here is probably not what you sign legal documents with.
Please use your "real name" in this line, and in your From: email line
(hint, no '_').  This prevents me from accepting the patch at all.

> Suggested-by: Alan Stern <stern@rowland.harvard.edu>

Changes were suggested, I don't know if the whole idea was :)

> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -992,6 +992,7 @@ struct device {
>  	bool			offline_disabled:1;
>  	bool			offline:1;
>  	bool			of_node_reused:1;
> +	bool			need_parent_lock:1;

Shouldn't this be a bus type flag, and not a device-specific flag?  You
are only ever caring about this based on the type of bus the device is
on, the rule would not be different for different devices on the same
bus from what I can tell.

thanks,

greg k-h

Re: [RFC PATCH v2] driver core: hold dev's parent lock when needed

[Alan Stern]

On Tue, 29 May 2018, martin_liu wrote:

> 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 <liumartin@google.com>
> Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> ---
> 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.

The match function is not the right place.  Take Greg's suggestion and 
put the new flag in the bus_type structure instead.

>  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(-)
> 
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index ef6183306b40..18ea94caec02 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -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->need_parent_lock)/* Needed for USB */

The new need_parent_lock flag is self-explanatory.  You can remove the 
"Needed for USB" comments here and elsewhere; instead just have a 
single comment where the flag is defined.

Alan Stern

Re: [RFC PATCH v2] driver core: hold dev's parent lock when needed

[Andy Shevchenko]

On Tue, May 29, 2018 at 10:07 AM, martin_liu <liumartin@google.com> wrote:
> 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 <liumartin@google.com>

Strange Firstname Lastname format...

> Suggested-by: Alan Stern <stern@rowland.harvard.edu>

I guess your SoB should go last (at least this is how -s works in git commit)

>         /* Some ULPI devices don't have a vendor id so rely on OF match */
> -       if (ulpi->id.vendor == 0)
> -               return of_driver_match_device(dev, driver);
> +       if (ulpi->id.vendor == 0) {
> +               if (of_driver_match_device(dev, driver)) {
> +                       dev->need_parent_lock = 1;
> +                       return 1;
> +               }

> +               return o;

Return what?

> +       }

-- 
With Best Regards,
Andy Shevchenko

[RFC PATCH v3] driver core: hold dev's parent lock when needed

[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 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 <liumartin@google.com>
---
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/base/bus.c        | 16 ++++++++--------
 drivers/base/dd.c         |  8 ++++----
 drivers/usb/common/ulpi.c |  1 +
 drivers/usb/core/driver.c |  1 +
 drivers/usb/serial/bus.c  |  1 +
 include/linux/device.h    |  3 +++
 6 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index ef6183306b40..1e606fbc95a7 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -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 && dev->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 && dev->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);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index c9f54089429b..7c09f73b96f3 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -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);
 }
 
diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index 9a2ab6751a23..073954a1a1c5 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -94,6 +94,7 @@ static struct bus_type ulpi_bus = {
 	.uevent = ulpi_uevent,
 	.probe = ulpi_probe,
 	.remove = ulpi_remove,
+	.need_parent_lock = 1,
 };
 
 /* -------------------------------------------------------------------------- */
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 9792cedfc351..209ee5d8a92d 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1922,4 +1922,5 @@ struct bus_type usb_bus_type = {
 	.name =		"usb",
 	.match =	usb_device_match,
 	.uevent =	usb_uevent,
+	.need_parent_lock =	1,
 };
diff --git a/drivers/usb/serial/bus.c b/drivers/usb/serial/bus.c
index 9e265eb92611..55b2636b2804 100644
--- a/drivers/usb/serial/bus.c
+++ b/drivers/usb/serial/bus.c
@@ -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)
diff --git a/include/linux/device.h b/include/linux/device.h
index 477956990f5e..019b193aeb24 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -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:	Assume devices on this bus should hold its' parent's
+ *			lock during probe and remove. Currently, USB needs it.
  *
  * 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;
 };
 
 extern int __must_check bus_register(struct bus_type *bus);
-- 
2.17.0.921.gf22659ad46-goog

Re: [RFC PATCH v3] driver core: hold dev's parent lock when needed

[Greg KH]

On Wed, May 30, 2018 at 12:34:29AM +0800, Martin Liu wrote:
> 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).

<snip>

Hint, I don't apply "RFC" patches as obviously the author does not think
it is good enough to be merged :)

Re: [RFC PATCH v3] driver core: hold dev's parent lock when needed

[Andy Shevchenko]

On Tue, May 29, 2018 at 7:34 PM, Martin Liu <liumartin@google.com> wrote:
> SOC have internal I/O buses that can't be proved for devices. The

Perhaps SoC as a common abbr for system-on-chip.

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

described


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

The formal commit reference doesn't include '' (surrounding quotes)
and words in square brackets (like [PATCH] here).

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

usb or USB ?

> 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

USB system

USB

relative

> bus_type struct to keep original lock behavior in driver core.
>
> Async probe could have more benefit after this patch.


> -               if (dev->parent)        /* Needed for USB */
> +               if (dev->parent && dev->bus->need_parent_lock)

So, why not to use bus directly like bus->...?

>                         device_lock(dev->parent);
>                 device_release_driver(dev);
> -               if (dev->parent)
> +               if (dev->parent && dev->bus->need_parent_lock)
>                         device_unlock(dev->parent);

Ditto here and everywhere else in the patch where applicable.

> +       .need_parent_lock = 1,
> +       .need_parent_lock =     1,
> +       .need_parent_lock =     1,

It's boolean, you need to use true or false. Check and fix your code
correspondingly.

> +       bool need_parent_lock;

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v3] driver core: hold dev's parent lock when needed

[Alan Stern]

On Wed, 30 May 2018, Martin Liu wrote:

> 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 <liumartin@google.com>
> ---
> 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.

As far as I know, only usb_bus_type needs the flag.  Not ulpi_bus or
usb_serial_bus_type.  However, you should check with the maintainers to
make sure.

> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> index 9a2ab6751a23..073954a1a1c5 100644
> --- a/drivers/usb/common/ulpi.c
> +++ b/drivers/usb/common/ulpi.c
> @@ -94,6 +94,7 @@ static struct bus_type ulpi_bus = {
>  	.uevent = ulpi_uevent,
>  	.probe = ulpi_probe,
>  	.remove = ulpi_remove,
> +	.need_parent_lock = 1,
>  };
>  
>  /* -------------------------------------------------------------------------- */
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 9792cedfc351..209ee5d8a92d 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1922,4 +1922,5 @@ struct bus_type usb_bus_type = {
>  	.name =		"usb",
>  	.match =	usb_device_match,
>  	.uevent =	usb_uevent,
> +	.need_parent_lock =	1,
>  };
> diff --git a/drivers/usb/serial/bus.c b/drivers/usb/serial/bus.c
> index 9e265eb92611..55b2636b2804 100644
> --- a/drivers/usb/serial/bus.c
> +++ b/drivers/usb/serial/bus.c
> @@ -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)
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 477956990f5e..019b193aeb24 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -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:	Assume devices on this bus should hold its' parent's
> + *			lock during probe and remove. Currently, USB needs it.

This comment is not written very well.  Suggested improvement:

 * @need_parent_lock:	When probing or removing a device on this bus, the 
			device core should lock the device's parent.

If you want, you can say that usb_bus_type sets the flag.  I don't
think it's really necessary to mention this.

Alan Stern

[PATCH v4] driver core: hold dev's parent lock when needed

[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 <liumartin@google.com>
---
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(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index ef6183306b40..8bfd27ec73d6 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -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);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index c9f54089429b..7c09f73b96f3 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -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);
 }
 
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 9792cedfc351..e76e95f62f76 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1922,4 +1922,5 @@ struct bus_type usb_bus_type = {
 	.name =		"usb",
 	.match =	usb_device_match,
 	.uevent =	usb_uevent,
+	.need_parent_lock =	true,
 };
diff --git a/include/linux/device.h b/include/linux/device.h
index 477956990f5e..beca424395dd 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -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;
 };
 
 extern int __must_check bus_register(struct bus_type *bus);
-- 
2.17.0.921.gf22659ad46-goog

Re: [PATCH v4] driver core: hold dev's parent lock when needed

[Alan Stern]

On Thu, 31 May 2018, Martin Liu wrote:

> 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 <liumartin@google.com>
> ---
> 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(-)
> 
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index ef6183306b40..8bfd27ec73d6 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -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);
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index c9f54089429b..7c09f73b96f3 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -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);
>  }
>  
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 9792cedfc351..e76e95f62f76 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1922,4 +1922,5 @@ struct bus_type usb_bus_type = {
>  	.name =		"usb",
>  	.match =	usb_device_match,
>  	.uevent =	usb_uevent,
> +	.need_parent_lock =	true,
>  };
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 477956990f5e..beca424395dd 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -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;
>  };
>  
>  extern int __must_check bus_register(struct bus_type *bus);

This looks okay to me.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Greg, any more comments?

Alan Stern

Re: [PATCH v4] driver core: hold dev's parent lock when needed

[Greg KH]

On Wed, May 30, 2018 at 01:21:02PM -0400, Alan Stern wrote:
> On Thu, 31 May 2018, Martin Liu wrote:
> 
> > 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 <liumartin@google.com>
> > ---
> > 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(-)
> > 
> > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > index ef6183306b40..8bfd27ec73d6 100644
> > --- a/drivers/base/bus.c
> > +++ b/drivers/base/bus.c
> > @@ -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);
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index c9f54089429b..7c09f73b96f3 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -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);
> >  }
> >  
> > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > index 9792cedfc351..e76e95f62f76 100644
> > --- a/drivers/usb/core/driver.c
> > +++ b/drivers/usb/core/driver.c
> > @@ -1922,4 +1922,5 @@ struct bus_type usb_bus_type = {
> >  	.name =		"usb",
> >  	.match =	usb_device_match,
> >  	.uevent =	usb_uevent,
> > +	.need_parent_lock =	true,
> >  };
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 477956990f5e..beca424395dd 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -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;
> >  };
> >  
> >  extern int __must_check bus_register(struct bus_type *bus);
> 
> This looks okay to me.
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> 
> Greg, any more comments?

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

greg k-h

Re: [PATCH v4] driver core: hold dev's parent lock when needed

[Martin Liu]

On Thu, May 31, 2018 at 08:31:18AM +0200, Greg KH wrote:
> On Wed, May 30, 2018 at 01:21:02PM -0400, Alan Stern wrote:
> > On Thu, 31 May 2018, Martin Liu wrote:
> > 
> > > 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 <liumartin@google.com>
> > > ---
> > > 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(-)
> > > 
> > > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > > index ef6183306b40..8bfd27ec73d6 100644
> > > --- a/drivers/base/bus.c
> > > +++ b/drivers/base/bus.c
> > > @@ -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);
> > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > index c9f54089429b..7c09f73b96f3 100644
> > > --- a/drivers/base/dd.c
> > > +++ b/drivers/base/dd.c
> > > @@ -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);
> > >  }
> > >  
> > > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > > index 9792cedfc351..e76e95f62f76 100644
> > > --- a/drivers/usb/core/driver.c
> > > +++ b/drivers/usb/core/driver.c
> > > @@ -1922,4 +1922,5 @@ struct bus_type usb_bus_type = {
> > >  	.name =		"usb",
> > >  	.match =	usb_device_match,
> > >  	.uevent =	usb_uevent,
> > > +	.need_parent_lock =	true,
> > >  };
> > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > index 477956990f5e..beca424395dd 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -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;
> > >  };
> > >  
> > >  extern int __must_check bus_register(struct bus_type *bus);
> > 
> > This looks okay to me.
> > 
> > Acked-by: Alan Stern <stern@rowland.harvard.edu>
> > 
> > Greg, any more comments?
> 
> Looks sane to me.  Martin, thanks for all of the changes.
> 
> greg k-h

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

Martin

Re: [PATCH v4] driver core: hold dev's parent lock when needed

[Andy Shevchenko]

On Thu, May 31, 2018 at 10:27 AM, Martin Liu <liumartin@google.com> wrote:
> On Thu, May 31, 2018 at 08:31:18AM +0200, Greg KH wrote:
>> On Wed, May 30, 2018 at 01:21:02PM -0400, Alan Stern wrote:
>> > On Thu, 31 May 2018, Martin Liu wrote:

>> > This looks okay to me.
>> >
>> > Acked-by: Alan Stern <stern@rowland.harvard.edu>
>> >
>> > 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!

You are welcome!

Btw, FWIW,

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko