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@xxxxxxxxxx>
> Suggested-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> ---
> 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