| Message ID | 20170621110007.11674-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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; }
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(-)