Commit a703c550 authored by Daniel Vetter's avatar Daniel Vetter

drm: safely free connectors from connector_iter

In

commit 613051da
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: default avatarBen Widawsky <ben@bwidawsk.net>
Cc: Ben Widawsky <ben@bwidawsk.net>
Fixes: 613051da ("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+
Reviewed-by: default avatarDave Airlie <airlied@gmail.com>
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20171204204818.24745-1-daniel.vetter@ffwll.ch
parent ae64f9bd
...@@ -152,6 +152,16 @@ static void drm_connector_free(struct kref *kref) ...@@ -152,6 +152,16 @@ static void drm_connector_free(struct kref *kref)
connector->funcs->destroy(connector); 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 * drm_connector_init - Init a preallocated connector
* @dev: DRM device * @dev: DRM device
...@@ -181,6 +191,8 @@ int drm_connector_init(struct drm_device *dev, ...@@ -181,6 +191,8 @@ int drm_connector_init(struct drm_device *dev,
if (ret) if (ret)
return ret; return ret;
INIT_WORK(&connector->free_work, drm_connector_free_work_fn);
connector->base.properties = &connector->properties; connector->base.properties = &connector->properties;
connector->dev = dev; connector->dev = dev;
connector->funcs = funcs; connector->funcs = funcs;
...@@ -529,6 +541,18 @@ void drm_connector_list_iter_begin(struct drm_device *dev, ...@@ -529,6 +541,18 @@ void drm_connector_list_iter_begin(struct drm_device *dev,
} }
EXPORT_SYMBOL(drm_connector_list_iter_begin); 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 * drm_connector_list_iter_next - return next connector
* @iter: connectr_list iterator * @iter: connectr_list iterator
...@@ -561,7 +585,7 @@ drm_connector_list_iter_next(struct drm_connector_list_iter *iter) ...@@ -561,7 +585,7 @@ drm_connector_list_iter_next(struct drm_connector_list_iter *iter)
spin_unlock_irqrestore(&config->connector_list_lock, flags); spin_unlock_irqrestore(&config->connector_list_lock, flags);
if (old_conn) if (old_conn)
drm_connector_put(old_conn); drm_connector_put_safe(old_conn);
return iter->conn; return iter->conn;
} }
...@@ -580,7 +604,7 @@ void drm_connector_list_iter_end(struct drm_connector_list_iter *iter) ...@@ -580,7 +604,7 @@ void drm_connector_list_iter_end(struct drm_connector_list_iter *iter)
{ {
iter->dev = NULL; iter->dev = NULL;
if (iter->conn) if (iter->conn)
drm_connector_put(iter->conn); drm_connector_put_safe(iter->conn);
lock_release(&connector_list_iter_dep_map, 0, _RET_IP_); lock_release(&connector_list_iter_dep_map, 0, _RET_IP_);
} }
EXPORT_SYMBOL(drm_connector_list_iter_end); EXPORT_SYMBOL(drm_connector_list_iter_end);
......
...@@ -431,6 +431,8 @@ void drm_mode_config_cleanup(struct drm_device *dev) ...@@ -431,6 +431,8 @@ void drm_mode_config_cleanup(struct drm_device *dev)
drm_connector_put(connector); drm_connector_put(connector);
} }
drm_connector_list_iter_end(&conn_iter); 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))) { if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) {
drm_connector_list_iter_begin(dev, &conn_iter); drm_connector_list_iter_begin(dev, &conn_iter);
drm_for_each_connector_iter(connector, &conn_iter) drm_for_each_connector_iter(connector, &conn_iter)
......
...@@ -916,6 +916,14 @@ struct drm_connector { ...@@ -916,6 +916,14 @@ struct drm_connector {
uint8_t num_h_tile, num_v_tile; uint8_t num_h_tile, num_v_tile;
uint8_t tile_h_loc, tile_v_loc; uint8_t tile_h_loc, tile_v_loc;
uint16_t tile_h_size, tile_v_size; 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) #define obj_to_connector(x) container_of(x, struct drm_connector, base)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment