Patchwork drm: savely free connectors from connector_iter

login
register
mail settings
Submitter Daniel Vetter
Date Dec. 4, 2017, 8:48 p.m.
Message ID <20171204204818.24745-1-daniel.vetter@ffwll.ch>
Download mbox | patch
Permalink /patch/10091551/
State New
Headers show

Comments

Daniel Vetter - Dec. 4, 2017, 8:48 p.m.
In

commit 613051dac40da1751ab269572766d3348d45a197
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Wed Dec 14 00:08:06 2016 +0100

    drm: locking&new iterators for connector_list

we've went to extreme lengths to make sure connector iterations works
in any context, without introducing any additional locking context.
This worked, except for a small fumble in the implementation:

When we actually race with a concurrent connector unplug event, and
our temporary connector reference turns out to be the final one, then
everything breaks: We call the connector release function from
whatever context we happen to be in, which can be an irq/atomic
context. And connector freeing grabs all kinds of locks and stuff.

Fix this by creating a specially safe put function for connetor_iter,
which (in this rare case) punts the cleanup to a worker.

Reported-by: Ben Widawsky <ben@bwidawsk.net>
Cc: Ben Widawsky <ben@bwidawsk.net>
Fixes: 613051dac40d ("drm: locking&new iterators for connector_list")
Cc: Dave Airlie <airlied@gmail.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: <stable@vger.kernel.org> # v4.11+
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_connector.c   | 28 ++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_mode_config.c |  2 ++
 include/drm/drm_connector.h       |  8 ++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)
Daniel Vetter - Dec. 5, 2017, 9:56 a.m.
On Mon, Dec 04, 2017 at 09:48:18PM +0100, Daniel Vetter wrote:
> In
> 
> commit 613051dac40da1751ab269572766d3348d45a197
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Wed Dec 14 00:08:06 2016 +0100
> 
>     drm: locking&new iterators for connector_list
> 
> we've went to extreme lengths to make sure connector iterations works
> in any context, without introducing any additional locking context.
> This worked, except for a small fumble in the implementation:
> 
> When we actually race with a concurrent connector unplug event, and
> our temporary connector reference turns out to be the final one, then
> everything breaks: We call the connector release function from
> whatever context we happen to be in, which can be an irq/atomic
> context. And connector freeing grabs all kinds of locks and stuff.
> 
> Fix this by creating a specially safe put function for connetor_iter,
> which (in this rare case) punts the cleanup to a worker.
> 
> Reported-by: Ben Widawsky <ben@bwidawsk.net>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Fixes: 613051dac40d ("drm: locking&new iterators for connector_list")
> Cc: Dave Airlie <airlied@gmail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: <stable@vger.kernel.org> # v4.11+
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

s/savely/safely/ in the summary and

Reviewed-by: Dave Airlie <airlied@gmail.com>

from irc. CI is also happy, I'll merge as soon as Ben has approved this
too.
-Daniel

> ---
>  drivers/gpu/drm/drm_connector.c   | 28 ++++++++++++++++++++++++++--
>  drivers/gpu/drm/drm_mode_config.c |  2 ++
>  include/drm/drm_connector.h       |  8 ++++++++
>  3 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 25f4b2e9a44f..482014137953 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -152,6 +152,16 @@ static void drm_connector_free(struct kref *kref)
>  	connector->funcs->destroy(connector);
>  }
>  
> +static void drm_connector_free_work_fn(struct work_struct *work)
> +{
> +	struct drm_connector *connector =
> +		container_of(work, struct drm_connector, free_work);
> +	struct drm_device *dev = connector->dev;
> +
> +	drm_mode_object_unregister(dev, &connector->base);
> +	connector->funcs->destroy(connector);
> +}
> +
>  /**
>   * drm_connector_init - Init a preallocated connector
>   * @dev: DRM device
> @@ -181,6 +191,8 @@ int drm_connector_init(struct drm_device *dev,
>  	if (ret)
>  		return ret;
>  
> +	INIT_WORK(&connector->free_work, drm_connector_free_work_fn);
> +
>  	connector->base.properties = &connector->properties;
>  	connector->dev = dev;
>  	connector->funcs = funcs;
> @@ -529,6 +541,18 @@ void drm_connector_list_iter_begin(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_connector_list_iter_begin);
>  
> +/*
> + * Extra-safe connector put function that works in any context. Should only be
> + * used from the connector_iter functions, where we never really expect to
> + * actually release the connector when dropping our final reference.
> + */
> +static void
> +drm_connector_put_safe(struct drm_connector *conn)
> +{
> +	if (refcount_dec_and_test(&conn->base.refcount.refcount))
> +		schedule_work(&conn->free_work);
> +}
> +
>  /**
>   * drm_connector_list_iter_next - return next connector
>   * @iter: connectr_list iterator
> @@ -561,7 +585,7 @@ drm_connector_list_iter_next(struct drm_connector_list_iter *iter)
>  	spin_unlock_irqrestore(&config->connector_list_lock, flags);
>  
>  	if (old_conn)
> -		drm_connector_put(old_conn);
> +		drm_connector_put_safe(old_conn);
>  
>  	return iter->conn;
>  }
> @@ -580,7 +604,7 @@ void drm_connector_list_iter_end(struct drm_connector_list_iter *iter)
>  {
>  	iter->dev = NULL;
>  	if (iter->conn)
> -		drm_connector_put(iter->conn);
> +		drm_connector_put_safe(iter->conn);
>  	lock_release(&connector_list_iter_dep_map, 0, _RET_IP_);
>  }
>  EXPORT_SYMBOL(drm_connector_list_iter_end);
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 7623607c0f1e..346c19c6ce01 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -431,6 +431,8 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>  		drm_connector_put(connector);
>  	}
>  	drm_connector_list_iter_end(&conn_iter);
> +	/* connector_iter drops references in a work item. */
> +	flush_scheduled_work();
>  	if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) {
>  		drm_connector_list_iter_begin(dev, &conn_iter);
>  		drm_for_each_connector_iter(connector, &conn_iter)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 66d6c99d15e5..c5c753a1be85 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -926,6 +926,14 @@ struct drm_connector {
>  	uint8_t num_h_tile, num_v_tile;
>  	uint8_t tile_h_loc, tile_v_loc;
>  	uint16_t tile_h_size, tile_v_size;
> +
> +	/**
> +	 * @free_work:
> +	 *
> +	 * Work used only by &drm_connector_iter to be able to clean up a
> +	 * connector from any context.
> +	 */
> +	struct work_struct free_work;
>  };
>  
>  #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> -- 
> 2.15.0
>

Patch

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 25f4b2e9a44f..482014137953 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -152,6 +152,16 @@  static void drm_connector_free(struct kref *kref)
 	connector->funcs->destroy(connector);
 }
 
