[v1] Fixed some issues and cleanup of ultrasoc-smb

[PATCH 2/3] coresight: ultrasoc-smb: simplify the code for check to_copy valid

Posted by Junhao He 1 year, 7 months ago

We only need to check once when before using the to_copy variable
to simplify the code.

Signed-off-by: Junhao He <hejunhao3@huawei.com>
---
 drivers/hwtracing/coresight/ultrasoc-smb.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
index b08a619d1116..e78edc3480ce 100644
--- a/drivers/hwtracing/coresight/ultrasoc-smb.c
+++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
@@ -127,20 +127,15 @@ static ssize_t smb_read(struct file *file, char __user *data, size_t len,
 					struct smb_drv_data, miscdev);
 	struct smb_data_buffer *sdb = &drvdata->sdb;
 	struct device *dev = &drvdata->csdev->dev;
-	ssize_t to_copy = 0;
-
-	if (!len)
-		return 0;
-
-	if (!sdb->data_size)
-		return 0;
-
-	to_copy = min(sdb->data_size, len);
+	ssize_t to_copy = min(sdb->data_size, len);
 
 	/* Copy parts of trace data when read pointer wrap around SMB buffer */
 	if (sdb->buf_rdptr + to_copy > sdb->buf_size)
 		to_copy = sdb->buf_size - sdb->buf_rdptr;
 
+	if (!to_copy)
+		return 0;
+
 	if (copy_to_user(data, sdb->buf_base + sdb->buf_rdptr, to_copy)) {
 		dev_dbg(dev, "Failed to copy data to user\n");
 		return -EFAULT;
-- 
2.33.0

Re: [PATCH 2/3] coresight: ultrasoc-smb: simplify the code for check to_copy valid

Posted by Jonathan Cameron 1 year, 6 months ago

On Thu, 12 Oct 2023 17:47:05 +0800
Junhao He <hejunhao3@huawei.com> wrote:

> We only need to check once when before using the to_copy variable
> to simplify the code.
> 
> Signed-off-by: Junhao He <hejunhao3@huawei.com>

I'm not convinced this one is an improvement. Sometimes it's easier to just
see the individual conditions checked even if we could combine them.
It's easy to understand we don't copy data if:
a) We ask for 0 data.
b) There is 0 data

Less easy to establish that with the extra wrap around code in there
(even though that has no impact on to_copy if it is 0)

Jonathan


> ---
>  drivers/hwtracing/coresight/ultrasoc-smb.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index b08a619d1116..e78edc3480ce 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -127,20 +127,15 @@ static ssize_t smb_read(struct file *file, char __user *data, size_t len,
>  					struct smb_drv_data, miscdev);
>  	struct smb_data_buffer *sdb = &drvdata->sdb;
>  	struct device *dev = &drvdata->csdev->dev;
> -	ssize_t to_copy = 0;
> -
> -	if (!len)
> -		return 0;
> -
> -	if (!sdb->data_size)
> -		return 0;
> -
> -	to_copy = min(sdb->data_size, len);
> +	ssize_t to_copy = min(sdb->data_size, len);
>  
>  	/* Copy parts of trace data when read pointer wrap around SMB buffer */
>  	if (sdb->buf_rdptr + to_copy > sdb->buf_size)
>  		to_copy = sdb->buf_size - sdb->buf_rdptr;
>  
> +	if (!to_copy)
> +		return 0;
> +
>  	if (copy_to_user(data, sdb->buf_base + sdb->buf_rdptr, to_copy)) {
>  		dev_dbg(dev, "Failed to copy data to user\n");
>  		return -EFAULT;

Re: [PATCH 2/3] coresight: ultrasoc-smb: simplify the code for check to_copy valid

Posted by hejunhao 1 year, 6 months ago

On 2023/10/19 21:35, Jonathan Cameron wrote:
> On Thu, 12 Oct 2023 17:47:05 +0800
> Junhao He <hejunhao3@huawei.com> wrote:
>
>> We only need to check once when before using the to_copy variable
>> to simplify the code.
>>
>> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> I'm not convinced this one is an improvement. Sometimes it's easier to just
> see the individual conditions checked even if we could combine them.
> It's easy to understand we don't copy data if:
> a) We ask for 0 data.
> b) There is 0 data
>
> Less easy to establish that with the extra wrap around code in there
> (even though that has no impact on to_copy if it is 0)
>
> Jonathan
>

Thanks, I will drop this patch.

Best regards,
Junhao.

>> ---
>>   drivers/hwtracing/coresight/ultrasoc-smb.c | 13 ++++---------
>>   1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
>> index b08a619d1116..e78edc3480ce 100644
>> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
>> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
>> @@ -127,20 +127,15 @@ static ssize_t smb_read(struct file *file, char __user *data, size_t len,
>>   					struct smb_drv_data, miscdev);
>>   	struct smb_data_buffer *sdb = &drvdata->sdb;
>>   	struct device *dev = &drvdata->csdev->dev;
>> -	ssize_t to_copy = 0;
>> -
>> -	if (!len)
>> -		return 0;
>> -
>> -	if (!sdb->data_size)
>> -		return 0;
>> -
>> -	to_copy = min(sdb->data_size, len);
>> +	ssize_t to_copy = min(sdb->data_size, len);
>>   
>>   	/* Copy parts of trace data when read pointer wrap around SMB buffer */
>>   	if (sdb->buf_rdptr + to_copy > sdb->buf_size)
>>   		to_copy = sdb->buf_size - sdb->buf_rdptr;
>>   
>> +	if (!to_copy)
>> +		return 0;
>> +
>>   	if (copy_to_user(data, sdb->buf_base + sdb->buf_rdptr, to_copy)) {
>>   		dev_dbg(dev, "Failed to copy data to user\n");
>>   		return -EFAULT;
> .
>