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).
>
