| Message ID | 20161213230814.19598-9-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 14, 2016 at 12:08:09AM +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! > > 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> > --- > drivers/gpu/drm/drm_connector.c | 18 +++++++++++++----- > include/drm/drm_connector.h | 12 +++++++++++- > 2 files changed, 24 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index b33334e09b00..0d4728704099 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); There's a mutex_destroy() for drm_connector_free() for that extra bit of debug icing. > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index 0e41a2e184a9..a24559ef8bb7 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -559,7 +559,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 +607,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 +626,10 @@ struct drm_connector { > bool interlace_allowed; > bool doublescan_allowed; > bool stereo_allowed; > + /** > + * @registered: Is this connector exposed (registered) with userspace? > + * Protected by @mutex. early_unregister, late_register should also be documented as being called under connector->mutex. With those minor improvements, Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Tue, Dec 13, 2016 at 6:08 PM, Daniel Vetter <daniel.vetter@ffwll.ch> 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! > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Reported-by: Chris Wilson <chris@chris-wilson.co.uk> With Chris' suggested improvements Reviewed-by: Sean Paul <seanpaul@chromium.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_connector.c | 18 +++++++++++++----- > include/drm/drm_connector.h | 12 +++++++++++- > 2 files changed, 24 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index b33334e09b00..0d4728704099 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; > > @@ -373,14 +374,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 +398,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 +418,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 +431,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..a24559ef8bb7 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -559,7 +559,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 +607,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 +626,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 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index b33334e09b00..0d4728704099 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; @@ -373,14 +374,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 +398,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 +418,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 +431,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..a24559ef8bb7 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -559,7 +559,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 +607,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 +626,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 */
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! 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> --- drivers/gpu/drm/drm_connector.c | 18 +++++++++++++----- include/drm/drm_connector.h | 12 +++++++++++- 2 files changed, 24 insertions(+), 6 deletions(-)