[PATCH 0/3] drm/{ast, mgag200}: Protect I/O regs against concurrent access

[PATCH 0/3] drm/{ast, mgag200}: Protect I/O regs against concurrent access

[Thomas Zimmermann]

Protect access to I/O registers in ast and mgag200 via lock. Commit-
tail functions and get-modes operations use the same registers and can
interfere with each other. This can result in failed mode-setting
operations.

As both drivers use fully managed cleanup, the patchset adds a new helper
that initializes a mutex with auto-cleanup.

Thomas Zimmermann (3):
  drm: Add DRM-managed mutex_init()
  drm/ast: Protect concurrent access to I/O registers with lock
  drm/mgag200: Protect concurrent access to I/O registers with lock

 drivers/gpu/drm/ast/ast_drv.h          |  1 +
 drivers/gpu/drm/ast/ast_main.c         |  4 +++
 drivers/gpu/drm/ast/ast_mode.c         | 48 ++++++++++++++++++++++++--
 drivers/gpu/drm/drm_managed.c          | 27 +++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_drv.c  |  6 ++++
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 +
 drivers/gpu/drm/mgag200/mgag200_mode.c | 14 ++++++++
 include/drm/drm_managed.h              |  3 ++
 8 files changed, 101 insertions(+), 3 deletions(-)


base-commit: 3ae2e00290c290713e21118220a817a24b44d39f
prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
-- 
2.36.0

[PATCH 1/3] drm: Add DRM-managed mutex_init()

[Thomas Zimmermann]

Add drmm_mutex_init(), a helper that provides managed mutex cleanup. The
mutex will be destroyed with the final reference of the DRM device.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_managed.c | 27 +++++++++++++++++++++++++++
 include/drm/drm_managed.h     |  3 +++
 2 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c
index 37d7db6223be6..4cf214de50c40 100644
--- a/drivers/gpu/drm/drm_managed.c
+++ b/drivers/gpu/drm/drm_managed.c
@@ -8,6 +8,7 @@
 #include <drm/drm_managed.h>
 
 #include <linux/list.h>
+#include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
@@ -262,3 +263,29 @@ void drmm_kfree(struct drm_device *dev, void *data)
 	free_dr(dr_match);
 }
 EXPORT_SYMBOL(drmm_kfree);
+
+static void drmm_mutex_release(struct drm_device *dev, void *res)
+{
+	struct mutex *lock = res;
+
+	mutex_destroy(lock);
+}
+
+/**
+ * drmm_mutex_init - &drm_device-managed mutex_init()
+ * @dev: DRM device
+ * @lock: lock to be initialized
+ *
+ * Returns:
+ * 0 on success, or a negative errno code otherwise.
+ *
+ * This is a &drm_device-managed version of mutex_init(). The initialized
+ * lock is automatically destroyed on the final drm_dev_put().
+ */
+int drmm_mutex_init(struct drm_device *dev, struct mutex *lock)
+{
+	mutex_init(lock);
+
+	return drmm_add_action_or_reset(dev, drmm_mutex_release, lock);
+}
+EXPORT_SYMBOL(drmm_mutex_init);
diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
index b45c6fbf53ac4..359883942612e 100644
--- a/include/drm/drm_managed.h
+++ b/include/drm/drm_managed.h
@@ -8,6 +8,7 @@
 #include <linux/types.h>
 
 struct drm_device;
+struct mutex;
 
 typedef void (*drmres_release_t)(struct drm_device *dev, void *res);
 
@@ -104,4 +105,6 @@ char *drmm_kstrdup(struct drm_device *dev, const char *s, gfp_t gfp);
 
 void drmm_kfree(struct drm_device *dev, void *data);
 
+int drmm_mutex_init(struct drm_device *dev, struct mutex *lock);
+
 #endif
-- 
2.36.0

Re: [PATCH 1/3] drm: Add DRM-managed mutex_init()

[Daniel Vetter]

On Mon, May 02, 2022 at 04:25:12PM +0200, Thomas Zimmermann wrote:
> Add drmm_mutex_init(), a helper that provides managed mutex cleanup. The
> mutex will be destroyed with the final reference of the DRM device.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> on this one, ack for
the other two driver patches.
-Daniel

