This is a pretty good horror show, but I think it's the best tradeoff:
- Thanks to srcu and delayed freeing the locking doesn't leak out to
callers, hence no added headaches with locking inversions.
- For core and drivers which hot-remove connectors all the connector
list walking details are hidden in a macro.
- Other drivers which don't care about hot-removing can simply keep on
using the normal list walking macros.
The new design is:
- New dev->mode_config.connector_list_lock to protect the
connector_list, and the connector_ida (since that's also
unprotected), plus num_connectors. This is a pure leaf lock, nothing
is allowed to nest within, nothing outside of connector init/cleanup
will ever need it.
- Protect connector_list itself with srcu. This allows sleeping and
anything else. The only thing which is not allowed is calling
synchronize_srcu, or grabbing any locks or waiting on
waitqueues/completions/whatever which might call that. To make this
100% safe we opt to not have any calls to synchronize_srcu.
- Protect against use-after-free of connectors using call_srcu, to
delay the kfree enough.
- To protect against zombie connectors which are in progress of final
destruction use kref_get_unless_zero. This is safe since srcu
prevents against use-after-free, and that's the only guarantee we
need.
For this demo I only bothered converting i915, and there also not
everything - I left the connector loops in the modeset code unchanged
since those will all be converted over to
drm_for_each_connector_in_state to make them work correctly for
nonblocking atomic commits. Only loops outside of atomic commits
should be converted to drm_for_each_connector.
Note that the i915 DP MST code still uses drm_modeset_lock_all(). But
that's not because of drm core connector lifetime issues, but because
the fbdev helper reuses core locks to mange its own lists and data
structures. Thierry Reding has a patch series to gift fbdev its own
lock, which will remedy this.
v2: Totally rewrite everything to pack it up in a neat
iterator macro, with init/check/next extracted into helpers.
v3: Tiny rebase conflict.
Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
---
drivers/gpu/drm/drm_crtc.c | 57 +++++++++++++++++++++-----
drivers/gpu/drm/i915/intel_dp_mst.c | 2 +-
include/drm/drm_crtc.h | 82 +++++++++++++++++++++++++++----------
3 files changed, 109 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 6a8f91e8821b..dc22c0bfbe99 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -44,6 +44,9 @@
#include "drm_crtc_internal.h"
#include "drm_internal.h"
+DEFINE_SRCU(drm_connector_list_srcu);
+EXPORT_SYMBOL(drm_connector_list_srcu);
+
static struct drm_framebuffer *
internal_framebuffer_create(struct drm_device *dev,
const struct drm_mode_fb_cmd2 *r,
@@ -825,7 +828,7 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
mode->interlace ? " interlaced" : "");
}
-static void drm_connector_free(struct kref *kref)
+static void drm_connector_kref_release(struct kref *kref)
{
struct drm_connector *connector =
container_of(kref, struct drm_connector, base.refcount);
@@ -858,11 +861,10 @@ int drm_connector_init(struct drm_device *dev,
struct ida *connector_ida =
&drm_connector_enum_list[connector_type].ida;
- drm_modeset_lock_all(dev);
-
+ mutex_lock(&dev->mode_config.connector_list_lock);
ret = drm_mode_object_get_reg(dev, &connector->base,
DRM_MODE_OBJECT_CONNECTOR,
- false, drm_connector_free);
+ false, drm_connector_kref_release);
if (ret)
goto out_unlock;
@@ -899,11 +901,6 @@ int drm_connector_init(struct drm_device *dev,
drm_connector_get_cmdline_mode(connector);
- /* We should add connectors at the end to avoid upsetting the connector
- * index too much. */
- list_add_tail(&connector->head, &config->connector_list);
- config->num_connector++;
-
if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL)
drm_object_attach_property(&connector->base,
config->edid_property,
@@ -917,6 +914,11 @@ int drm_connector_init(struct drm_device *dev,
}
connector->debugfs_entry = NULL;
+
+ /* We must add the connector to the list last. */
+ list_add_tail_rcu(&connector->head, &config->connector_list);
+ config->num_connector++;
+
out_put_type_id:
if (ret)
ida_remove(connector_ida, connector->connector_type_id);
@@ -928,7 +930,7 @@ out_put:
drm_mode_object_unregister(dev, &connector->base);
out_unlock:
- drm_modeset_unlock_all(dev);
+ mutex_unlock(&dev->mode_config.connector_list_lock);
return ret;
}
@@ -951,6 +953,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
if (WARN_ON(connector->registered))
drm_connector_unregister(connector);
+ mutex_lock(&dev->mode_config.connector_list_lock);
if (connector->tile_group) {
drm_mode_put_tile_group(dev, connector->tile_group);
connector->tile_group = NULL;
@@ -981,9 +984,42 @@ void drm_connector_cleanup(struct drm_connector *connector)
connector->state);
memset(connector, 0, sizeof(*connector));
+ mutex_unlock(&dev->mode_config.connector_list_lock);
}
EXPORT_SYMBOL(drm_connector_cleanup);
+static void drm_connector_free_cb(struct rcu_head *head)
+{
+ struct drm_connector *connector = container_of(head,
+ struct drm_connector,
+ free_head);
+
+ kfree(connector);
+}
+
+/**
+ * drm_connector_free - frees a connector structure
+ * @connector: connector to free
+ *
+ * This frees @connector using call_srcu() and kfree(). If the driver subclasses
+ * the connector, then the embedded struct &drm_connector must be the first
+ * element, and @connector must have been allocated using kmalloc() or one of
+ * the functions wrapping that.
+ *
+ * Delayed freeing is required to avoid use-after-free when walking the
+ * connector list, which is srcu protected. Hence drivers must use this function
+ * for connectors which can be removed while the driver is loaded. Currently
+ * that's only true for DP MST connectors. For any other connectors it is valid
+ * to call kfree directly.
+ */
+void drm_connector_free(struct drm_connector *connector)
+{
+ call_srcu(&drm_connector_list_srcu,
+ &connector->free_head,
+ drm_connector_free_cb);
+}
+EXPORT_SYMBOL(drm_connector_free);
+
/**
* drm_connector_register - register a connector
* @connector: the connector to register
@@ -5554,6 +5590,7 @@ void drm_mode_config_init(struct drm_device *dev)
mutex_init(&dev->mode_config.idr_mutex);
mutex_init(&dev->mode_config.fb_lock);
mutex_init(&dev->mode_config.blob_lock);
+ mutex_init(&dev->mode_config.connector_list_lock);
INIT_LIST_HEAD(&dev->mode_config.fb_list);
INIT_LIST_HEAD(&dev->mode_config.crtc_list);
INIT_LIST_HEAD(&dev->mode_config.connector_list);
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 9646816604be..11bbbb194f2d 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -327,7 +327,7 @@ intel_dp_mst_connector_destroy(struct drm_connector *connector)
kfree(intel_connector->edid);
drm_connector_cleanup(connector);
- kfree(connector);
+ drm_connector_free(connector);
}
static const struct drm_connector_funcs intel_dp_mst_connector_funcs = {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index ba574b9c1ec4..8f4380ac113e 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -32,6 +32,7 @@
#include <linux/fb.h>
#include <linux/hdmi.h>
#include <linux/media-bus-format.h>
+#include <linux/srcu.h>
#include <uapi/drm/drm_mode.h>
#include <uapi/drm/drm_fourcc.h>
#include <drm/drm_modeset_lock.h>
@@ -1186,6 +1187,7 @@ struct drm_encoder {
* @kdev: kernel device for sysfs attributes
* @attr: sysfs attributes
* @head: list management
+ * @free_head: rcu delayed freeing management
* @base: base KMS object
* @name: human readable name, can be overwritten by the driver
* @connector_id: compacted connector id useful indexing arrays
@@ -1241,6 +1243,7 @@ struct drm_connector {
struct device *kdev;
struct device_attribute *attr;
struct list_head head;
+ struct rcu_head free_head;
struct drm_mode_object base;
@@ -2309,7 +2312,7 @@ struct drm_mode_config {
* @tile_idr:
*
* Use this idr for allocating new IDs for tiled sinks like use in some
- * high-res DP MST screens.
+ * high-res DP MST screens. Protected by @idr_mutex.
*/
struct idr tile_idr;
@@ -2320,6 +2323,14 @@ struct drm_mode_config {
int num_connector;
struct ida connector_ida;
struct list_head connector_list;
+
+ /**
+ * @connector_list_lock:
+ *
+ * Protects @connector_list, @connector_ida and * @num_conncetors.
+ */
+ struct mutex connector_list_lock;
+
int num_encoder;
struct list_head encoder_list;
@@ -2426,6 +2437,8 @@ struct drm_mode_config {
struct drm_mode_config_helper_funcs *helper_private;
};
+extern struct srcu_struct drm_connector_list_srcu;
+
/**
* drm_for_each_plane_mask - iterate over planes specified by bitmask
* @plane: the loop cursor
@@ -2505,6 +2518,7 @@ int drm_connector_register(struct drm_connector *connector);
void drm_connector_unregister(struct drm_connector *connector);
extern void drm_connector_cleanup(struct drm_connector *connector);
+extern void drm_connector_free(struct drm_connector *connector);
static inline unsigned drm_connector_index(struct drm_connector *connector)
{
return connector->connector_id;
@@ -2835,31 +2849,57 @@ static inline void drm_connector_unreference(struct drm_connector *connector)
#define drm_for_each_crtc(crtc, dev) \
list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head)
+struct drm_connector_iter {
+ struct drm_mode_config *mode_config;
+ struct drm_connector *connector;
+ int srcu_ret;
+};
+
static inline void
-assert_drm_connector_list_read_locked(struct drm_mode_config *mode_config)
+__drm_connector_iter_init(struct drm_mode_config *mode_config,
+ struct drm_connector_iter *iter)
{
- /*
- * The connector hotadd/remove code currently grabs both locks when
- * updating lists. Hence readers need only hold either of them to be
- * safe and the check amounts to
- *
- * WARN_ON(not_holding(A) && not_holding(B)).
- */
- WARN_ON(!mutex_is_locked(&mode_config->mutex) &&
- !drm_modeset_is_locked(&mode_config->connection_mutex));
+ iter->mode_config = mode_config;
+ iter->srcu_ret = srcu_read_lock(&drm_connector_list_srcu);
+ iter->connector = list_entry_rcu(mode_config->connector_list.next,
+ struct drm_connector, head);
}
-struct drm_connector_iter {
- struct drm_mode_config *mode_config;
-};
+static inline struct drm_connector *
+__drm_connector_iter_check(struct drm_connector_iter *iter)
+{
+retry:
+ if (&iter->connector->head == &iter->mode_config->connector_list) {
+ /* last iteration, clean up */
+ srcu_read_unlock(&drm_connector_list_srcu, iter->srcu_ret);
+ return NULL;
+ }
+
+ /* srcu protects against iter->connector disappearing */
+ if (!kref_get_unless_zero(&iter->connector->base.refcount)) {
+ iter->connector = list_entry_rcu(iter->connector->head.next,
+ struct drm_connector, head);
+ goto retry;
+ }
+
+ return iter->connector;
+}
+
+static inline void
+__drm_connector_iter_next(struct drm_connector_iter *iter)
+{
+ /* _next() is only called when _check() succeeded on iter->connector,
+ * and _check() only succeeds if kref_get_unless_zero() succeeded.
+ * Therefore dropping the reference here is the correct place. */
+ drm_connector_unreference(iter->connector);
+ iter->connector = list_entry_rcu(iter->connector->head.next,
+ struct drm_connector, head);
+}
-#define drm_for_each_connector(connector, dev, iter) \
- for ((iter).mode_config = NULL, \
- assert_drm_connector_list_read_locked(&(dev)->mode_config), \
- connector = list_first_entry(&(dev)->mode_config.connector_list, \
- struct drm_connector, head); \
- &connector->head != (&(dev)->mode_config.connector_list); \
- connector = list_next_entry(connector, head))
+#define drm_for_each_connector(connector, dev, iter) \
+ for (__drm_connector_iter_init(&(dev)->mode_config, &(iter)); \
+ (connector = __drm_connector_iter_check(&(iter))); \
+ __drm_connector_iter_next(&(iter)))
#define drm_for_each_encoder(encoder, dev) \
list_for_each_entry(encoder, &(dev)->mode_config.encoder_list, head)
--
2.8.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
