[PATCH] drm: actually remove the newline for CRC source name.

[PATCH] drm: actually remove the newline for CRC source name.

[Dingchen Zhang]

'n-1' is the index to replace the newline character of CRC source name.

Cc: Leo Li <sunpeng.li@amd.com>
Cc: Harry Wentland<Harry.Wentland@amd.com>
Signed-off-by: Dingchen Zhang <dingchen.zhang@amd.com>
---
 drivers/gpu/drm/drm_debugfs_crc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 585169f0dcc5..e20adef9d623 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -131,8 +131,8 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	if (IS_ERR(source))
 		return PTR_ERR(source);
 
-	if (source[len] == '\n')
-		source[len] = '\0';
+	if (source[len-1] == '\n')
+		source[len-1] = '\0';
 
 	ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
 	if (ret)
-- 
2.17.1

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

Re: [PATCH] drm: actually remove the newline for CRC source name.

[Sam Ravnborg]

Hi Dingchen.

Thanks for the quick follow-up.

On Wed, Jun 05, 2019 at 02:35:56PM -0400, Dingchen Zhang wrote:
> 'n-1' is the index to replace the newline character of CRC source name.

subject is much better now.
It would be fine if the body of the changelog conveyed the same message.
The body explains what the patch does, it is better to focus on why the
patch does what is do.

So maybe a short explanation that userspace may transfer a newine, and
that this terminating newline is replaced by a '\0' to avoid followup
isses.

> 
> Cc: Leo Li <sunpeng.li@amd.com>
> Cc: Harry Wentland<Harry.Wentland@amd.com>
Please add a space after the name, before the '<'.
This is also suggested by checkpatch.pl.


> Signed-off-by: Dingchen Zhang <dingchen.zhang@amd.com>
> ---
>  drivers/gpu/drm/drm_debugfs_crc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 585169f0dcc5..e20adef9d623 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -131,8 +131,8 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
>  	if (IS_ERR(source))
>  		return PTR_ERR(source);
>  
> -	if (source[len] == '\n')
> -		source[len] = '\0';
> +	if (source[len-1] == '\n')
> +		source[len-1] = '\0';
You did not add spaces around operators as requested.

Whith the above things fixed:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

>  
>  	ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
>  	if (ret)
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: actually remove the newline for CRC source name.

[Harry Wentland]

Thanks for the quick follow-up to Sam.

Drop the word "actually" from the patch subject line.

It's generally helpful to generate a 2nd version of the patch with '-v
2', and to leave a description what v2 changed.

Also CC anyone who previously commented.

On 2019-06-05 2:35 p.m., Dingchen Zhang wrote:
> 'n-1' is the index to replace the newline character of CRC source name.
> 

Add here:
v2: Update patch subject (Sam)

> Cc: Leo Li <sunpeng.li@amd.com>
> Cc: Harry Wentland<Harry.Wentland@amd.com>
> Signed-off-by: Dingchen Zhang <dingchen.zhang@amd.com>
> ---
>  drivers/gpu/drm/drm_debugfs_crc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 585169f0dcc5..e20adef9d623 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -131,8 +131,8 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
>  	if (IS_ERR(source))
>  		return PTR_ERR(source);
>  
> -	if (source[len] == '\n')
> -		source[len] = '\0';
> +	if (source[len-1] == '\n')
> +		source[len-1] = '\0';
>  

As Sam mentioned, you'll want this to be

+	if (source[len - 1] == '\n')
+		source[len - 1] = '\0';

I forgot to mention this to you before, but please run
./scripts/checkpatch.pl on your patches before sending them and fix up
any errors or warnings.

Harry

>  	ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
>  	if (ret)
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel