Hi Javier Am 24.07.22 um 14:37 schrieb Javier Martinez Canillas:
DRM drivers initialize the mode configuration with drmm_mode_config_init() and that function (among other things) initializes mutexes that are later used by modeset helpers. But the helpers should only attempt to grab those locks if the mode config was properly initialized. Otherwise it can lead to kernel oops. An example is when a DRM driver using the component framework does not initialize the drm_mode_config, because its .bind callback was not being executed due one of its expected sub-devices' driver failing to probe. Some drivers check the struct drm_driver.registered field as an indication on whether their .shutdown callback should call helpers to tearn down the mode configuration or not, but most drivers just assume that it is always safe to call helpers such as drm_atomic_helper_shutdown() during shutdown. Let make the DRM core more robust and prevent this to happen, by marking a struct drm_mode_config as initialized during drmm_mode_config_init(). that way helpers can check for it and not attempt to grab uninitialized mutexes.
I disagree. This patch looks like cargo-cult programming and entirely arbitrary. The solution here is to fix drivers. The actual test to perform is to instrument the mutex implementation to detect uninitialized mutexes.
Best regards Thomas
Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> --- drivers/gpu/drm/drm_mode_config.c | 4 ++++ drivers/gpu/drm/drm_modeset_lock.c | 6 ++++++ include/drm/drm_mode_config.h | 8 ++++++++ 3 files changed, 18 insertions(+) diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 59b34f07cfce..db649f97120b 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -456,6 +456,8 @@ int drmm_mode_config_init(struct drm_device *dev) dma_resv_fini(&resv); }+ dev->mode_config.initialized = true;+ return drmm_add_action_or_reset(dev, drm_mode_config_init_release, NULL); } @@ -549,6 +551,8 @@ void drm_mode_config_cleanup(struct drm_device *dev) idr_destroy(&dev->mode_config.tile_idr); idr_destroy(&dev->mode_config.object_idr); drm_modeset_lock_fini(&dev->mode_config.connection_mutex); + + dev->mode_config.initialized = false; } EXPORT_SYMBOL(drm_mode_config_cleanup);diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.cindex 918065982db4..d6a81cb88123 100644 --- a/drivers/gpu/drm/drm_modeset_lock.c +++ b/drivers/gpu/drm/drm_modeset_lock.c @@ -444,6 +444,9 @@ EXPORT_SYMBOL(drm_modeset_unlock); * * See also: DRM_MODESET_LOCK_ALL_BEGIN() and DRM_MODESET_LOCK_ALL_END() * + * This function must only be called after drmm_mode_config_init(), since it + * takes locks that are initialized as part of the initial mode configuration. + * * Returns: 0 on success or a negative error-code on failure. */ int drm_modeset_lock_all_ctx(struct drm_device *dev, @@ -454,6 +457,9 @@ int drm_modeset_lock_all_ctx(struct drm_device *dev, struct drm_plane *plane; int ret;+ if (WARN_ON(!dev->mode_config.initialized))+ return -EINVAL; + ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx); if (ret) return ret; diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 6b5e01295348..d2e1a6d7dcc2 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -954,6 +954,14 @@ struct drm_mode_config { struct drm_atomic_state *suspend_state;const struct drm_mode_config_helper_funcs *helper_private;+ + /** + * @initialized: + * + * Internally used by modeset helpers such as drm_modeset_lock_all_ctx() + * to determine if the mode configuration has been properly initialized. + */ + bool initialized; };int __must_check drmm_mode_config_init(struct drm_device *dev);
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
