Re: [driver-core PATCH v5 4/9] driver core: Move async_synchronize_full call

[Bart Van Assche]

On Tue, 2018-11-06 at 08:18 -0800, Alexander Duyck wrote:
+AD4 On Mon, 2018-11-05 at 17:04 -0800, Bart Van Assche wrote:
+AD4 +AD4 On Mon, 2018-11-05 at 13:11 -0800, Alexander Duyck wrote:
+AD4 +AD4 +AD4 This patch moves the async+AF8-synchronize+AF8-full call out of
+AD4 +AD4 +AD4 +AF8AXw-device+AF8-release+AF8-driver and into driver+AF8-detach.
+AD4 +AD4 +AD4 
+AD4 +AD4 +AD4 The idea behind this is that the async+AF8-synchronize+AF8-full call will only
+AD4 +AD4 +AD4 guarantee that any existing async operations are flushed. This doesn't do
+AD4 +AD4 +AD4 anything to guarantee that a hotplug event that may occur while we are
+AD4 +AD4 +AD4 doing the release of the driver will not be asynchronously scheduled.
+AD4 +AD4 +AD4 
+AD4 +AD4 +AD4 By moving this into the driver+AF8-detach path we can avoid potential deadlocks
+AD4 +AD4 +AD4 as we aren't holding the device lock at this point and we should not have
+AD4 +AD4 +AD4 the driver we want to flush loaded so the flush will take care of any
+AD4 +AD4 +AD4 asynchronous events the driver we are detaching might have scheduled.
+AD4 +AD4 +AD4 
+AD4 +AD4 +AD4 Signed-off-by: Alexander Duyck +ADw-alexander.h.duyck+AEA-linux.intel.com+AD4
+AD4 +AD4 +AD4 ---
+AD4 +AD4 +AD4  drivers/base/dd.c +AHw    6 +-+-+----
+AD4 +AD4 +AD4  1 file changed, 3 insertions(+-), 3 deletions(-)
+AD4 +AD4 +AD4 
+AD4 +AD4 +AD4 diff --git a/drivers/base/dd.c b/drivers/base/dd.c
+AD4 +AD4 +AD4 index 76c40fe69463..e74cefeb5b69 100644
+AD4 +AD4 +AD4 --- a/drivers/base/dd.c
+AD4 +AD4 +AD4 +-+-+- b/drivers/base/dd.c
+AD4 +AD4 +AD4 +AEAAQA -975,9 +-975,6 +AEAAQA static void +AF8AXw-device+AF8-release+AF8-driver(struct device +ACo-dev, struct device +ACo-parent)
+AD4 +AD4 +AD4  
+AD4 +AD4 +AD4  	drv +AD0 dev-+AD4-driver+ADs
+AD4 +AD4 +AD4  	if (drv) +AHs
+AD4 +AD4 +AD4 -		if (driver+AF8-allows+AF8-async+AF8-probing(drv))
+AD4 +AD4 +AD4 -			async+AF8-synchronize+AF8-full()+ADs
+AD4 +AD4 +AD4 -
+AD4 +AD4 +AD4  		while (device+AF8-links+AF8-busy(dev)) +AHs
+AD4 +AD4 +AD4  			+AF8AXw-device+AF8-driver+AF8-unlock(dev, parent)+ADs
+AD4 +AD4 +AD4  
+AD4 +AD4 +AD4 +AEAAQA -1087,6 +-1084,9 +AEAAQA void driver+AF8-detach(struct device+AF8-driver +ACo-drv)
+AD4 +AD4 +AD4  	struct device+AF8-private +ACo-dev+AF8-prv+ADs
+AD4 +AD4 +AD4  	struct device +ACo-dev+ADs
+AD4 +AD4 +AD4  
+AD4 +AD4 +AD4 +-	if (driver+AF8-allows+AF8-async+AF8-probing(drv))
+AD4 +AD4 +AD4 +-		async+AF8-synchronize+AF8-full()+ADs
+AD4 +AD4 +AD4 +-
+AD4 +AD4 +AD4  	for (+ADsAOw) +AHs
+AD4 +AD4 +AD4  		spin+AF8-lock(+ACY-drv-+AD4-p-+AD4-klist+AF8-devices.k+AF8-lock)+ADs
+AD4 +AD4 +AD4  		if (list+AF8-empty(+ACY-drv-+AD4-p-+AD4-klist+AF8-devices.k+AF8-list)) +AHs
+AD4 +AD4 
+AD4 +AD4 Have you considered to move that async+AF8-synchronize+AF8-full() call into
+AD4 +AD4 bus+AF8-remove+AF8-driver()? Verifying the correctness of this patch requires to
+AD4 +AD4 check whether the async+AF8-synchronize+AF8-full() comes after the
+AD4 +AD4 klist+AF8-remove(+ACY-drv-+AD4-p-+AD4-knode+AF8-bus) call. That verification is easier when
+AD4 +AD4 the async+AF8-synchronize+AF8-full() call occurs in bus+AF8-remove+AF8-driver() instead
+AD4 +AD4 of in driver+AF8-detach().
+AD4 
+AD4 I considered it, however it ends up with things being more symmetric to
+AD4 have use take care of synchronizing things in driver+AF8-detach since after
+AD4 this patch set we are scheduling thing asynchronously in driver+AF8-attach.
+AD4 
+AD4 Also I don't think it would be any great risk simply because calling
+AD4 driver+AF8-detach with the driver still associated with the bus would be a
+AD4 blatent error as it could easily lead to issues where you unbind a
+AD4 driver but have it get hotplugged to a device while that is going on.

Thanks for the additional clarification. Since I'm fine with this patch:

Reviewed-by: Bart Van Assche +ADw-bvanassche+AEA-acm.org+AD4