> ---
>  drivers/gpu/drm/drm_managed.c | 27 +++++++++++++++++++++++++++
>  include/drm/drm_managed.h     |  3 +++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c
> index 37d7db6223be6..4cf214de50c40 100644
> --- a/drivers/gpu/drm/drm_managed.c
> +++ b/drivers/gpu/drm/drm_managed.c
> @@ -8,6 +8,7 @@
>  #include <drm/drm_managed.h>
>  
>  #include <linux/list.h>
> +#include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  
> @@ -262,3 +263,29 @@ void drmm_kfree(struct drm_device *dev, void *data)
>  	free_dr(dr_match);
>  }
>  EXPORT_SYMBOL(drmm_kfree);
> +
> +static void drmm_mutex_release(struct drm_device *dev, void *res)
> +{
> +	struct mutex *lock = res;
> +
> +	mutex_destroy(lock);
> +}
> +
> +/**
> + * drmm_mutex_init - &drm_device-managed mutex_init()
> + * @dev: DRM device
> + * @lock: lock to be initialized
> + *
> + * Returns:
> + * 0 on success, or a negative errno code otherwise.
> + *
> + * This is a &drm_device-managed version of mutex_init(). The initialized
> + * lock is automatically destroyed on the final drm_dev_put().
> + */
> +int drmm_mutex_init(struct drm_device *dev, struct mutex *lock)
> +{
> +	mutex_init(lock);
> +
> +	return drmm_add_action_or_reset(dev, drmm_mutex_release, lock);
> +}
> +EXPORT_SYMBOL(drmm_mutex_init);
> diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
> index b45c6fbf53ac4..359883942612e 100644
> --- a/include/drm/drm_managed.h
> +++ b/include/drm/drm_managed.h
> @@ -8,6 +8,7 @@
>  #include <linux/types.h>
>  
>  struct drm_device;
> +struct mutex;
>  
>  typedef void (*drmres_release_t)(struct drm_device *dev, void *res);
>  
> @@ -104,4 +105,6 @@ char *drmm_kstrdup(struct drm_device *dev, const char *s, gfp_t gfp);
>  
>  void drmm_kfree(struct drm_device *dev, void *data);
>  
> +int drmm_mutex_init(struct drm_device *dev, struct mutex *lock);
> +
>  #endif
> -- 
> 2.36.0
> 

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

[PATCH 2/3] drm/ast: Protect concurrent access to I/O registers with lock

[Thomas Zimmermann]

Add a mutex lock to protect concurrent access to I/O registers
against each other. This happens between invokataion of commit-
tail functions and get-mode operations. Both with use the CRTC
index register AST_IO_CRTC_PORT. Concurrent access can lead to
failed mode-setting operations.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reported-by: KuoHsiang Chou <kuohsiang_chou@aspeedtech.com>
---
 drivers/gpu/drm/ast/ast_drv.h  |  1 +
 drivers/gpu/drm/ast/ast_main.c |  4 +++
 drivers/gpu/drm/ast/ast_mode.c | 48 +++++++++++++++++++++++++++++++---
 3 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index a19315b2f7e56..43cedd767f8f1 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -158,6 +158,7 @@ to_ast_sil164_connector(struct drm_connector *connector)
 struct ast_private {
 	struct drm_device base;
 
+	struct mutex ioregs_lock; /* Protects access to I/O registers in ioregs */
 	void __iomem *regs;
 	void __iomem *ioregs;
 	void __iomem *dp501_fw_buf;
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 22e9e2d3c71ab..5ad473aeaea2b 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -420,6 +420,10 @@ struct ast_private *ast_device_create(const struct drm_driver *drv,
 
 	pci_set_drvdata(pdev, dev);
 
+	ret = drmm_mutex_init(dev, &ast->ioregs_lock);
+	if (ret)
+		return ERR_PTR(ret);
+
 	ast->regs = pcim_iomap(pdev, 1, 0);
 	if (!ast->regs)
 		return ERR_PTR(-EIO);
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 45b56b39ad473..f7849638fcb7e 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1099,6 +1099,20 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc,
 	return 0;
 }
 
+static void ast_crtc_helper_atomic_begin(struct drm_crtc *crtc, struct drm_atomic_state *state)
+{
+	struct drm_device *dev = crtc->dev;
+	struct ast_private *ast = to_ast_private(dev);
+
+	/*
+	 * Concurrent operations could possibly trigger a call to
+	 * drm_connector_helper_funcs.get_modes by trying to read the
+	 * display modes. Protect access to I/O registers by acquiring
+	 * the I/O-register lock. Released in atomic_flush().
+	 */
+	mutex_lock(&ast->ioregs_lock);
+}
+
 static void
 ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
 			     struct drm_atomic_state *state)
@@ -1107,7 +1121,8 @@ ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
 									  crtc);
 	struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state,
 									      crtc);
-	struct ast_private *ast = to_ast_private(crtc->dev);
+	struct drm_device *dev = crtc->dev;
+	struct ast_private *ast = to_ast_private(dev);
 	struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc_state);
 	struct ast_crtc_state *old_ast_crtc_state = to_ast_crtc_state(old_crtc_state);
 
@@ -1117,6 +1132,8 @@ ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
 	 */
 	if (old_ast_crtc_state->format != ast_crtc_state->format)
 		ast_crtc_load_lut(ast, crtc);
+
+	mutex_unlock(&ast->ioregs_lock);
 }
 
 static void
