[PATCH i-g-t] tools/intel_reg: Add --pci-slot option

[Krzysztof Karas]

Hi Kamil,

[...]
>  	printf(" --all          Decode registers for all known platforms. Implies --decode\n");
> +	printf(" --pci-slot     Decode registers for platform described by PCI slot\n"
> +	       "		<domain>:<bus>:<device>[.<func>]\n");
You could include information that if PCI slot is not provided,
then the first matching Intel device (starting with 0000:00:02.0)
will be chosen.

[...]
> +static int parse_pci_slot_name(struct igt_pci_slot *st, const char *slot_name)
> +{
> +	int i;
> +
> +	st->domain = 0;
> +	st->bus = 0;
> +	st->dev = 0;
> +	st->func = 0;
If you zeroed bdf: struct igt_pci_slot bdf = {}; before calling
parse_pci_slot_name(), then it could become:

return sscanf(slot_name, "%x:%x:%x.%x", &st->domain, &st->bus, &st->dev, &st->func);

> +	i = sscanf(slot_name, "%x:%x:%x.%x", &st->domain, &st->bus, &st->dev, &st->func);
> +
> +	return i;
> +}
> +
> +static bool is_graphics_card_valid(struct pci_device *pci_dev)
> +{
> +	if (!pci_dev) {
> +		fprintf(stderr, "Graphics card not found\n");
> +		return false;
> +	}
> +	if (pci_device_probe(pci_dev) != 0) {
> +		fprintf(stderr, "Couldn't probe graphics card\n");
> +		return false;
> +	}
> +	if (pci_dev->vendor_id != 0x8086) {
> +		fprintf(stderr, "Graphics card is non-intel\n");
> +		return false;
> +	}
> +	return true;
> +}
This function checks whether a pci_dev can be found, probed and
has Intel as vendor - there technically is no information about
this device being a GPU. You could change the name to something
like: "is_pci_device_valid" (unless there is a way to determine
a device is a GPU, then that addition would be better).

[...]
>  int main(int argc, char *argv[])
>  {
> -	int ret, i, index;
> +	int i, index;
>  	char *endp;
> +	char *opt_slot = NULL;
>  	enum opt opt;
>  	const struct command *command = NULL;
>  	struct config config = {
> @@ -1191,6 +1264,7 @@ int main(int argc, char *argv[])
>  		.fd = -1,
>  	};
>  	bool help = false;
> +	int ret = EXIT_FAILURE;
this ret variable is not used up till ret = command->function(),
so I don't think there is a benefit to adding a certain error
code to it, unless you'd change all "return EXIT_FAILURE;"
to "return ret;".

[...]
> -	if (read_reg_spec(&config) < 0)
> -		return EXIT_FAILURE;
> -
> -	ret = command->function(&config, argc, argv);
> +	if (read_reg_spec(&config) >= 0)
> +		ret = command->function(&config, argc, argv);
This is unrelated to adding "--pci-slot" - it could be placed in
a separate patch or mentioned in commit message as a cleanup.

I also noticed that the intel_reg program is not very resistant
to user misinput: if you attempt to use "read" option with
a device that does not have a certain register, then the program
crashes with assestion failures.

Best regards,
Krzysztof