Re: [PATCH v2 03/24] media: iris: Fix memory leak by freeing untracked persist buffer

[Dikshita Agarwal <quic_dikshita@quicinc.com>]

On 8/16/2025 3:45 PM, Bryan O'Donoghue wrote:
> On 13/08/2025 10:37, Dikshita Agarwal wrote:
>> One internal buffer which is allocated only once per session was not
>> being freed during session close because it was not being tracked as
>> part of internal buffer list which resulted in a memory leak.
>>
>> Add the necessary logic to explicitly free the untracked internal buffer
>> during session close to ensure all allocated memory is released
>> properly.
>>
>> Fixes: 73702f45db81 ("media: iris: allocate, initialize and queue
>> internal buffers")
>> Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> Tested-by: Vikash Garodia <quic_vgarodia@quicinc.com> # X1E80100
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/iris/iris_buffer.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c
>> b/drivers/media/platform/qcom/iris/iris_buffer.c
>> index
>> 6425e4919e3b0b849ba801ca9e01921c114144cd..9f664c241149362d44d3a8fa65e2266f9c2e80e0 100644
>> --- a/drivers/media/platform/qcom/iris/iris_buffer.c
>> +++ b/drivers/media/platform/qcom/iris/iris_buffer.c
>> @@ -413,6 +413,16 @@ static int iris_destroy_internal_buffers(struct
>> iris_inst *inst, u32 plane, bool
>>           }
>>       }
>>   +    if (force) {
>> +        buffers = &inst->buffers[BUF_PERSIST];
>> +
>> +        list_for_each_entry_safe(buf, next, &buffers->list, list) {
>> +            ret = iris_destroy_internal_buffer(inst, buf);
>> +            if (ret)
>> +                return ret;
>> +        }
>> +    }
>> +
>>       return 0;
>>   }
>>  
> 
> Why is the logic here not to simply release every index of the enum
> iris_buffer_type ?

All buffers indicated with enum iris_buffer_type are not related to both
encoder and decoder, some are specific for encoder and some are for
decoder. Hence while freeing we read the internal buffer list maintained in
platform data to get the exact buffer list applicable.

> 
> If I'm reading the code right here, len indicates the list of linked lists
> to free, adding BUF_PERSIST appends to the list that may be freed if force
> is true but, then what about the remaining entries BUF_SCRATCH_1 ?
> 

the buffer list here is the list of buffers of a specific kind eg: BUF_PERSIST.

BUF_PERSIST is not part of list maintained in platform data because it has
a different lifecycle than other internal buffers. that's why this specific
routine is needed to free BUF_PERSIST when destroying all buffers.


> Is it valid to leave this routine with force = true but BUF_SCRATCH_1 not
> specifically indexed, if so why ?

Other internal buffers are freed in another routine in the same API which
is not part of this particular code snippet.

Thanks,
Dikshita

> 

> ---
> bod