[igt-dev] [Intel-gfx] [PATCH i-g-t] intel_gpu_top: Fix cleanup on old kernels / unsupported GPU

[Das, Nirmoy]

On 1/23/2023 12:09 PM, Das, Nirmoy wrote:
> Hi Tvrtko,
>
> On 1/23/2023 11:24 AM, Tvrtko Ursulin wrote:
>>
>> On 20/01/2023 14:29, Das, Nirmoy wrote:
>>> Hi Tvrtko,
>>>
>>> On 1/19/2023 4:54 PM, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>
>>>> Avoid trying to dereference null engines on exit when there are either
>>>> none which are supported, or kernel does not have i915 PMU support.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>> ---
>>>>   tools/intel_gpu_top.c | 10 +++++++---
>>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
>>>> index 7aa233570463..517dc2995d26 100644
>>>> --- a/tools/intel_gpu_top.c
>>>> +++ b/tools/intel_gpu_top.c
>>>> @@ -448,6 +448,9 @@ static void free_engines(struct engines *engines)
>>>>       };
>>>>       unsigned int i;
>>>> +    if (!engines)
>>>> +        return;
>>>> +
>>>
>>>
>>> This check should happen before the  struct pmu_counter free_list 
>>> generation.
>>
>> I could, but it doesn't have to, not sure what reasons for should you 
>> have in mind?
>
> In my tree, I see:
>
> "
>
> static void free_engines(struct engines *engines)
>
> {
>         struct pmu_counter **pmu, *free_list[] = {
>                 &engines->r_gpu,
>                 &engines->r_pkg,
>                 &engines->imc_reads,
>                 &engines->imc_writes,
>                 NULL
>         };
>
>         unsigned int i;
>
> "
>
> Maybe I am missing something here, wouldn't those dereferences of 
> "engines" segfault if the engines  is NULL.


Ah never mind it shouldn't matter.

>
>
> Regards,
>
> Nirmoy
>
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> Regards,
>>>
>>> Nirmoy
>>>
>>>>       for (pmu = &free_list[0]; *pmu; pmu++) {
>>>>           if ((*pmu)->present)
>>>>               free((char *)(*pmu)->units);
>>>> @@ -2568,7 +2571,7 @@ int main(int argc, char **argv)
>>>>               "Failed to detect engines! (%s)\n(Kernel 4.16 or 
>>>> newer is required for i915 PMU support.)\n",
>>>>               strerror(errno));
>>>>           ret = EXIT_FAILURE;
>>>> -        goto err;
>>>> +        goto err_engines;
>>>>       }
>>>>       ret = pmu_init(engines);
>>>> @@ -2585,7 +2588,7 @@ int main(int argc, char **argv)
>>>>   "More information can be found at 'Perf events and tool security' 
>>>> document:\n"
>>>> "https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html\n"); 
>>>>
>>>>           ret = EXIT_FAILURE;
>>>> -        goto err;
>>>> +        goto err_pmu;
>>>>       }
>>>>       ret = EXIT_SUCCESS;
>>>> @@ -2699,8 +2702,9 @@ int main(int argc, char **argv)
>>>>           free_clients(clients);
>>>>       free(codename);
>>>> -err:
>>>> +err_pmu:
>>>>       free_engines(engines);
>>>> +err_engines:
>>>>       free(pmu_device);
>>>>   exit:
>>>>       igt_devices_free();