| Message ID | 20161218133545.2106-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Dec 18, 2016 at 02:35:45PM +0100, Daniel Vetter wrote: > If we're unlucky then the registration from a hotplugged connector > might race with the final registration step on driver load. And since > MST topology discover is asynchronous that's even somewhat likely. > > v2: Also update the kerneldoc for @registered! > > v3: Review from Chris: > - Improve kerneldoc for late_register/early_unregister callbacks. > - Use mutex_destroy. > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Reported-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Oops, that was supposed to be in-reply-to v2. -Daniel > --- > drivers/gpu/drm/drm_connector.c | 20 +++++++++++++++----- > include/drm/drm_connector.h | 16 +++++++++++++++- > 2 files changed, 30 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index b33334e09b00..5906775b1fa8 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -223,6 +223,7 @@ int drm_connector_init(struct drm_device *dev, > > INIT_LIST_HEAD(&connector->probed_modes); > INIT_LIST_HEAD(&connector->modes); > + mutex_init(&connector->mutex); > connector->edid_blob_ptr = NULL; > connector->status = connector_status_unknown; > > @@ -358,6 +359,8 @@ void drm_connector_cleanup(struct drm_connector *connector) > connector->funcs->atomic_destroy_state(connector, > connector->state); > > + mutex_destroy(&connector->mutex); > + > memset(connector, 0, sizeof(*connector)); > } > EXPORT_SYMBOL(drm_connector_cleanup); > @@ -373,14 +376,15 @@ EXPORT_SYMBOL(drm_connector_cleanup); > */ > int drm_connector_register(struct drm_connector *connector) > { > - int ret; > + int ret = 0; > > + mutex_lock(&connector->mutex); > if (connector->registered) > - return 0; > + goto unlock; > > ret = drm_sysfs_connector_add(connector); > if (ret) > - return ret; > + goto unlock; > > ret = drm_debugfs_connector_add(connector); > if (ret) { > @@ -396,12 +400,14 @@ int drm_connector_register(struct drm_connector *connector) > drm_mode_object_register(connector->dev, &connector->base); > > connector->registered = true; > - return 0; > + goto unlock; > > err_debugfs: > drm_debugfs_connector_remove(connector); > err_sysfs: > drm_sysfs_connector_remove(connector); > +unlock: > + mutex_unlock(&connector->mutex); > return ret; > } > EXPORT_SYMBOL(drm_connector_register); > @@ -414,8 +420,11 @@ EXPORT_SYMBOL(drm_connector_register); > */ > void drm_connector_unregister(struct drm_connector *connector) > { > - if (!connector->registered) > + mutex_lock(&connector->mutex); > + if (!connector->registered) { > + mutex_unlock(&connector->mutex); > return; > + } > > if (connector->funcs->early_unregister) > connector->funcs->early_unregister(connector); > @@ -424,6 +433,7 @@ void drm_connector_unregister(struct drm_connector *connector) > drm_debugfs_connector_remove(connector); > > connector->registered = false; > + mutex_unlock(&connector->mutex); > } > EXPORT_SYMBOL(drm_connector_unregister); > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index 0e41a2e184a9..e4f23cc2aaab 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -381,6 +381,8 @@ struct drm_connector_funcs { > * core drm connector interfaces. Everything added from this callback > * should be unregistered in the early_unregister callback. > * > + * This is called while holding drm_connector->mutex. > + * > * Returns: > * > * 0 on success, or a negative error code on failure. > @@ -395,6 +397,8 @@ struct drm_connector_funcs { > * late_register(). It is called from drm_connector_unregister(), > * early in the driver unload sequence to disable userspace access > * before data structures are torndown. > + * > + * This is called while holding drm_connector->mutex. > */ > void (*early_unregister)(struct drm_connector *connector); > > @@ -559,7 +563,6 @@ struct drm_cmdline_mode { > * @interlace_allowed: can this connector handle interlaced modes? > * @doublescan_allowed: can this connector handle doublescan? > * @stereo_allowed: can this connector handle stereo modes? > - * @registered: is this connector exposed (registered) with userspace? > * @modes: modes available on this connector (from fill_modes() + user) > * @status: one of the drm_connector_status enums (connected, not, or unknown) > * @probed_modes: list of modes derived directly from the display > @@ -608,6 +611,13 @@ struct drm_connector { > char *name; > > /** > + * @mutex: Lock for general connector state, but currently only protects > + * @registered. Most of the connector state is still protected by the > + * mutex in &drm_mode_config. > + */ > + struct mutex mutex; > + > + /** > * @index: Compacted connector index, which matches the position inside > * the mode_config.list for drivers not supporting hot-add/removing. Can > * be used as an array index. It is invariant over the lifetime of the > @@ -620,6 +630,10 @@ struct drm_connector { > bool interlace_allowed; > bool doublescan_allowed; > bool stereo_allowed; > + /** > + * @registered: Is this connector exposed (registered) with userspace? > + * Protected by @mutex. > + */ > bool registered; > struct list_head modes; /* list of modes on this connector */ > > -- > 2.11.0 >
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index b33334e09b00..5906775b1fa8 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -223,6 +223,7 @@ int drm_connector_init(struct drm_device *dev, INIT_LIST_HEAD(&connector->probed_modes); INIT_LIST_HEAD(&connector->modes); + mutex_init(&connector->mutex); connector->edid_blob_ptr = NULL; connector->status = connector_status_unknown; @@ -358,6 +359,8 @@ void drm_connector_cleanup(struct drm_connector *connector) connector->funcs->atomic_destroy_state(connector, connector->state); + mutex_destroy(&connector->mutex); + memset(connector, 0, sizeof(*connector)); } EXPORT_SYMBOL(drm_connector_cleanup); @@ -373,14 +376,15 @@ EXPORT_SYMBOL(drm_connector_cleanup); */ int drm_connector_register(struct drm_connector *connector) { - int ret; + int ret = 0; + mutex_lock(&connector->mutex); if (connector->registered) - return 0; + goto unlock; ret = drm_sysfs_connector_add(connector); if (ret) - return ret; + goto unlock; ret = drm_debugfs_connector_add(connector); if (ret) { @@ -396,12 +400,14 @@ int drm_connector_register(struct drm_connector *connector) drm_mode_object_register(connector->dev, &connector->base); connector->registered = true; - return 0; + goto unlock; err_debugfs: drm_debugfs_connector_remove(connector); err_sysfs: drm_sysfs_connector_remove(connector); +unlock: + mutex_unlock(&connector->mutex); return ret; } EXPORT_SYMBOL(drm_connector_register); @@ -414,8 +420,11 @@ EXPORT_SYMBOL(drm_connector_register); */ void drm_connector_unregister(struct drm_connector *connector) { - if (!connector->registered) + mutex_lock(&connector->mutex); + if (!connector->registered) { + mutex_unlock(&connector->mutex); return; + } if (connector->funcs->early_unregister) connector->funcs->early_unregister(connector); @@ -424,6 +433,7 @@ void drm_connector_unregister(struct drm_connector *connector) drm_debugfs_connector_remove(connector); connector->registered = false; + mutex_unlock(&connector->mutex); } EXPORT_SYMBOL(drm_connector_unregister); diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 0e41a2e184a9..e4f23cc2aaab 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -381,6 +381,8 @@ struct drm_connector_funcs { * core drm connector interfaces. Everything added from this callback * should be unregistered in the early_unregister callback. * + * This is called while holding drm_connector->mutex. + * * Returns: * * 0 on success, or a negative error code on failure. @@ -395,6 +397,8 @@ struct drm_connector_funcs { * late_register(). It is called from drm_connector_unregister(), * early in the driver unload sequence to disable userspace access * before data structures are torndown. + * + * This is called while holding drm_connector->mutex. */ void (*early_unregister)(struct drm_connector *connector); @@ -559,7 +563,6 @@ struct drm_cmdline_mode { * @interlace_allowed: can this connector handle interlaced modes? * @doublescan_allowed: can this connector handle doublescan? * @stereo_allowed: can this connector handle stereo modes? - * @registered: is this connector exposed (registered) with userspace? * @modes: modes available on this connector (from fill_modes() + user) * @status: one of the drm_connector_status enums (connected, not, or unknown) * @probed_modes: list of modes derived directly from the display @@ -608,6 +611,13 @@ struct drm_connector { char *name; /** + * @mutex: Lock for general connector state, but currently only protects + * @registered. Most of the connector state is still protected by the + * mutex in &drm_mode_config. + */ + struct mutex mutex; + + /** * @index: Compacted connector index, which matches the position inside * the mode_config.list for drivers not supporting hot-add/removing. Can * be used as an array index. It is invariant over the lifetime of the @@ -620,6 +630,10 @@ struct drm_connector { bool interlace_allowed; bool doublescan_allowed; bool stereo_allowed; + /** + * @registered: Is this connector exposed (registered) with userspace? + * Protected by @mutex. + */ bool registered; struct list_head modes; /* list of modes on this connector */