[PATCH/RFC 1/3] drm: Move legacy device list out of drm_driver

[Daniel Vetter]

On Sat, Feb 22, 2020 at 05:24:28PM +0200, Laurent Pinchart wrote:
> The drm_driver structure contains a single field (legacy_dev_list) that
> is modified by the DRM core, used to store a linked list of legacy DRM
> devices associated with the driver. In order to make the structure
> const, move the field out to a global variable. This requires locking
> access to the global where the local field didn't require serialization,
> but this only affects legacy drivers, and isn't in any hot path.
> 
> While at it, compile-out the legacy_dev_list field when DRM_LEGACY isn't
> defined.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas at ideasonboard.com>
> ---
>  drivers/gpu/drm/drm_pci.c | 30 ++++++++++++++++++++++--------
>  include/drm/drm_device.h  |  2 ++
>  include/drm/drm_drv.h     |  2 --
>  3 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
> index c6bb98729a26..44805ac3177c 100644
> --- a/drivers/gpu/drm/drm_pci.c
> +++ b/drivers/gpu/drm/drm_pci.c
> @@ -24,6 +24,8 @@
>  
>  #include <linux/dma-mapping.h>
>  #include <linux/export.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
>  #include <linux/pci.h>
>  #include <linux/slab.h>
>  
> @@ -36,6 +38,12 @@
>  #include "drm_internal.h"
>  #include "drm_legacy.h"
>  
> +#ifdef CONFIG_DRM_LEGACY
> +/* List of devices hanging off drivers with stealth attach. */
> +static LIST_HEAD(legacy_dev_list);
> +static DEFINE_MUTEX(legacy_dev_list_lock);
> +#endif
> +
>  /**
>   * drm_pci_alloc - Allocate a PCI consistent memory block, for DMA.
>   * @dev: DRM device
> @@ -236,10 +244,13 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
>  	if (ret)
>  		goto err_agp;
>  
> -	/* No locking needed since shadow-attach is single-threaded since it may
> -	 * only be called from the per-driver module init hook. */
> -	if (drm_core_check_feature(dev, DRIVER_LEGACY))
> -		list_add_tail(&dev->legacy_dev_list, &driver->legacy_dev_list);
> +#ifdef CONFIG_DRM_LEGACY
> +	if (drm_core_check_feature(dev, DRIVER_LEGACY)) {
> +		mutex_lock(&legacy_dev_list_lock);
> +		list_add_tail(&dev->legacy_dev_list, &legacy_dev_list);
> +		mutex_unlock(&legacy_dev_list_lock);
> +	}
> +#endif
>  
>  	return 0;
>  
> @@ -275,7 +286,6 @@ int drm_legacy_pci_init(struct drm_driver *driver, struct pci_driver *pdriver)
>  		return -EINVAL;
>  
>  	/* If not using KMS, fall back to stealth mode manual scanning. */
> -	INIT_LIST_HEAD(&driver->legacy_dev_list);
>  	for (i = 0; pdriver->id_table[i].vendor != 0; i++) {
>  		pid = &pdriver->id_table[i];
>  
> @@ -317,11 +327,15 @@ void drm_legacy_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver)
>  	if (!(driver->driver_features & DRIVER_LEGACY)) {
>  		WARN_ON(1);
>  	} else {
> -		list_for_each_entry_safe(dev, tmp, &driver->legacy_dev_list,
> +		mutex_lock(&legacy_dev_list_lock);
> +		list_for_each_entry_safe(dev, tmp, &legacy_dev_list,
>  					 legacy_dev_list) {
> -			list_del(&dev->legacy_dev_list);
> -			drm_put_dev(dev);
> +			if (dev->driver == driver) {
> +				list_del(&dev->legacy_dev_list);
> +				drm_put_dev(dev);

I checked whether this would result in any issues with the new mutex_lock,
but with the legacy model there's no hotunplug or anything like that, so
we never need to remove ourselves from this list coming from the other
direction. We just oops :-)

> +			}
>  		}
> +		mutex_unlock(&legacy_dev_list_lock);
>  	}
>  	DRM_INFO("Module unloaded\n");
>  }
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index bb60a949f416..215b3472c773 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -51,12 +51,14 @@ enum switch_power_state {
>   * may contain multiple heads.
>   */
>  struct drm_device {
> +#ifdef CONFIG_DRM_LEGACY
>  	/**
>  	 * @legacy_dev_list:
>  	 *
>  	 * List of devices per driver for stealth attach cleanup
>  	 */
>  	struct list_head legacy_dev_list;
> +#endif

We have a CONFIG_DRM_LEGACY dungeon already at the end of this struct, can
you pls move it there? Also drop the kerneldoc comment, we want to hide
this for good :-)

With that tiny bikeshed:

Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

>  
>  	/** @if_version: Highest interface version set */
>  	int if_version;
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 97109df5beac..7dcf3b7bb5e6 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -601,8 +601,6 @@ struct drm_driver {
>  	/* Everything below here is for legacy driver, never use! */
>  	/* private: */
>  
> -	/* List of devices hanging off this driver with stealth attach. */
> -	struct list_head legacy_dev_list;
>  	int (*firstopen) (struct drm_device *);
>  	void (*preclose) (struct drm_device *, struct drm_file *file_priv);
>  	int (*dma_ioctl) (struct drm_device *dev, void *data, struct drm_file *file_priv);
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch