[PATCH V3] amdgpu: remove unnecessary condition check

[PATCH V3] amdgpu: remove unnecessary condition check

[Bernard Zhao]

There is no need to if check again, maybe we could merge
into the above else branch.

Signed-off-by: Bernard Zhao <bernard@vivo.com>

Changes since V1:
*commit message improve
*code style refactoring

Changes since V2:
*code style adjust

Link for V1:
*https://lore.kernel.org/patchwork/patch/1226587/
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c   | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 9dff792c9290..5424bd921a7b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -660,13 +660,12 @@ static int reserve_bo_and_vm(struct kgd_mem *mem,
 
 	ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list,
 				     false, &ctx->duplicates);
-	if (!ret)
-		ctx->reserved = true;
-	else {
-		pr_err("Failed to reserve buffers in ttm\n");
+	if (ret) {
+		pr_err("Failed to reserve buffers in ttm.\n");
 		kfree(ctx->vm_pd);
 		ctx->vm_pd = NULL;
-	}
+	} else
+		ctx->reserved = true;
 
 	return ret;
 }
@@ -733,15 +732,12 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem,
 
 	ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list,
 				     false, &ctx->duplicates);
-	if (!ret)
-		ctx->reserved = true;
-	else
-		pr_err("Failed to reserve buffers in ttm.\n");
-
 	if (ret) {
+		pr_err("Failed to reserve buffers in ttm.\n");
 		kfree(ctx->vm_pd);
 		ctx->vm_pd = NULL;
-	}
+	} else
+		ctx->reserved = true;
 
 	return ret;
 }
-- 
2.26.2

Re: [PATCH V3] amdgpu: remove unnecessary condition check

[Christian König]

Am 21.04.20 um 10:03 schrieb Bernard Zhao:
> There is no need to if check again, maybe we could merge
> into the above else branch.
>
> Signed-off-by: Bernard Zhao <bernard@vivo.com>
>
> Changes since V1:
> *commit message improve
> *code style refactoring
>
> Changes since V2:
> *code style adjust
>
> Link for V1:
> *https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1226587%2F&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C0b8fffafb715474289b208d7e5ca7f6c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637230530201280350&amp;sdata=Sewv5ESX%2B0B4DbFbE03uM5sifrEcmJllC8pt7J42I7M%3D&amp;reserved=0
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c   | 18 +++++++-----------
>   1 file changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 9dff792c9290..5424bd921a7b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -660,13 +660,12 @@ static int reserve_bo_and_vm(struct kgd_mem *mem,
>   
>   	ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list,
>   				     false, &ctx->duplicates);
> -	if (!ret)
> -		ctx->reserved = true;
> -	else {
> -		pr_err("Failed to reserve buffers in ttm\n");
> +	if (ret) {
> +		pr_err("Failed to reserve buffers in ttm.\n");
>   		kfree(ctx->vm_pd);
>   		ctx->vm_pd = NULL;
> -	}
> +	} else
> +		ctx->reserved = true;

That is still not correct coding style. In general when one branch of an 
if/else uses {} the other one should use it as well.

But I agree with Felix that this change looks rather superfluous to me 
as well.

Regards,
Christian.

>   
>   	return ret;
>   }
> @@ -733,15 +732,12 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem,
>   
>   	ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list,
>   				     false, &ctx->duplicates);
> -	if (!ret)
> -		ctx->reserved = true;
> -	else
> -		pr_err("Failed to reserve buffers in ttm.\n");
> -
>   	if (ret) {
> +		pr_err("Failed to reserve buffers in ttm.\n");
>   		kfree(ctx->vm_pd);
>   		ctx->vm_pd = NULL;
> -	}
> +	} else
> +		ctx->reserved = true;
>   
>   	return ret;
>   }

Re:Re: [PATCH V3] amdgpu: remove unnecessary condition check

[赵军奎]

