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 <maarten.lankhorst@linux.intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20170621110007.11674-1-maarten.lankhorst@linux.intel.com
Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
[mlankhorst: Add description that this patch has been tested with IGT,
based on tomeu's feedback]

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 1722d8f214499794d5304dc7e9597bfcd0dadd6b..d0ea4627a093677dbf1ea157db26a67aaf2822b2 100644 (file)
--- 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;
        }