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
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
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
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;
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 *
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 *
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jocelyn Falempe -
Thomas Zimmermann