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

[Kamil Konieczny]

Hi Krzysztof,
On 2025-02-12 at 13:40:15 +0000, Krzysztof Karas wrote:
> 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.
> 

If user will not provide any input for slot she/he can just
use it without this option and this is already a default
behaviour but I agree, it could be clarified so I will add it.

Btw all options are optional (see printed help).

> [...]
> > +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);
> 

Right, will change.

> > +	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).
> 
> [...]

Right, I will change this into is_intel_card_valid()
and also improve error prints plus style (newlines after
closing brackets).

> >  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.

Ah, good catch, I should resist unneeded refactoring, I will
revert this so no big changes here.

> 
> 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

Please provide example(s) how did you run it so I could
reproduce it.

Regards,
Kamil