Re: [PATCH] drm: savely free connectors from connector_iter

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