@@ -1175,6 +1192,7 @@ ast_crtc_helper_atomic_disable(struct drm_crtc *crtc,
 static const struct drm_crtc_helper_funcs ast_crtc_helper_funcs = {
 	.mode_valid = ast_crtc_helper_mode_valid,
 	.atomic_check = ast_crtc_helper_atomic_check,
+	.atomic_begin = ast_crtc_helper_atomic_begin,
 	.atomic_flush = ast_crtc_helper_atomic_flush,
 	.atomic_enable = ast_crtc_helper_atomic_enable,
 	.atomic_disable = ast_crtc_helper_atomic_disable,
@@ -1260,21 +1278,33 @@ static int ast_crtc_init(struct drm_device *dev)
 static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
 {
 	struct ast_vga_connector *ast_vga_connector = to_ast_vga_connector(connector);
+	struct drm_device *dev = connector->dev;
+	struct ast_private *ast = to_ast_private(dev);
 	struct edid *edid;
 	int count;
 
 	if (!ast_vga_connector->i2c)
 		goto err_drm_connector_update_edid_property;
 
+	/*
+	 * Protect access to I/O registers from concurrent modesetting
+	 * by acquiring the I/O-register lock.
+	 */
+	mutex_lock(&ast->ioregs_lock);
+
 	edid = drm_get_edid(connector, &ast_vga_connector->i2c->adapter);
 	if (!edid)
-		goto err_drm_connector_update_edid_property;
+		goto err_mutex_unlock;
+
+	mutex_unlock(&ast->ioregs_lock);
 
 	count = drm_add_edid_modes(connector, edid);
 	kfree(edid);
 
 	return count;
 
+err_mutex_unlock:
+	mutex_unlock(&ast->ioregs_lock);
 err_drm_connector_update_edid_property:
 	drm_connector_update_edid_property(connector, NULL);
 	return 0;
@@ -1354,21 +1384,33 @@ static int ast_vga_output_init(struct ast_private *ast)
 static int ast_sil164_connector_helper_get_modes(struct drm_connector *connector)
 {
 	struct ast_sil164_connector *ast_sil164_connector = to_ast_sil164_connector(connector);
+	struct drm_device *dev = connector->dev;
+	struct ast_private *ast = to_ast_private(dev);
 	struct edid *edid;
 	int count;
 
 	if (!ast_sil164_connector->i2c)
 		goto err_drm_connector_update_edid_property;
 
+	/*
+	 * Protect access to I/O registers from concurrent modesetting
+	 * by acquiring the I/O-register lock.
+	 */
+	mutex_lock(&ast->ioregs_lock);
+
 	edid = drm_get_edid(connector, &ast_sil164_connector->i2c->adapter);
 	if (!edid)
-		goto err_drm_connector_update_edid_property;
+		goto err_mutex_unlock;
+
+	mutex_unlock(&ast->ioregs_lock);
 
 	count = drm_add_edid_modes(connector, edid);
 	kfree(edid);
 
 	return count;
 
+err_mutex_unlock:
+	mutex_unlock(&ast->ioregs_lock);
 err_drm_connector_update_edid_property:
 	drm_connector_update_edid_property(connector, NULL);
 	return 0;
-- 
2.36.0

[PATCH 3/3] drm/mgag200: Protect concurrent access to I/O registers with lock

[Thomas Zimmermann]

Add a mutex lock to protect concurrent access to I/O registers
against each other. This happens between invokataion of commit-
tail functions and get-mode operations. Both with use the CRTC
index registers MGA1064_GEN_IO_DATA and MGA1064_GEN_IO_CTL.
Concurrent access can lead to failed mode-setting operations.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c  |  6 ++++++
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 +
 drivers/gpu/drm/mgag200/mgag200_mode.c | 14 ++++++++++++++
 3 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 217844d71ab5c..08839460606fe 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -14,6 +14,7 @@
 #include <drm/drm_drv.h>
 #include <drm/drm_file.h>
 #include <drm/drm_ioctl.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_module.h>
 #include <drm/drm_pciids.h>
 
@@ -65,6 +66,11 @@ static int mgag200_regs_init(struct mga_device *mdev)
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	u32 option, option2;
 	u8 crtcext3;
+	int ret;
+
+	ret = drmm_mutex_init(dev, &mdev->rmmio_lock);
+	if (ret)
+		return ret;
 
 	switch (mdev->type) {
 	case G200_PCI:
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 4368112023f7c..18829eb8398a0 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -213,6 +213,7 @@ struct mga_device {
 	struct drm_device		base;
 	unsigned long			flags;
 
+	struct mutex			rmmio_lock;
 	resource_size_t			rmmio_base;
 	resource_size_t			rmmio_size;
 	void __iomem			*rmmio;
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 6e18d3bbd7207..abde7655477db 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -881,6 +881,14 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 		.y2 = fb->height,
 	};
 
+	/*
+	 * Concurrent operations could possibly trigger a call to
+	 * drm_connector_helper_funcs.get_modes by trying to read the
+	 * display modes. Protect access to I/O registers by acquiring
+	 * the I/O-register lock.
+	 */
+	mutex_lock(&mdev->rmmio_lock);
+
 	if (mdev->type == G200_WB || mdev->type == G200_EW3)
 		mgag200_g200wb_hold_bmc(mdev);
 
@@ -904,6 +912,8 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 	mgag200_enable_display(mdev);
 
 	mgag200_handle_damage(mdev, fb, &fullscreen, &shadow_plane_state->data[0]);
+
+	mutex_unlock(&mdev->rmmio_lock);
 }
 
 static void
@@ -963,8 +973,12 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
 	if (!fb)
 		return;
 
+	mutex_lock(&mdev->rmmio_lock);
+
 	if (drm_atomic_helper_damage_merged(old_state, state, &damage))
 		mgag200_handle_damage(mdev, fb, &damage, &shadow_plane_state->data[0]);
+
+	mutex_unlock(&mdev->rmmio_lock);
 }
 
 static struct drm_crtc_state *
-- 
2.36.0

Re: [PATCH 3/3] drm/mgag200: Protect concurrent access to I/O registers with lock

[Jocelyn Falempe]

On 02/05/2022 16:25, Thomas Zimmermann wrote:
> Add a mutex lock to protect concurrent access to I/O registers
> against each other. This happens between invokataion of commit-

Typo in commit message, invokataion => invocation

> tail functions and get-mode operations. Both with use the CRTC
> index registers MGA1064_GEN_IO_DATA and MGA1064_GEN_IO_CTL.
> Concurrent access can lead to failed mode-setting operations.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

There is a trivial conflict with 
https://lists.freedesktop.org/archives/dri-devel/2022-April/352966.html
But I will need to send a v2 for it anyway.

It looks good to me,
Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

> ---
>   drivers/gpu/drm/mgag200/mgag200_drv.c  |  6 ++++++
>   drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 +
>   drivers/gpu/drm/mgag200/mgag200_mode.c | 14 ++++++++++++++
>   3 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 217844d71ab5c..08839460606fe 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -14,6 +14,7 @@
>   #include <drm/drm_drv.h>
>   #include <drm/drm_file.h>
>   #include <drm/drm_ioctl.h>
> +#include <drm/drm_managed.h>
>   #include <drm/drm_module.h>
>   #include <drm/drm_pciids.h>
>   
> @@ -65,6 +66,11 @@ static int mgag200_regs_init(struct mga_device *mdev)
>   	struct pci_dev *pdev = to_pci_dev(dev->dev);
>   	u32 option, option2;
>   	u8 crtcext3;
> +	int ret;
> +
> +	ret = drmm_mutex_init(dev, &mdev->rmmio_lock);
> +	if (ret)
> +		return ret;
>   
>   	switch (mdev->type) {
>   	case G200_PCI:
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 4368112023f7c..18829eb8398a0 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -213,6 +213,7 @@ struct mga_device {
>   	struct drm_device		base;
>   	unsigned long			flags;
>   
> +	struct mutex			rmmio_lock;
>   	resource_size_t			rmmio_base;
>   	resource_size_t			rmmio_size;
>   	void __iomem			*rmmio;
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 6e18d3bbd7207..abde7655477db 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -881,6 +881,14 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
>   		.y2 = fb->height,
>   	};
>   
> +	/*
> +	 * Concurrent operations could possibly trigger a call to
> +	 * drm_connector_helper_funcs.get_modes by trying to read the
> +	 * display modes. Protect access to I/O registers by acquiring
> +	 * the I/O-register lock.
> +	 */
> +	mutex_lock(&mdev->rmmio_lock);
> +
>   	if (mdev->type == G200_WB || mdev->type == G200_EW3)
>   		mgag200_g200wb_hold_bmc(mdev);
>   
> @@ -904,6 +912,8 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
>   	mgag200_enable_display(mdev);
>   
>   	mgag200_handle_damage(mdev, fb, &fullscreen, &shadow_plane_state->data[0]);
> +
> +	mutex_unlock(&mdev->rmmio_lock);
>   }
>   
>   static void
> @@ -963,8 +973,12 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
>   	if (!fb)
>   		return;
>   
> +	mutex_lock(&mdev->rmmio_lock);
> +
>   	if (drm_atomic_helper_damage_merged(old_state, state, &damage))
>   		mgag200_handle_damage(mdev, fb, &damage, &shadow_plane_state->data[0]);
> +
> +	mutex_unlock(&mdev->rmmio_lock);
>   }
>   
>   static struct drm_crtc_state *