Re: [Intel-gfx] [PATCH] drm/crc: Handle opening and closing crc better

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;
  

•   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.11.0

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx