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

[Tomeu Vizoso]

Looks good to me:

Reviewed-by: Tomeu Vizoso <tomeu.vizoso at 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 at 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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx