Nouveau - Oct 2024 - [PATCH v3 10/15] nvkm: refine the variable names in r535_gsp_msg_recv()

On Thu, 2024-10-31 at 01:52 -0700, Zhi Wang wrote:
> @@ -336,59 +336,60 @@ static struct nvfw_gsp_rpc *
> ?r535_gsp_msg_recv(struct nvkm_gsp *gsp, int fn, u32 gsp_rpc_len)
> ?{
> ?	struct nvkm_subdev *subdev = &gsp->subdev;
> -	struct nvfw_gsp_rpc *msg;
> +	struct nvfw_gsp_rpc *rpc;
> ?	int time = 4000000, i;
> ?	u32 size;
> ?
> ?retry:
> -	msg = r535_gsp_msgq_wait(gsp, sizeof(*msg), &size, &time);
> -	if (IS_ERR_OR_NULL(msg))
> -		return msg;
> +	rpc = r535_gsp_msgq_wait(gsp, sizeof(*rpc), &size, &time);
> +	if (IS_ERR_OR_NULL(rpc))
> +		return rpc;
I know this change is supposed to be non-functional, but I did notice a
pattern here.

This function:

	rpc = r535_gsp_msgq_wait(gsp, sizeof(*rpc), &size, &time);
	if (IS_ERR_OR_NULL(rpc))
		return rpc;

Function r535_gsp_rpc_poll, which calls this function:

	repv = r535_gsp_msg_recv(gsp, fn, 0);
	mutex_unlock(&gsp->cmdq.mutex);
	if (IS_ERR(repv))
		return PTR_ERR(repv);

So if rpc is NULL, r535_gsp_msg_recv() will return NULL, but r535_gsp_rpc_poll
expects an error code instead.  Since it technically doesn't get one, it
returns 0 (success).

To be fair, it does not appear that r535_gsp_msgq_wait() can return NULL, but
that is obscured by the code.

On 31/10/2024 16.27, Timur Tabi wrote:
> On Thu, 2024-10-31 at 01:52 -0700, Zhi Wang wrote:
>> @@ -336,59 +336,60 @@ static struct nvfw_gsp_rpc *
>>  ?r535_gsp_msg_recv(struct nvkm_gsp *gsp, int fn, u32 gsp_rpc_len)
>>  ?{
>>  ?	struct nvkm_subdev *subdev = &gsp->subdev;
>> -	struct nvfw_gsp_rpc *msg;
>> +	struct nvfw_gsp_rpc *rpc;
>>  ?	int time = 4000000, i;
>>  ?	u32 size;
>>   
>>  ?retry:
>> -	msg = r535_gsp_msgq_wait(gsp, sizeof(*msg), &size, &time);
>> -	if (IS_ERR_OR_NULL(msg))
>> -		return msg;
>> +	rpc = r535_gsp_msgq_wait(gsp, sizeof(*rpc), &size, &time);
>> +	if (IS_ERR_OR_NULL(rpc))
>> +		return rpc;
> 
> I know this change is supposed to be non-functional, but I did notice a
> pattern here.
> 
> This function:
> 
> 	rpc = r535_gsp_msgq_wait(gsp, sizeof(*rpc), &size, &time);
> 	if (IS_ERR_OR_NULL(rpc))
> 		return rpc;
> 
> Function r535_gsp_rpc_poll, which calls this function:
> 
> 	repv = r535_gsp_msg_recv(gsp, fn, 0);
> 	mutex_unlock(&gsp->cmdq.mutex);
> 	if (IS_ERR(repv))
> 		return PTR_ERR(repv);
> 
> So if rpc is NULL, r535_gsp_msg_recv() will return NULL, but
r535_gsp_rpc_poll
> expects an error code instead.  Since it technically doesn't get one,
it
> returns 0 (success).
> 
> To be fair, it does not appear that r535_gsp_msgq_wait() can return NULL,
but
> that is obscured by the code.
> 
Nice catch!

It should be fixed in the re-factor in PATCH 12, where 
r535_gsp_msgq_wait() always returns an int (error code).
>