commit 4f5368b5541a902f6596558b05f5c21a9770dd32 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Fri Jun 14 08:17:23 2019 +0200
drm/kms: Catch mode_object lifetime errors
uncovered a bit a mess in dp drivers. Most drivers (from a quick look, all except i915) register all the dp stuff in their init code, which is too early. With CONFIG_DRM_DP_AUX_CHARDEV this will blow up, because drm_dp_aux_register tries to add a child to a device in sysfs (the connector) which doesn't even exist yet.
No one seems to have cared thus far. But with the above change I also moved the setting of dev->registered after the ->load callback, in an attempt to keep old drivers from hitting any WARN_ON backtraces. But that moved radeon.ko from the "working, by accident" to "now also broken" category.
Since this is a huge mess I figured a revert would be simplest. But this check has already caught issues in i915:
commit 1b9bd09630d4db4827cc04d358a41a16a6bc2cb0 Author: Ville Syrjälä ville.syrjala@linux.intel.com Date: Tue Aug 20 19:16:57 2019 +0300
drm/i915: Do not create a new max_bpc prop for MST connectors
Hence I'd like to retain it. Fix the radeon regression by moving the setting of dev->registered back to were it was, and stop the backtraces with an explicit check for dev->driver->load.
Everyone else will stay as broken with CONFIG_DRM_DP_AUX_CHARDEV. The next patch will improve the kerneldoc and add a todo entry for this.
Fixes: 4f5368b5541a ("drm/kms: Catch mode_object lifetime errors") Cc: Sean Paul sean@poorly.run Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reported-by: Michel Dänzer michel@daenzer.net Cc: Michel Dänzer michel@daenzer.net Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_drv.c | 4 ++-- drivers/gpu/drm/drm_mode_object.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index b634882056c8..0c9978af9f72 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -983,14 +983,14 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) if (ret) goto err_minors;
+ dev->registered = true; + if (dev->driver->load) { ret = dev->driver->load(dev, flags); if (ret) goto err_minors; }
- dev->registered = true; - if (drm_core_check_feature(dev, DRIVER_MODESET)) drm_modeset_register_all(dev);
diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c index c355ba8e6d5d..6a23e36ed4fe 100644 --- a/drivers/gpu/drm/drm_mode_object.c +++ b/drivers/gpu/drm/drm_mode_object.c @@ -42,7 +42,7 @@ int __drm_mode_object_add(struct drm_device *dev, struct drm_mode_object *obj, { int ret;
- WARN_ON(dev->registered && !obj_free_cb); + WARN_ON(!dev->driver->load && dev->registered && !obj_free_cb);
mutex_lock(&dev->mode_config.idr_mutex); ret = idr_alloc(&dev->mode_config.object_idr, register_obj ? obj : NULL, @@ -104,7 +104,7 @@ void drm_mode_object_register(struct drm_device *dev, void drm_mode_object_unregister(struct drm_device *dev, struct drm_mode_object *object) { - WARN_ON(dev->registered && !object->free_cb); + WARN_ON(!dev->driver->load && dev->registered && !object->free_cb);
mutex_lock(&dev->mode_config.idr_mutex); if (object->id) {
Current code is quite a mess unfortunately, so also add a todo.rst entry to maybe fix it up eventually.
Cc: Michel Dänzer michel@daenzer.net Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- Documentation/gpu/todo.rst | 12 ++++++++++++ drivers/gpu/drm/drm_connector.c | 10 ++++++++-- drivers/gpu/drm/drm_dp_helper.c | 8 ++++++++ 3 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 32787acff0a8..8dc147c93c9c 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -284,6 +284,18 @@ drm_fb_helper tasks removed: drm_fb_helper_single_add_all_connectors(), drm_fb_helper_add_one_connector() and drm_fb_helper_remove_one_connector().
+connector register/unregister fixes +----------------------------------- + +- For most connectors it's a no-op to call drm_connector_register/unregister + directly from driver code, drm_dev_register/unregister take care of this + already. We can remove all of them. + +- For dp drivers it's a bit more a mess, since we need the connector to be + registered when calling drm_dp_aux_register. Fix this by instead calling + drm_dp_aux_init, and moving the actual registering into a late_register + callback as recommended in the kerneldoc. + Core refactorings =================
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 4c766624b20d..cd5048ceab1d 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -480,7 +480,10 @@ EXPORT_SYMBOL(drm_connector_cleanup); * drm_connector_register - register a connector * @connector: the connector to register * - * Register userspace interfaces for a connector + * Register userspace interfaces for a connector. Only call this for connectors + * which can be hotplugged after drm_dev_register() has been called already, + * e.g. DP MST connectors. All other connectors will be registered automatically + * when calling drm_dev_register(). * * Returns: * Zero on success, error code on failure. @@ -526,7 +529,10 @@ EXPORT_SYMBOL(drm_connector_register); * drm_connector_unregister - unregister a connector * @connector: the connector to unregister * - * Unregister userspace interfaces for a connector + * Unregister userspace interfaces for a connector. Only call this for + * connectors which have registered explicitly by calling drm_dev_register(), + * since connectors are unregistered automatically when drm_dev_unregister() is + * called. */ void drm_connector_unregister(struct drm_connector *connector) { diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index ffc68d305afe..f373798d82f6 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -1109,6 +1109,14 @@ EXPORT_SYMBOL(drm_dp_aux_init); * @aux: DisplayPort AUX channel * * Automatically calls drm_dp_aux_init() if this hasn't been done yet. + * This should only be called when the underlying &struct drm_connector is + * initialiazed already. Therefore the best place to call this is from + * &drm_connector_funcs.late_register. Not that drivers which don't follow this + * will Oops when CONFIG_DRM_DP_AUX_CHARDEV is enabled. + * + * Drivers which need to use the aux channel before that point (e.g. at driver + * load time, before drm_dev_register() has been called) need to call + * drm_dp_aux_init(). * * Returns 0 on success or a negative error code on failure. */
Reviewed-by: Lyude Paul lyude@redhat.com
On Tue, 2019-09-17 at 14:09 +0200, Daniel Vetter wrote:
Current code is quite a mess unfortunately, so also add a todo.rst entry to maybe fix it up eventually.
Cc: Michel Dänzer michel@daenzer.net Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Documentation/gpu/todo.rst | 12 ++++++++++++ drivers/gpu/drm/drm_connector.c | 10 ++++++++-- drivers/gpu/drm/drm_dp_helper.c | 8 ++++++++ 3 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 32787acff0a8..8dc147c93c9c 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -284,6 +284,18 @@ drm_fb_helper tasks removed: drm_fb_helper_single_add_all_connectors(), drm_fb_helper_add_one_connector() and drm_fb_helper_remove_one_connector().
+connector register/unregister fixes +-----------------------------------
+- For most connectors it's a no-op to call drm_connector_register/unregister
- directly from driver code, drm_dev_register/unregister take care of this
- already. We can remove all of them.
+- For dp drivers it's a bit more a mess, since we need the connector to be
- registered when calling drm_dp_aux_register. Fix this by instead calling
- drm_dp_aux_init, and moving the actual registering into a late_register
- callback as recommended in the kerneldoc.
Core refactorings
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 4c766624b20d..cd5048ceab1d 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -480,7 +480,10 @@ EXPORT_SYMBOL(drm_connector_cleanup);
- drm_connector_register - register a connector
- @connector: the connector to register
- Register userspace interfaces for a connector
- Register userspace interfaces for a connector. Only call this for
connectors
- which can be hotplugged after drm_dev_register() has been called
already,
- e.g. DP MST connectors. All other connectors will be registered
automatically
- when calling drm_dev_register().
- Returns:
- Zero on success, error code on failure.
@@ -526,7 +529,10 @@ EXPORT_SYMBOL(drm_connector_register);
- drm_connector_unregister - unregister a connector
- @connector: the connector to unregister
- Unregister userspace interfaces for a connector
- Unregister userspace interfaces for a connector. Only call this for
- connectors which have registered explicitly by calling
drm_dev_register(),
- since connectors are unregistered automatically when
drm_dev_unregister() is
*/
- called.
void drm_connector_unregister(struct drm_connector *connector) { diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index ffc68d305afe..f373798d82f6 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -1109,6 +1109,14 @@ EXPORT_SYMBOL(drm_dp_aux_init);
- @aux: DisplayPort AUX channel
- Automatically calls drm_dp_aux_init() if this hasn't been done yet.
- This should only be called when the underlying &struct drm_connector is
- initialiazed already. Therefore the best place to call this is from
- &drm_connector_funcs.late_register. Not that drivers which don't follow
this
- will Oops when CONFIG_DRM_DP_AUX_CHARDEV is enabled.
- Drivers which need to use the aux channel before that point (e.g. at
driver
- load time, before drm_dev_register() has been called) need to call
*/
- drm_dp_aux_init().
- Returns 0 on success or a negative error code on failure.
On Tue, Sep 17, 2019 at 03:40:29PM -0400, Lyude Paul wrote:
Reviewed-by: Lyude Paul lyude@redhat.com
Thanks for taking a look, pushed to drm-misc-next. -Daniel
On Tue, 2019-09-17 at 14:09 +0200, Daniel Vetter wrote:
Current code is quite a mess unfortunately, so also add a todo.rst entry to maybe fix it up eventually.
Cc: Michel Dänzer michel@daenzer.net Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Documentation/gpu/todo.rst | 12 ++++++++++++ drivers/gpu/drm/drm_connector.c | 10 ++++++++-- drivers/gpu/drm/drm_dp_helper.c | 8 ++++++++ 3 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 32787acff0a8..8dc147c93c9c 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -284,6 +284,18 @@ drm_fb_helper tasks removed: drm_fb_helper_single_add_all_connectors(), drm_fb_helper_add_one_connector() and drm_fb_helper_remove_one_connector().
+connector register/unregister fixes +-----------------------------------
+- For most connectors it's a no-op to call drm_connector_register/unregister
- directly from driver code, drm_dev_register/unregister take care of this
- already. We can remove all of them.
+- For dp drivers it's a bit more a mess, since we need the connector to be
- registered when calling drm_dp_aux_register. Fix this by instead calling
- drm_dp_aux_init, and moving the actual registering into a late_register
- callback as recommended in the kerneldoc.
Core refactorings
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 4c766624b20d..cd5048ceab1d 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -480,7 +480,10 @@ EXPORT_SYMBOL(drm_connector_cleanup);
- drm_connector_register - register a connector
- @connector: the connector to register
- Register userspace interfaces for a connector
- Register userspace interfaces for a connector. Only call this for
connectors
- which can be hotplugged after drm_dev_register() has been called
already,
- e.g. DP MST connectors. All other connectors will be registered
automatically
- when calling drm_dev_register().
- Returns:
- Zero on success, error code on failure.
@@ -526,7 +529,10 @@ EXPORT_SYMBOL(drm_connector_register);
- drm_connector_unregister - unregister a connector
- @connector: the connector to unregister
- Unregister userspace interfaces for a connector
- Unregister userspace interfaces for a connector. Only call this for
- connectors which have registered explicitly by calling
drm_dev_register(),
- since connectors are unregistered automatically when
drm_dev_unregister() is
*/
- called.
void drm_connector_unregister(struct drm_connector *connector) { diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index ffc68d305afe..f373798d82f6 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -1109,6 +1109,14 @@ EXPORT_SYMBOL(drm_dp_aux_init);
- @aux: DisplayPort AUX channel
- Automatically calls drm_dp_aux_init() if this hasn't been done yet.
- This should only be called when the underlying &struct drm_connector is
- initialiazed already. Therefore the best place to call this is from
- &drm_connector_funcs.late_register. Not that drivers which don't follow
this
- will Oops when CONFIG_DRM_DP_AUX_CHARDEV is enabled.
- Drivers which need to use the aux channel before that point (e.g. at
driver
- load time, before drm_dev_register() has been called) need to call
*/
- drm_dp_aux_init().
- Returns 0 on success or a negative error code on failure.
-- Cheers, Lyude Paul
On 2019-09-17 2:09 p.m., Daniel Vetter wrote:
commit 4f5368b5541a902f6596558b05f5c21a9770dd32 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Fri Jun 14 08:17:23 2019 +0200
drm/kms: Catch mode_object lifetime errorsuncovered a bit a mess in dp drivers. Most drivers (from a quick look, all except i915) register all the dp stuff in their init code, which is too early. With CONFIG_DRM_DP_AUX_CHARDEV this will blow up, because drm_dp_aux_register tries to add a child to a device in sysfs (the connector) which doesn't even exist yet.
No one seems to have cared thus far. But with the above change I also moved the setting of dev->registered after the ->load callback, in an attempt to keep old drivers from hitting any WARN_ON backtraces. But that moved radeon.ko from the "working, by accident" to "now also broken" category.
Since this is a huge mess I figured a revert would be simplest. But this check has already caught issues in i915:
commit 1b9bd09630d4db4827cc04d358a41a16a6bc2cb0 Author: Ville Syrjälä ville.syrjala@linux.intel.com Date: Tue Aug 20 19:16:57 2019 +0300
drm/i915: Do not create a new max_bpc prop for MST connectorsHence I'd like to retain it. Fix the radeon regression by moving the setting of dev->registered back to were it was, and stop the backtraces with an explicit check for dev->driver->load.
Everyone else will stay as broken with CONFIG_DRM_DP_AUX_CHARDEV. The next patch will improve the kerneldoc and add a todo entry for this.
Fixes: 4f5368b5541a ("drm/kms: Catch mode_object lifetime errors") Cc: Sean Paul sean@poorly.run Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reported-by: Michel Dänzer michel@daenzer.net Cc: Michel Dänzer michel@daenzer.net Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Reviewed-by: Michel Dänzer mdaenzer@redhat.com Tested-by: Michel Dänzer mdaenzer@redhat.com
Thanks!
On Tue, Sep 17, 2019 at 04:48:51PM +0200, Michel Dänzer wrote:
On 2019-09-17 2:09 p.m., Daniel Vetter wrote:
commit 4f5368b5541a902f6596558b05f5c21a9770dd32 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Fri Jun 14 08:17:23 2019 +0200
drm/kms: Catch mode_object lifetime errorsuncovered a bit a mess in dp drivers. Most drivers (from a quick look, all except i915) register all the dp stuff in their init code, which is too early. With CONFIG_DRM_DP_AUX_CHARDEV this will blow up, because drm_dp_aux_register tries to add a child to a device in sysfs (the connector) which doesn't even exist yet.
No one seems to have cared thus far. But with the above change I also moved the setting of dev->registered after the ->load callback, in an attempt to keep old drivers from hitting any WARN_ON backtraces. But that moved radeon.ko from the "working, by accident" to "now also broken" category.
Since this is a huge mess I figured a revert would be simplest. But this check has already caught issues in i915:
commit 1b9bd09630d4db4827cc04d358a41a16a6bc2cb0 Author: Ville Syrjälä ville.syrjala@linux.intel.com Date: Tue Aug 20 19:16:57 2019 +0300
drm/i915: Do not create a new max_bpc prop for MST connectorsHence I'd like to retain it. Fix the radeon regression by moving the setting of dev->registered back to were it was, and stop the backtraces with an explicit check for dev->driver->load.
Everyone else will stay as broken with CONFIG_DRM_DP_AUX_CHARDEV. The next patch will improve the kerneldoc and add a todo entry for this.
Fixes: 4f5368b5541a ("drm/kms: Catch mode_object lifetime errors") Cc: Sean Paul sean@poorly.run Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reported-by: Michel Dänzer michel@daenzer.net Cc: Michel Dänzer michel@daenzer.net Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Reviewed-by: Michel Dänzer mdaenzer@redhat.com Tested-by: Michel Dänzer mdaenzer@redhat.com
Thanks, merged into drm-misc-next-fixes. -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
Lyude Paul -
Michel Dänzer