+static void drm_connector_free_work_fn(struct work_struct *work)
+{
+	struct drm_connector *connector =
+		container_of(work, struct drm_connector, free_work);
+	struct drm_device *dev = connector->dev;
+
+	drm_mode_object_unregister(dev, &connector->base);
+	connector->funcs->destroy(connector);
+}
+
 /**
  * drm_connector_init - Init a preallocated connector
  * @dev: DRM device
@@ -181,6 +191,8 @@  int drm_connector_init(struct drm_device *dev,
 	if (ret)
 		return ret;
 
+	INIT_WORK(&connector->free_work, drm_connector_free_work_fn);
+
 	connector->base.properties = &connector->properties;
 	connector->dev = dev;
 	connector->funcs = funcs;
@@ -529,6 +541,18 @@  void drm_connector_list_iter_begin(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_connector_list_iter_begin);
 
+/*
+ * Extra-safe connector put function that works in any context. Should only be
+ * used from the connector_iter functions, where we never really expect to
+ * actually release the connector when dropping our final reference.
+ */
+static void
+drm_connector_put_safe(struct drm_connector *conn)
+{
+	if (refcount_dec_and_test(&conn->base.refcount.refcount))
+		schedule_work(&conn->free_work);
+}
+
 /**
  * drm_connector_list_iter_next - return next connector
  * @iter: connectr_list iterator
@@ -561,7 +585,7 @@  drm_connector_list_iter_next(struct drm_connector_list_iter *iter)
 	spin_unlock_irqrestore(&config->connector_list_lock, flags);
 
 	if (old_conn)
-		drm_connector_put(old_conn);
+		drm_connector_put_safe(old_conn);
 
 	return iter->conn;
 }
@@ -580,7 +604,7 @@  void drm_connector_list_iter_end(struct drm_connector_list_iter *iter)
 {
 	iter->dev = NULL;
 	if (iter->conn)
-		drm_connector_put(iter->conn);
+		drm_connector_put_safe(iter->conn);
 	lock_release(&connector_list_iter_dep_map, 0, _RET_IP_);
 }
 EXPORT_SYMBOL(drm_connector_list_iter_end);
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 7623607c0f1e..346c19c6ce01 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -431,6 +431,8 @@  void drm_mode_config_cleanup(struct drm_device *dev)
 		drm_connector_put(connector);
 	}
 	drm_connector_list_iter_end(&conn_iter);
+	/* connector_iter drops references in a work item. */
+	flush_scheduled_work();
 	if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) {
 		drm_connector_list_iter_begin(dev, &conn_iter);
 		drm_for_each_connector_iter(connector, &conn_iter)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 66d6c99d15e5..c5c753a1be85 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -926,6 +926,14 @@  struct drm_connector {
 	uint8_t num_h_tile, num_v_tile;
 	uint8_t tile_h_loc, tile_v_loc;
 	uint16_t tile_h_size, tile_v_size;
+
+	/**
+	 * @free_work:
+	 *
+	 * Work used only by &drm_connector_iter to be able to clean up a
+	 * connector from any context.
+	 */
+	struct work_struct free_work;
 };
 
 #define obj_to_connector(x) container_of(x, struct drm_connector, base)