diff mbox series

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

Message ID 20200222152430.2984-2-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series Constify drm_driver | expand

Commit Message

Laurent Pinchart Feb. 22, 2020, 3:24 p.m. UTC 
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@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(-)

Comments

Daniel Vetter Feb. 22, 2020, 5:58 p.m. UTC | #1 
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@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@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
>
diff mbox series

Patch

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);
+			}
 		}
+		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
 
 	/** @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);