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]
return CIRC_CNT(crc->head, crc->tail, DRM_CRC_ENTRIES_NR);
}
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;
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;
- 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);
+ 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;
if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) {
ret = -EINVAL;
goto err_disable;
spin_lock_irq(&crc->lock);
crc->entries = entries;
crc->values_cnt = values_cnt;
spin_lock_irq(&crc->lock);
crc->entries = entries;
crc->values_cnt = values_cnt;
/*
* Only return once we got a first frame, so userspace doesn't have to
/*
* Only return once we got a first frame, so userspace doesn't have to
crc->lock);
spin_unlock_irq(&crc->lock);
crc->lock);
spin_unlock_irq(&crc->lock);
+ if (ret)
+ goto err_disable;
return 0;
err_disable:
crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
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);
struct drm_crtc_crc *crc = &crtc->crc;
size_t values_cnt;
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);
spin_lock_irq(&crc->lock);
- kfree(crc->entries);
- crc->entries = NULL;
- crc->head = 0;
- crc->tail = 0;
- crc->values_cnt = 0;
- crc->opened = false;
spin_unlock_irq(&crc->lock);
spin_unlock_irq(&crc->lock);
- crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
-
spin_lock(&crc->lock);
/* Caller may not have noticed yet that userspace has stopped reading */
spin_lock(&crc->lock);
/* Caller may not have noticed yet that userspace has stopped reading */
spin_unlock(&crc->lock);
return -EINVAL;
}
spin_unlock(&crc->lock);
return -EINVAL;
}