Re: [PATCH 01/10] drm: crc: Introduce verify_crc_source callback

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: dri-devel@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org, Mahesh Kumar <mahesh1.kumar@intel.com>
Subject: Re: [PATCH 01/10] drm: crc: Introduce verify_crc_source callback
Date: Tue, 10 Jul 2018 14:26:35 +0300	[thread overview]
Message-ID: <10563672.RUzMss5xrO@avalon> (raw)
In-Reply-To: <20180702110729.8171-2-mahesh1.kumar@intel.com>

Hi Mahesh,

Thank you for the patch.

On Monday, 2 July 2018 14:07:20 EEST Mahesh Kumar wrote:
> This patch adds a new callback function "verify_crc_source" which will
> be used during setting the crc source in control node and while opening
> data node for crc reading. This will help in avoiding setting of wrong
> string for source.

Why do you need to verify the source in the open() operation ? Isn't it enough 
to verify it in the write() handler ? The answer to this question might lie in 
patch 08/10, in which case I'd add the .verify_crc_source() call in open() in 
that patch, not here.

> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++++++
>  include/drm/drm_crtc.h            | 15 +++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> b/drivers/gpu/drm/drm_debugfs_crc.c index 9f8312137cad..c6a725b79ac6 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const
> char __user *ubuf, struct drm_crtc *crtc = m->private;
>  	struct drm_crtc_crc *crc = &crtc->crc;
>  	char *source;
> +	size_t values_cnt;
> +	int ret = 0;
> 
>  	if (len == 0)
>  		return 0;
> @@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file,
> const char __user *ubuf, if (source[len] == '\n')
>  		source[len] = '\0';
> 
> +	if (crtc->funcs->verify_crc_source) {
> +		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	spin_lock_irq(&crc->lock);
> 
>  	if (crc->opened) {
> @@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct
> file *filep) return ret;
>  	}
> 
> +	if (crtc->funcs->verify_crc_source) {
> +		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
> +						     &values_cnt);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	spin_lock_irq(&crc->lock);
>  	if (!crc->opened)
>  		crc->opened = true;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 23eddbccab10..1a6dcbf91744 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -661,6 +661,21 @@ struct drm_crtc_funcs {
>  	 */
>  	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
>  			      size_t *values_cnt);
> +	/**
> +	 * @verify_crc_source:
> +	 *
> +	 * verifies the source of CRC checksums of frames before setting the
> +	 * source for CRC and during crc open.
> +	 *
> +	 * This callback is optional if the driver does not support any CRC
> +	 * generation functionality.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success or a negative error code on failure.
> +	 */
> +	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
> +				 size_t *values_cnt);
> 
>  	/**
>  	 * @atomic_print_state:

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel