[driver-core,v5,4/9] driver core: Move async_synchronize_full call
diff mbox series

Message ID 154145231976.29224.39638353241256116.stgit@ahduyck-desk1.jf.intel.com
State Superseded
Headers show
Series
  • Add NUMA aware async_schedule calls
Related show

Commit Message

Alexander Duyck Nov. 5, 2018, 9:11 p.m. UTC
This patch moves the async_synchronize_full call out of
__device_release_driver and into driver_detach.

The idea behind this is that the async_synchronize_full call will only
guarantee that any existing async operations are flushed. This doesn't do
anything to guarantee that a hotplug event that may occur while we are
doing the release of the driver will not be asynchronously scheduled.

By moving this into the driver_detach path we can avoid potential deadlocks
as we aren't holding the device lock at this point and we should not have
the driver we want to flush loaded so the flush will take care of any
asynchronous events the driver we are detaching might have scheduled.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 drivers/base/dd.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Bart Van Assche Nov. 6, 2018, 1:04 a.m. UTC | #1
On Mon, 2018-11-05 at 13:11 -0800, Alexander Duyck wrote:
> This patch moves the async_synchronize_full call out of
> __device_release_driver and into driver_detach.
> 
> The idea behind this is that the async_synchronize_full call will only
> guarantee that any existing async operations are flushed. This doesn't do
> anything to guarantee that a hotplug event that may occur while we are
> doing the release of the driver will not be asynchronously scheduled.
> 
> By moving this into the driver_detach path we can avoid potential deadlocks
> as we aren't holding the device lock at this point and we should not have
> the driver we want to flush loaded so the flush will take care of any
> asynchronous events the driver we are detaching might have scheduled.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  drivers/base/dd.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 76c40fe69463..e74cefeb5b69 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -975,9 +975,6 @@ static void __device_release_driver(struct device *dev, struct device *parent)
>  
>  	drv = dev->driver;
>  	if (drv) {
> -		if (driver_allows_async_probing(drv))
> -			async_synchronize_full();
> -
>  		while (device_links_busy(dev)) {
>  			__device_driver_unlock(dev, parent);
>  
> @@ -1087,6 +1084,9 @@ void driver_detach(struct device_driver *drv)
>  	struct device_private *dev_prv;
>  	struct device *dev;
>  
> +	if (driver_allows_async_probing(drv))
> +		async_synchronize_full();
> +
>  	for (;;) {
>  		spin_lock(&drv->p->klist_devices.k_lock);
>  		if (list_empty(&drv->p->klist_devices.k_list)) {

Have you considered to move that async_synchronize_full() call into
bus_remove_driver()? Verifying the correctness of this patch requires to
check whether the async_synchronize_full() comes after the
klist_remove(&drv->p->knode_bus) call. That verification is easier when
the async_synchronize_full() call occurs in bus_remove_driver() instead
of in driver_detach().

Thanks,

Bart.
Alexander Duyck Nov. 6, 2018, 4:18 p.m. UTC | #2
On Mon, 2018-11-05 at 17:04 -0800, Bart Van Assche wrote:
> On Mon, 2018-11-05 at 13:11 -0800, Alexander Duyck wrote:
> > This patch moves the async_synchronize_full call out of
> > __device_release_driver and into driver_detach.
> > 
> > The idea behind this is that the async_synchronize_full call will only
> > guarantee that any existing async operations are flushed. This doesn't do
> > anything to guarantee that a hotplug event that may occur while we are
> > doing the release of the driver will not be asynchronously scheduled.
> > 
> > By moving this into the driver_detach path we can avoid potential deadlocks
> > as we aren't holding the device lock at this point and we should not have
> > the driver we want to flush loaded so the flush will take care of any
> > asynchronous events the driver we are detaching might have scheduled.
> > 
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> >  drivers/base/dd.c |    6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index 76c40fe69463..e74cefeb5b69 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -975,9 +975,6 @@ static void __device_release_driver(struct device *dev, struct device *parent)
> >  
> >  	drv = dev->driver;
> >  	if (drv) {
> > -		if (driver_allows_async_probing(drv))
> > -			async_synchronize_full();
> > -
> >  		while (device_links_busy(dev)) {
> >  			__device_driver_unlock(dev, parent);
> >  
> > @@ -1087,6 +1084,9 @@ void driver_detach(struct device_driver *drv)
> >  	struct device_private *dev_prv;
> >  	struct device *dev;
> >  
> > +	if (driver_allows_async_probing(drv))
> > +		async_synchronize_full();
> > +
> >  	for (;;) {
> >  		spin_lock(&drv->p->klist_devices.k_lock);
> >  		if (list_empty(&drv->p->klist_devices.k_list)) {
> 
> Have you considered to move that async_synchronize_full() call into
> bus_remove_driver()? Verifying the correctness of this patch requires to
> check whether the async_synchronize_full() comes after the
> klist_remove(&drv->p->knode_bus) call. That verification is easier when
> the async_synchronize_full() call occurs in bus_remove_driver() instead
> of in driver_detach().
> 
> Thanks,
> 
> Bart.

I considered it, however it ends up with things being more symmetric to
have use take care of synchronizing things in driver_detach since after
this patch set we are scheduling thing asynchronously in driver_attach.

Also I don't think it would be any great risk simply because calling
driver_detach with the driver still associated with the bus would be a
blatent error as it could easily lead to issues where you unbind a
driver but have it get hotplugged to a device while that is going on.

- Alex
Bart Van Assche Nov. 6, 2018, 5:22 p.m. UTC | #3
On Tue, 2018-11-06 at 08:18 -0800, Alexander Duyck wrote:
> On Mon, 2018-11-05 at 17:04 -0800, Bart Van Assche wrote:
> > On Mon, 2018-11-05 at 13:11 -0800, Alexander Duyck wrote:
> > > This patch moves the async_synchronize_full call out of
> > > __device_release_driver and into driver_detach.
> > > 
> > > The idea behind this is that the async_synchronize_full call will only
> > > guarantee that any existing async operations are flushed. This doesn't do
> > > anything to guarantee that a hotplug event that may occur while we are
> > > doing the release of the driver will not be asynchronously scheduled.
> > > 
> > > By moving this into the driver_detach path we can avoid potential deadlocks
> > > as we aren't holding the device lock at this point and we should not have
> > > the driver we want to flush loaded so the flush will take care of any
> > > asynchronous events the driver we are detaching might have scheduled.
> > > 
> > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > ---
> > >  drivers/base/dd.c |    6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > index 76c40fe69463..e74cefeb5b69 100644
> > > --- a/drivers/base/dd.c
> > > +++ b/drivers/base/dd.c
> > > @@ -975,9 +975,6 @@ static void __device_release_driver(struct device *dev, struct device *parent)
> > >  
> > >  	drv = dev->driver;
> > >  	if (drv) {
> > > -		if (driver_allows_async_probing(drv))
> > > -			async_synchronize_full();
> > > -
> > >  		while (device_links_busy(dev)) {
> > >  			__device_driver_unlock(dev, parent);
> > >  
> > > @@ -1087,6 +1084,9 @@ void driver_detach(struct device_driver *drv)
> > >  	struct device_private *dev_prv;
> > >  	struct device *dev;
> > >  
> > > +	if (driver_allows_async_probing(drv))
> > > +		async_synchronize_full();
> > > +
> > >  	for (;;) {
> > >  		spin_lock(&drv->p->klist_devices.k_lock);
> > >  		if (list_empty(&drv->p->klist_devices.k_list)) {
> > 
> > Have you considered to move that async_synchronize_full() call into
> > bus_remove_driver()? Verifying the correctness of this patch requires to
> > check whether the async_synchronize_full() comes after the
> > klist_remove(&drv->p->knode_bus) call. That verification is easier when
> > the async_synchronize_full() call occurs in bus_remove_driver() instead
> > of in driver_detach().
> 
> I considered it, however it ends up with things being more symmetric to
> have use take care of synchronizing things in driver_detach since after
> this patch set we are scheduling thing asynchronously in driver_attach.
> 
> Also I don't think it would be any great risk simply because calling
> driver_detach with the driver still associated with the bus would be a
> blatent error as it could easily lead to issues where you unbind a
> 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 <bvanassche@acm.org>

Patch
diff mbox series

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 76c40fe69463..e74cefeb5b69 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -975,9 +975,6 @@  static void __device_release_driver(struct device *dev, struct device *parent)
 
 	drv = dev->driver;
 	if (drv) {
-		if (driver_allows_async_probing(drv))
-			async_synchronize_full();
-
 		while (device_links_busy(dev)) {
 			__device_driver_unlock(dev, parent);
 
@@ -1087,6 +1084,9 @@  void driver_detach(struct device_driver *drv)
 	struct device_private *dev_prv;
 	struct device *dev;
 
+	if (driver_allows_async_probing(drv))
+		async_synchronize_full();
+
 	for (;;) {
 		spin_lock(&drv->p->klist_devices.k_lock);
 		if (list_empty(&drv->p->klist_devices.k_list)) {