[PATCHv2] media: venus: venus_helper_get_bufreq(): req is never NULL

[PATCHv2] media: venus: venus_helper_get_bufreq(): req is never NULL

[Hans Verkuil]

Fix a smatch error:

drivers/media/platform/qcom/venus/helpers.c: drivers/media/platform/qcom/venus/helpers.c:678 venus_helper_get_bufreq() error: we previously assumed 'req' could be null (see line 674)

After checking how venus_helper_get_bufreq() is called it is clear that
req is never NULL, so just drop the checks.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/qcom/venus/helpers.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index 5c1104379c49..60de4200375d 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -671,8 +671,7 @@ int venus_helper_get_bufreq(struct venus_inst *inst, u32 type,
 	unsigned int i;
 	int ret;
 
-	if (req)
-		memset(req, 0, sizeof(*req));
+	memset(req, 0, sizeof(*req));
 
 	if (type == HFI_BUFFER_OUTPUT || type == HFI_BUFFER_OUTPUT2)
 		req->count_min = inst->fw_min_cnt;
@@ -694,8 +693,7 @@ int venus_helper_get_bufreq(struct venus_inst *inst, u32 type,
 		if (hprop.bufreq[i].type != type)
 			continue;
 
-		if (req)
-			memcpy(req, &hprop.bufreq[i], sizeof(*req));
+		memcpy(req, &hprop.bufreq[i], sizeof(*req));
 		ret = 0;
 		break;
 	}
-- 
2.35.1

Re: [PATCHv2] media: venus: venus_helper_get_bufreq(): req is never NULL

[Stanimir Varbanov]

Hi Hans,

Thanks for the patch!

On 7/12/22 13:47, Hans Verkuil wrote:
> Fix a smatch error:
> 
> drivers/media/platform/qcom/venus/helpers.c: drivers/media/platform/qcom/venus/helpers.c:678 venus_helper_get_bufreq() error: we previously assumed 'req' could be null (see line 674)
> 
> After checking how venus_helper_get_bufreq() is called it is clear that
> req is never NULL, so just drop the checks.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/platform/qcom/venus/helpers.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 5c1104379c49..60de4200375d 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -671,8 +671,7 @@ int venus_helper_get_bufreq(struct venus_inst *inst, u32 type,
>  	unsigned int i;
>  	int ret;
>  
> -	if (req)
> -		memset(req, 0, sizeof(*req));
> +	memset(req, 0, sizeof(*req));

I wonder, can we just return EINVAL if req == NULL? By that way we could
avoid future errors.

>  
>  	if (type == HFI_BUFFER_OUTPUT || type == HFI_BUFFER_OUTPUT2)
>  		req->count_min = inst->fw_min_cnt;
> @@ -694,8 +693,7 @@ int venus_helper_get_bufreq(struct venus_inst *inst, u32 type,
>  		if (hprop.bufreq[i].type != type)
>  			continue;
>  
> -		if (req)
> -			memcpy(req, &hprop.bufreq[i], sizeof(*req));
> +		memcpy(req, &hprop.bufreq[i], sizeof(*req));
>  		ret = 0;
>  		break;
>  	}

-- 
regards,
Stan

Re: [PATCHv2] media: venus: venus_helper_get_bufreq(): req is never NULL

[Hans Verkuil]

On 7/12/22 15:08, Stanimir Varbanov wrote:
> Hi Hans,
> 
> Thanks for the patch!
> 
> On 7/12/22 13:47, Hans Verkuil wrote:
>> Fix a smatch error:
>>
>> drivers/media/platform/qcom/venus/helpers.c: drivers/media/platform/qcom/venus/helpers.c:678 venus_helper_get_bufreq() error: we previously assumed 'req' could be null (see line 674)
>>
>> After checking how venus_helper_get_bufreq() is called it is clear that
>> req is never NULL, so just drop the checks.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/platform/qcom/venus/helpers.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
>> index 5c1104379c49..60de4200375d 100644
>> --- a/drivers/media/platform/qcom/venus/helpers.c
>> +++ b/drivers/media/platform/qcom/venus/helpers.c
>> @@ -671,8 +671,7 @@ int venus_helper_get_bufreq(struct venus_inst *inst, u32 type,
>>  	unsigned int i;
>>  	int ret;
>>  
>> -	if (req)
>> -		memset(req, 0, sizeof(*req));
>> +	memset(req, 0, sizeof(*req));
> 
> I wonder, can we just return EINVAL if req == NULL? By that way we could
> avoid future errors.

We can, but it makes no sense for this function to be called with a NULL
pointer for req, if you do, then it is a driver bug. I.e, we don't test if
inst is NULL either.

I thought about it, but decided that it would just add unnecessary overhead
in this specific case.

If you still want this, then I can post a v3.

Regards,

	Hans

> 
>>  
>>  	if (type == HFI_BUFFER_OUTPUT || type == HFI_BUFFER_OUTPUT2)
>>  		req->count_min = inst->fw_min_cnt;
>> @@ -694,8 +693,7 @@ int venus_helper_get_bufreq(struct venus_inst *inst, u32 type,
>>  		if (hprop.bufreq[i].type != type)
>>  			continue;
>>  
>> -		if (req)
>> -			memcpy(req, &hprop.bufreq[i], sizeof(*req));
>> +		memcpy(req, &hprop.bufreq[i], sizeof(*req));
>>  		ret = 0;
>>  		break;
>>  	}
> 

Re: [PATCHv2] media: venus: venus_helper_get_bufreq(): req is never NULL

[Stanimir Varbanov]

On 7/12/22 13:47, Hans Verkuil wrote:
> Fix a smatch error:
> 
> drivers/media/platform/qcom/venus/helpers.c: drivers/media/platform/qcom/venus/helpers.c:678 venus_helper_get_bufreq() error: we previously assumed 'req' could be null (see line 674)
> 
> After checking how venus_helper_get_bufreq() is called it is clear that
> req is never NULL, so just drop the checks.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/platform/qcom/venus/helpers.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

Acked-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

-- 
regards,
Stan