From eb42ea6d0b8ed9ca8e73cc24fa801f0d8ab28905 Mon Sep 17 00:00:00 2001 From: Maarten Lankhorst Date: Wed, 21 Jun 2017 13:00:07 +0200 Subject: [PATCH] drm/crc: Handle opening and closing crc better When I was doing a grep . -r /sys/kernel/debug/dri/0 I noticed a WARN appearing when I aborted the grep with ^C. After investigating I've also noticed that the error handling was lacking and there are race conditions involving multiple calls to open/close simultaneously. Fix this by setting the opened flag first and using crc->entries to decide when crc can be collected. Also call unset crc source before cleaning up, this way there is no race with a future open(). This patch has been tested with all the tests in igt with CRC in their name. Signed-off-by: Maarten Lankhorst Link: http://patchwork.freedesktop.org/patch/msgid/20170621110007.11674-1-maarten.lankhorst@linux.intel.com Reviewed-by: Tomeu Vizoso [mlankhorst: Add description that this patch has been tested with IGT, based on tomeu's feedback] --- drivers/gpu/drm/drm_debugfs_crc.c | 46 +++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c index 1722d8f21449..d0ea4627a093 100644 --- a/drivers/gpu/drm/drm_debugfs_crc.c +++ b/drivers/gpu/drm/drm_debugfs_crc.c @@ -136,21 +136,38 @@ static int crtc_crc_data_count(struct drm_crtc_crc *crc) return CIRC_CNT(crc->head, crc->tail, DRM_CRC_ENTRIES_NR); } +static void crtc_crc_cleanup(struct drm_crtc_crc *crc) +{ + kfree(crc->entries); + crc->entries = NULL; + crc->head = 0; + crc->tail = 0; + crc->values_cnt = 0; + crc->opened = false; +} + static int crtc_crc_open(struct inode *inode, struct file *filep) { struct drm_crtc *crtc = inode->i_private; struct drm_crtc_crc *crc = &crtc->crc; struct drm_crtc_crc_entry *entries = NULL; size_t values_cnt; - int ret; + int ret = 0; - if (crc->opened) - return -EBUSY; + spin_lock_irq(&crc->lock); + if (!crc->opened) + crc->opened = true; + else + ret = -EBUSY; + spin_unlock_irq(&crc->lock); - ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt); if (ret) return ret; + ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt); + if (ret) + goto err; + if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) { ret = -EINVAL; goto err_disable; @@ -170,7 +187,6 @@ static int crtc_crc_open(struct inode *inode, struct file *filep) spin_lock_irq(&crc->lock); crc->entries = entries; crc->values_cnt = values_cnt; - crc->opened = true; /* * Only return once we got a first frame, so userspace doesn't have to @@ -182,12 +198,17 @@ static int crtc_crc_open(struct inode *inode, struct file *filep) crc->lock); spin_unlock_irq(&crc->lock); - WARN_ON(ret); + if (ret) + goto err_disable; return 0; err_disable: crtc->funcs->set_crc_source(crtc, NULL, &values_cnt); +err: + spin_lock_irq(&crc->lock); + crtc_crc_cleanup(crc); + spin_unlock_irq(&crc->lock); return ret; } @@ -197,17 +218,12 @@ static int crtc_crc_release(struct inode *inode, struct file *filep) struct drm_crtc_crc *crc = &crtc->crc; size_t values_cnt; + crtc->funcs->set_crc_source(crtc, NULL, &values_cnt); + spin_lock_irq(&crc->lock); - kfree(crc->entries); - crc->entries = NULL; - crc->head = 0; - crc->tail = 0; - crc->values_cnt = 0; - crc->opened = false; + crtc_crc_cleanup(crc); spin_unlock_irq(&crc->lock); - crtc->funcs->set_crc_source(crtc, NULL, &values_cnt); - return 0; } @@ -334,7 +350,7 @@ int drm_crtc_add_crc_entry(struct drm_crtc *crtc, bool has_frame, spin_lock(&crc->lock); /* Caller may not have noticed yet that userspace has stopped reading */ - if (!crc->opened) { + if (!crc->entries) { spin_unlock(&crc->lock); return -EINVAL; } -- 2.20.1