[igt-dev] [PATCH i-g-t v4 1/3] lib/igt_device_scan: Remember vendor/device for pci devices

[Tvrtko Ursulin]

On 19/11/2020 07:25, Zbigniew Kempczyński wrote:
> On Wed, Nov 18, 2020 at 03:17:08PM +0000, Tvrtko Ursulin wrote:
>>
>> On 18/11/2020 13:50, Zbigniew Kempczyński wrote:
>>> If we want to use pci device id for not opened device we need to
>>> keep it in card structure.
>>>
>>> Export igt_device_get_pretty_name() function to the caller to
>>> allow return pretty name (for lsgpu, intel_gpu_top and others).
>>>
>>> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> Cc: Petri Latvala <petri.latvala at intel.com>
>>> ---
>>>    lib/igt_device_scan.c | 150 ++++++++++++++++++++++++++++++++++++++----
>>>    lib/igt_device_scan.h |   3 +
>>>    2 files changed, 140 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
>>> index e97f7163..df41ee51 100644
>>> --- a/lib/igt_device_scan.c
>>> +++ b/lib/igt_device_scan.c
>>> @@ -25,7 +25,9 @@
>>>    #include "igt_core.h"
>>>    #include "igt_device_scan.h"
>>>    #include "igt_list.h"
>>> +#include "intel_chipset.h"
>>>    
>>> +#include <ctype.h>
>>>    #include <dirent.h>
>>>    #include <fcntl.h>
>>>    #include <glib.h>
>>> @@ -178,12 +180,43 @@ static struct {
>>>    	bool devs_scanned;
>>>    } igt_devs;
>>>    
>>> +typedef char *(*devname_fn)(uint16_t, uint16_t);
>>> +
>>> +static char *devname_hex(uint16_t vendor, uint16_t device)
>>> +{
>>> +	char *s;
>>> +
>>> +	igt_assert(asprintf(&s, "%04x:%04x", vendor, device) == 9);
>>> +
>>> +	return s;
>>> +}
>>> +
>>> +static char *devname_intel(uint16_t vendor, uint16_t device)
>>> +{
>>> +	const struct intel_device_info *info = intel_get_device_info(device);
>>> +	char *devname, *s;
>>> +
>>> +	if (info->codename) {
>>> +		devname = strdup(info->codename);
>>> +		devname[0] = toupper(devname[0]);
>>
>> Strictly could be a null ptr deref here.
> 
> IMO not. We can have info->codename NULL for missing gen, so we enter else path.
> Other cases are info->codename lead to valid string and even if it could be ""
> toupper does nothing there.

I was referring to strdup returning NULL.

>>
>>> +	} else {
>>> +		asprintf(&devname, "%04x:%04x", vendor, device);
>>
>> Could call devname_hex, for some questionable value of improvement, up to you.
> 
> Ack.
> 
>>
>>> +	}
>>> +
>>> +	asprintf(&s, "Intel %s (Gen%u)", devname, ffs(info->gen));
>>
>> Is info->gen always valid? Looks like not, we'd gen Gen0 on unknown devices. Weird API, anyway.. Maybe try like this then:
>>
>> if (info->codename) {
>> 	char *devname = strdup(info->codename);
>>
>> 	if (devname) {
>> 		devname[0] = toupper(devname[0]);
>> 		asprintf(&s, "Intel %s (Gen%u)", devname, ffs(info->gen));
>> 		free(devname);
>> 	}
>> }
>>
>> if (!devname)
>> 	s = devname_hex(vendor, device);
>>
>> return s;
>>
>> I think that should cover all the cases.
> 
> free(devname) then if (!devname) is likely not we want here. But I got
> your point we should properly display at least vendor:device.

Did not get what you meant but yes, fallback (even if strdup fails!) and 
possibly simplified flow.

Regards,

Tvrtko