From: "Christian König" <christian.koenig@amd.com>
Date: 2020-04-21 16:06:03
To:  1587180037-113840-1-git-send-email-bernard@vivo.com,Felix Kuehling <Felix.Kuehling@amd.com>,Alex Deucher <alexander.deucher@amd.com>,"David (ChunMing) Zhou" <David1.Zhou@amd.com>,David Airlie <airlied@linux.ie>,Daniel Vetter <daniel@ffwll.ch>,amd-gfx@lists.freedesktop.org,dri-devel@lists.freedesktop.org,linux-kernel@vger.kernel.org
Cc:  opensource.kernel@vivo.com,Bernard Zhao <bernard@vivo.com>
Subject: Re: [PATCH V3] amdgpu: remove unnecessary condition check>Am 21.04.20 um 10:03 schrieb Bernard Zhao:
>> There is no need to if check again, maybe we could merge
>> into the above else branch.
>>
>> Signed-off-by: Bernard Zhao <bernard@vivo.com>
>>
>> Changes since V1:
>> *commit message improve
>> *code style refactoring
>>
>> Changes since V2:
>> *code style adjust
>>
>> Link for V1:
>> *https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1226587%2F&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C0b8fffafb715474289b208d7e5ca7f6c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637230530201280350&amp;sdata=Sewv5ESX%2B0B4DbFbE03uM5sifrEcmJllC8pt7J42I7M%3D&amp;reserved=0
>> ---
>>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c   | 18 +++++++-----------
>>   1 file changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index 9dff792c9290..5424bd921a7b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -660,13 +660,12 @@ static int reserve_bo_and_vm(struct kgd_mem *mem,
>>   
>>   	ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list,
>>   				     false, &ctx->duplicates);
>> -	if (!ret)
>> -		ctx->reserved = true;
>> -	else {
>> -		pr_err("Failed to reserve buffers in ttm\n");
>> +	if (ret) {
>> +		pr_err("Failed to reserve buffers in ttm.\n");
>>   		kfree(ctx->vm_pd);
>>   		ctx->vm_pd = NULL;
>> -	}
>> +	} else
>> +		ctx->reserved = true;
>
>That is still not correct coding style. In general when one branch of an 
>if/else uses {} the other one should use it as well.
>
>But I agree with Felix that this change looks rather superfluous to me 
>as well.
>
>Regards,
>Christian.

About the code style, you are right, I checked the refers:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=90280eaa88ac1a9140dc759941123530d5545bb6#n191
The if and else should use the same style.
But i have to say there are so many code not follow the kernel code-style in amdgpu module.
And also the ./scripts/checkpatch.pl did not throw any warning or error.

