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@xxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxx>
>
> /** @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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
