Re: [PATCH 1/2] drm: Provide a drm_dev_release_barrier() function to wait for device release callbacks

["Christian König" <christian.koenig@amd.com>]

On 3/16/26 17:20, Thomas Hellström wrote:
> If helper components, like for example drm_pagemap hold references to
> drm devices, it's typically possible for the drm driver module to be
> unloaded without that reference being dropped, 

That is an extremely bad idea to begin with, drm_devices should reference the module who issued them.

> resulting in execution out
> of freed memory. Such components are therefore required to hold a
> module refcount on the module that created the drm device, and ensure
> that module reference is dropped after all references to the drm
> device are dropped.
> 
> To relax that, drivers can keep a drm device-count and ensure that the
> module isn't unloaded until the drm device-count has dropped to zero
> and that the caller that decremented the last device-count has finished
> executing driver callbacks.

Stop for a second. The question is why would anybody want to relax that?

That unbinding a driver from a device is separated from module unloading is a design we had in Linux since the very beginning.

Regards,
Christian.

> 
> To help with the latter, add a drm_dev_release_barrier() function.
> The function ensures that any caller that has started executing
> device release callbacks has also finished executing them.
> 
> Use SRCU for the implementation.
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_drv.c | 25 +++++++++++++++++++++++++
>  include/drm/drm_drv.h     |  1 +
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 2915118436ce..9fa72adf173d 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -898,21 +898,46 @@ struct drm_device *drm_dev_alloc(const struct drm_driver *driver,
>  }
>  EXPORT_SYMBOL(drm_dev_alloc);
>  
> +DEFINE_STATIC_SRCU(drm_dev_release_srcu);
> +
>  static void drm_dev_release(struct kref *ref)
>  {
>  	struct drm_device *dev = container_of(ref, struct drm_device, ref);
> +	int idx;
>  
>  	/* Just in case register/unregister was never called */
>  	drm_debugfs_dev_fini(dev);
>  
> +	idx = srcu_read_lock(&drm_dev_release_srcu);
>  	if (dev->driver->release)
>  		dev->driver->release(dev);
>  
>  	drm_managed_release(dev);
> +	srcu_read_unlock(&drm_dev_release_srcu, idx);
>  
>  	kfree(dev->managed.final_kfree);
>  }
>  
> +/**
> + * drm_dev_release_barrier() - Ensure drm device release callbacks are finished
> + *
> + * If a device release method or any of the drm managed release callbacks
> + * have been called for a device, wait until all of them have finished
> + * executing. This function can be used to help determine whether it's safe
> + * to unload a driver module.
> + *
> + * Assume for example the driver maintains a device count which is decremented
> + * using a drmm callback or a device release callback. From a drm device
> + * lifetime POV, it's then safe to unload the driver when that device-count
> + * has reached zero and drm_dev_release_barrier() has been called.
> + */
> +void drm_dev_release_barrier(void)
> +{
> +	synchronize_srcu(&drm_dev_release_srcu);
> +}
> +EXPORT_SYMBOL(drm_dev_release_barrier);
> +
> +
>  /**
>   * drm_dev_get - Take reference of a DRM device
>   * @dev: device to take reference of or NULL
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 42fc085f986d..b288a885cf45 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -489,6 +489,7 @@ void drm_dev_exit(int idx);
>  void drm_dev_unplug(struct drm_device *dev);
>  int drm_dev_wedged_event(struct drm_device *dev, unsigned long method,
>  			 struct drm_wedge_task_info *info);
> +void drm_dev_release_barrier(void);
>  
>  /**
>   * drm_dev_is_unplugged - is a DRM device unplugged