If this change looks rather superfluous to all of you, should i change to the V1 change?
After all i don`t think there is any necessary to check "ret" again, merge the <else and if (ret)>
maybe better.
Original code:
static int reserve_bo_and_cond_vms(struct kgd_mem *mem,.....
	if (!ret)
		ctx->reserved = true;
	else
		pr_err("Failed to reserve buffers in ttm.\n");

	if (ret) {
		kfree(ctx->vm_pd);
		ctx->vm_pd = NULL;
	}

BR//bernard

>>   
>>   	return ret;
>>   }
>> @@ -733,15 +732,12 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem,
>>   
>>   	ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list,
>>   				     false, &ctx->duplicates);
>> -	if (!ret)
>> -		ctx->reserved = true;
>> -	else
>> -		pr_err("Failed to reserve buffers in ttm.\n");
>> -
>>   	if (ret) {
>> +		pr_err("Failed to reserve buffers in ttm.\n");
>>   		kfree(ctx->vm_pd);
>>   		ctx->vm_pd = NULL;
>> -	}
>> +	} else
>> +		ctx->reserved = true;
>>   
>>   	return ret;
>>   }
>

Re: [V3] amdgpu: remove unnecessary condition check

[Markus Elfring]

> But i have to say there are so many code not follow the kernel code-style in amdgpu module.
> And also the ./scripts/checkpatch.pl did not throw any warning or error.

Will such information become more interesting for further evolution
in the affected software areas?

Regards,
Markus

Re: [PATCH V3] amdgpu: remove unnecessary condition check

[Christian König]

Am 21.04.20 um 10:44 schrieb 赵军奎:
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> index 9dff792c9290..5424bd921a7b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> @@ -660,13 +660,12 @@ static int reserve_bo_and_vm(struct kgd_mem *mem,
>>>    
>>>    	ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list,
>>>    				     false, &ctx->duplicates);
>>> -	if (!ret)
>>> -		ctx->reserved = true;
>>> -	else {
>>> -		pr_err("Failed to reserve buffers in ttm\n");
>>> +	if (ret) {
>>> +		pr_err("Failed to reserve buffers in ttm.\n");
>>>    		kfree(ctx->vm_pd);
>>>    		ctx->vm_pd = NULL;
>>> -	}
>>> +	} else
>>> +		ctx->reserved = true;
>> That is still not correct coding style. In general when one branch of an
>> if/else uses {} the other one should use it as well.
>>
>> But I agree with Felix that this change looks rather superfluous to me
>> as well.
>>
>> Regards,
>> Christian.
> About the code style, you are right, I checked the refers:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2FDocumentation%2Fprocess%2Fcoding-style.rst%3Fid%3D90280eaa88ac1a9140dc759941123530d5545bb6%23n191&data=02%7C01%7Cchristian.koenig%40amd.com%7C3640a853a31e4e98f66d08d7e5d05300%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637230555230917911&sdata=Ihum8tjQAw%2Bk67KTq%2B3zBBLHL1bSHBhBhSyG0jWeMcE%3D&reserved=0
> The if and else should use the same style.
> But i have to say there are so many code not follow the kernel code-style in amdgpu module.
> And also the ./scripts/checkpatch.pl did not throw any warning or error.

That is unfortunately true, yes. But we try to push new code through the 
usual code review and improve things as we go.

On the other hand patches just to fix the coding style are usually seen 
as an unnecessary interruption and just a waste of time.

But in this particular case you could argue that the original code is 
not easily readable and you try to improve that.

> If this change looks rather superfluous to all of you, should i change to the V1 change?
> After all i don`t think there is any necessary to check "ret" again, merge the <else and if (ret)>
> maybe better.
> Original code:
> static int reserve_bo_and_cond_vms(struct kgd_mem *mem,.....
> 	if (!ret)
> 		ctx->reserved = true;
> 	else
> 		pr_err("Failed to reserve buffers in ttm.\n");
>
> 	if (ret) {
> 		kfree(ctx->vm_pd);
> 		ctx->vm_pd = NULL;
> 	}

In general I suggest to use the following pattern for this error 
handling and avoid the else branch altogether:

if (ret) {
     pr_err("Failed to reserve buffers in ttm.\n");
     kfree(ctx->vm_pd);
     ctx->vm_pd = NULL;
     return ret;
}

ctx->reserved = true;
return 0;

When things become more complex good practice is to insert a "goto 
error_cleanup"; instead of in place cleanup.

Regards,
Christian.

>
> BR//bernard

Re: [V3] amdgpu: remove unnecessary condition check

[Markus Elfring]

>> But i have to say there are so many code not follow the kernel code-style in amdgpu module.
>> And also the ./scripts/checkpatch.pl did not throw any warning or error.
>
> That is unfortunately true, yes. But we try to push new code through the usual code review and improve things as we go.
>
> On the other hand patches just to fix the coding style are usually seen as an unnecessary interruption and just a waste of time.

Would you become interested in adjusting deviations from known programming guidelines
in an automatic way with the help of corresponding advanced development tools?

Regards,
Markus

Re: [V3] amdgpu: remove unnecessary condition check

[Markus Elfring]

>>> There is no need to if check again, maybe we could merge
>>> into the above else branch.

I find also this commit message still improvable (besides the mentioned
implementation details around coding style concerns).
How will corresponding review comments be taken better into account?

Regards,
Markus