[igt-dev] [PATCH i-g-t 6/6] lib/igt_device: Move intel_get_pci_device under igt_device

[Chris Wilson]

Quoting Michał Winiarski (2019-03-13 12:53:32)
> +#define IGT_DEV_PATH_LEN 80
> +
> +static bool igt_device_is_pci(int fd)
> +{
> +       char path[IGT_DEV_PATH_LEN];
> +       char link[IGT_DEV_PATH_LEN];
> +       char *subsystem;
> +       int len;
> +
> +       igt_assert_f(igt_sysfs_path(fd, path, sizeof(path)),
> +                    "Failed to find device sysfs path.\n");
> +
> +       strcat(path, "/device/subsystem");
> +
> +       if ((len = readlink(path, link, sizeof(link) - 1)) == -1)
> +               return false;
> +       link[len] = '\0';
> +
> +       subsystem = strrchr(link, '/') + 1;
> +
> +       return strcmp(subsystem, "pci") == 0;
> +}
> +
> +static void igt_device_get_pci_addr(int fd, char *buf, int len)
> +{
> +       char path[IGT_DEV_PATH_LEN];
> +       char link[IGT_DEV_PATH_LEN];
> +       char *pci_addr;
> +       int link_len;
> +
> +       igt_assert_f(igt_device_is_pci(fd), "This is not a PCI device.");
> +
> +       igt_assert_f(igt_sysfs_path(fd, path, sizeof(path)),
> +                    "Failed to find device sysfs path.\n");
> +

not

	dir = igt_sysfs_dir(fd);
	readlinkat(dir, "device", ...)
?

> +       strcat(path, "/device");
> +
> +       igt_assert_f((link_len = readlink(path, link, sizeof(link) - 1)) != -1,
> +                    "Failed to find device sysfs path.\n");

igt_ignore_warn()

Less with the asserts, just return an error so that the routines are
more reusable.

> +       if (link_len)
> +               link[link_len] = '\0';
> +
> +       pci_addr = strrchr(link, '/') + 1;
> +
> +       strncpy(buf, pci_addr, len - 1);
> +       if (len)

Eek, but now you said len-1 may have been -1!!!

> +               buf[len] = '\0';
> +}
> +
> +struct pci_device *igt_device_get_pci_dev(int fd)

I would resist the urge to contract.
igt_device_get_pci_device()
igt_device_get_pci (maybe)

> +{
> +       char pci_addr[IGT_DEV_PATH_LEN];
> +       struct pci_device *pci_dev;
> +       unsigned int domain, bus, device, function;
> +       int err;
> +
> +       igt_device_get_pci_addr(fd, pci_addr, sizeof(pci_addr));
> +
> +       err = sscanf(pci_addr, "%4x:%2x:%2x.%2x",
> +                    &domain, &bus, &device, &function);
> +       igt_assert_f(err == 4, "Unable to find device PCI address.");

Include pci_addr in the error message.

"Unable to extract PCI device address from '%s'\n", pci_addr;

Are we always going to have PCI addresses? I guess even TB presents as
[e]PCI. Hmm, there's a is-pci? above.

> +       err = pci_system_init();
> +       igt_fail_on_f(err != 0, "Couldn't initialize PCI system\n");

Fail? Requires, I'd say. We require libpciaccess support.

> +       pci_dev = pci_device_find_by_slot(domain,
> +                                         bus,
> +                                         device,
> +                                         function);

As always with libpciaccess, one is left asking there has to be a better
way.

> +       igt_assert_f(pci_dev, "Couldn't find Intel graphics card\n");

This would be require, ... And not "Intel" specific anymore.

> +       err = pci_device_probe(pci_dev);
> +       igt_fail_on_f(err != 0,
> +                     "Couldn't probe graphics card\n");

and this would be assert. But imo, it would be nicer to return NULL. At
least provide an option to get the pci_device without asserting.

> +
> +       return pci_dev;
> +}