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().
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- 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; }
Looks good to me:
Reviewed-by: Tomeu Vizoso tomeu.vizoso@collabora.com
I guess you have tested this with IGT? In any case, I think it would be good to mention how a patch has been tested in the changelog. That can be very useful to others if things go wrong at some point.
Thanks,
Tomeu
On 21 June 2017 at 13:00, Maarten Lankhorst maarten.lankhorst@linux.intel.com wrote:
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().
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
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;elseret = -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.11.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 06-07-17 om 13:09 schreef Tomeu Vizoso:
Looks good to me:
Reviewed-by: Tomeu Vizoso tomeu.vizoso@collabora.com
I guess you have tested this with IGT? In any case, I think it would be good to mention how a patch has been tested in the changelog. That can be very useful to others if things go wrong at some point.
Testcase: debugfs_test.read_all_entries
But I hit it by doing a recursive grep, which I guess is the same thing here. :)
One further improvement I wanted to do was reject opening the CRC with -EIO when the crtc is not active, that way the above test will not hang. Does the below patch also look good to you?
----8<----- Commit e8fa5671183c ("drm: crc: Wait for a frame before returning from open()") adds a wait for CRC frame, but with the CRTC off this will never be generated. For atomic drivers we know if a CRTC is active through crtc_state->active, so when inactive reject the open with -EIO.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Fixes: e8fa5671183c ("drm: crc: Wait for a frame before returning from open()") Testcase: debugfs_test.read_all_entries --- diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c index d0ea4627a093..f9e26dda56d6 100644 --- a/drivers/gpu/drm/drm_debugfs_crc.c +++ b/drivers/gpu/drm/drm_debugfs_crc.c @@ -154,6 +154,19 @@ static int crtc_crc_open(struct inode *inode, struct file *filep) size_t values_cnt; int ret = 0;
+ if (drm_drv_uses_atomic_modeset(crtc->dev)) { + ret = drm_modeset_lock_interruptible(&crtc->mutex, NULL); + if (ret) + return ret; + + if (!crtc->state->active) + ret = -EIO; + drm_modeset_unlock(&crtc->mutex); + + if (ret) + return ret; + } + spin_lock_irq(&crc->lock); if (!crc->opened) crc->opened = true;
On Thu, Jul 06, 2017 at 03:03:15PM +0200, Maarten Lankhorst wrote:
Op 06-07-17 om 13:09 schreef Tomeu Vizoso:
Looks good to me:
Reviewed-by: Tomeu Vizoso tomeu.vizoso@collabora.com
I guess you have tested this with IGT? In any case, I think it would be good to mention how a patch has been tested in the changelog. That can be very useful to others if things go wrong at some point.
Testcase: debugfs_test.read_all_entries
But I hit it by doing a recursive grep, which I guess is the same thing here. :)
One further improvement I wanted to do was reject opening the CRC with -EIO when the crtc is not active, that way the above test will not hang. Does the below patch also look good to you?
----8<----- Commit e8fa5671183c ("drm: crc: Wait for a frame before returning from open()") adds a wait for CRC frame, but with the CRTC off this will never be generated. For atomic drivers we know if a CRTC is active through crtc_state->active, so when inactive reject the open with -EIO.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Fixes: e8fa5671183c ("drm: crc: Wait for a frame before returning from open()") Testcase: debugfs_test.read_all_entries
At least for the semantics I think this makes sense. Opening the CRC file when the crtc is off is undefined.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
But pls get Tomeu's ack too.
Thanks, Daniel
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c index d0ea4627a093..f9e26dda56d6 100644 --- a/drivers/gpu/drm/drm_debugfs_crc.c +++ b/drivers/gpu/drm/drm_debugfs_crc.c @@ -154,6 +154,19 @@ static int crtc_crc_open(struct inode *inode, struct file *filep) size_t values_cnt; int ret = 0;
- if (drm_drv_uses_atomic_modeset(crtc->dev)) {
ret = drm_modeset_lock_interruptible(&crtc->mutex, NULL);if (ret)return ret;if (!crtc->state->active)ret = -EIO;drm_modeset_unlock(&crtc->mutex);if (ret)return ret;- }
- spin_lock_irq(&crc->lock); if (!crc->opened) crc->opened = true;
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 07-07-17 om 13:01 schreef Daniel Vetter:
On Thu, Jul 06, 2017 at 03:03:15PM +0200, Maarten Lankhorst wrote:
Op 06-07-17 om 13:09 schreef Tomeu Vizoso:
Looks good to me:
Reviewed-by: Tomeu Vizoso tomeu.vizoso@collabora.com
I guess you have tested this with IGT? In any case, I think it would be good to mention how a patch has been tested in the changelog. That can be very useful to others if things go wrong at some point.
Testcase: debugfs_test.read_all_entries
But I hit it by doing a recursive grep, which I guess is the same thing here. :)
One further improvement I wanted to do was reject opening the CRC with -EIO when the crtc is not active, that way the above test will not hang. Does the below patch also look good to you?
----8<----- Commit e8fa5671183c ("drm: crc: Wait for a frame before returning from open()") adds a wait for CRC frame, but with the CRTC off this will never be generated. For atomic drivers we know if a CRTC is active through crtc_state->active, so when inactive reject the open with -EIO.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Fixes: e8fa5671183c ("drm: crc: Wait for a frame before returning from open()") Testcase: debugfs_test.read_all_entries
At least for the semantics I think this makes sense. Opening the CRC file when the crtc is off is undefined.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
But pls get Tomeu's ack too.
Tomeu, can you ack? :)
I did some testing on IGT with both patches applied on all tests with CRC in their name, no problems with opening CRC as far as I can see, and all tests except kms_ccs and kms_mmap_write_crc succeed. The former needs render compression, the latter fails on a crc failure, so it can't have been caused by this patch.
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Maarten Lankhorst -
Tomeu Vizoso