[01/10] drm: crc: Introduce verify_crc_source callback
diff mbox

Message ID 20180702110729.8171-2-mahesh1.kumar@intel.com
State New
Headers show

Commit Message

Kumar, Mahesh July 2, 2018, 11:07 a.m. UTC
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.

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(+)

Comments

Laurent Pinchart July 10, 2018, 11:26 a.m. UTC | #1
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:
Kumar, Mahesh July 10, 2018, 11:54 a.m. UTC | #2
Hi,

Thanks for the review,
On 7/10/2018 4:56 PM, Laurent Pinchart wrote:
> 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.
Yes, final goal is achieved by patch 08/10. But if crc_read is called 
before setting the source, it may have wrong source set in that case I 
wanted to return early at least for the drivers implemented 
verify_crc_source. If you still think otherwise I will modify 
accordingly in next series.

-Mahesh

>
>> 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:
>
Laurent Pinchart July 10, 2018, 12:10 p.m. UTC | #3
Hi Mahesh,

On Tuesday, 10 July 2018 14:54:11 EEST Kumar, Mahesh wrote:
> On 7/10/2018 4:56 PM, Laurent Pinchart wrote:
> > 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.
> 
> Yes, final goal is achieved by patch 08/10. But if crc_read is called
> before setting the source, it may have wrong source set in that case I
> wanted to return early at least for the drivers implemented
> verify_crc_source. If you still think otherwise I will modify
> accordingly in next series.

I don't disagree with you, but I don't think this issue is new. As I got a bit 
confused as to why the call is needed in open() (there's no explanation in 
this patch), I think fixing the problem in 08/10 would be better.

> >> 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:
Kumar, Mahesh July 10, 2018, 12:12 p.m. UTC | #4
Hi,


On 7/10/2018 5:40 PM, Laurent Pinchart wrote:
> Hi Mahesh,
>
> On Tuesday, 10 July 2018 14:54:11 EEST Kumar, Mahesh wrote:
>> On 7/10/2018 4:56 PM, Laurent Pinchart wrote:
>>> 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.
>> Yes, final goal is achieved by patch 08/10. But if crc_read is called
>> before setting the source, it may have wrong source set in that case I
>> wanted to return early at least for the drivers implemented
>> verify_crc_source. If you still think otherwise I will modify
>> accordingly in next series.
> I don't disagree with you, but I don't think this issue is new. As I got a bit
> confused as to why the call is needed in open() (there's no explanation in
> this patch), I think fixing the problem in 08/10 would be better.
ok sure :)
-Mahesh
>
>>>> 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:
>

Patch
diff mbox

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: