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

[Chris Wilson]

Quoting Michał Winiarski (2019-03-13 17:38:21)
> +#define IGT_DEV_PATH_LEN 80
> +
> +static bool igt_device_is_pci(int fd)
> +{
> +       char path[IGT_DEV_PATH_LEN];
> +       char *subsystem;
> +       int sysfs;
> +       int len;
> +
> +       if ((sysfs = igt_sysfs_open(fd)) == -1)
> +               return false;
> +
> +       len = readlinkat(sysfs, "device/subsystem", path, sizeof(path) - 1);
> +       if (len == -1)
> +               return false;
> +       path[len] = '\0';
> +
> +       subsystem = strrchr(path, '/');
> +       if (!subsystem)
> +               return false;
> +
> +       return strcmp(subsystem, "/pci") == 0;
> +}
> +
> +struct igt_pci_addr {
> +       unsigned int domain;
> +       unsigned int bus;
> +       unsigned int device;
> +       unsigned int function;
> +};
> +
> +static int igt_device_get_pci_addr(int fd, struct igt_pci_addr *pci)
> +{
> +       char path[IGT_DEV_PATH_LEN];
> +       char *buf;
> +       int sysfs;
> +       int len;
> +
> +       if (!igt_device_is_pci(fd))
> +               return -ENODEV;
> +
> +       if ((sysfs = igt_sysfs_open(fd)) == -1)
> +               return -ENOENT;
> +
> +       len = readlinkat(sysfs, "device", path, sizeof(path) - 1);
> +       if (len == -1)
> +               return -ENOENT;
> +       path[len] = '\0';
> +
> +       buf = strrchr(path, '/');
> +       if (!buf)
> +               return -ENOENT;
> +
> +       if (sscanf(buf, "/%4x:%2x:%2x.%2x",

Heh, buf+1 :)

> +                  &pci->domain, &pci->bus,
> +                  &pci->device, &pci->function) != 4) {
> +               igt_warn("Unable to extract PCI device address from '%s'\n", buf);
> +               return -ENOENT;
> +       }
> +
> +       return 0;
> +}
> +
> +static struct pci_device *__igt_device_get_pci_device(int fd)
> +{
> +       struct igt_pci_addr pci_addr;
> +       struct pci_device *pci_dev;
> +
> +       if (igt_device_get_pci_addr(fd, &pci_addr)) {
> +               igt_warn("Unable to find device PCI address\n");
> +               return NULL;
> +       }
> +
> +       if (pci_system_init()) {
> +               igt_warn("Couldn't initialize PCI system\n");
> +               return NULL;
> +       }
> +
> +       pci_dev = pci_device_find_by_slot(pci_addr.domain,
> +                                         pci_addr.bus,
> +                                         pci_addr.device,
> +                                         pci_addr.function);
> +       if (!pci_dev) {
> +               igt_warn("Couldn't find PCI device\n");

"Couldn't find PCI device %04x:%02x:%02x:%02x\n"

> +               return NULL;
> +       }
> +
> +       if (pci_device_probe(pci_dev)) {
> +               igt_warn("Couldn't probe PCI device\n");
> +               return NULL;
> +       }
> +
> +       return pci_dev;
> +}
> +
> +/**
> + * igt_device_get_pci_device:
> + *
> + * @fd: the device
> + *
> + * Looks up the main graphics pci device using libpciaccess.
> + *
> + * Returns:
> + * The pci_device, skips the test on any failures.
> + */
> +struct pci_device *igt_device_get_pci_device(int fd)
> +{
> +       struct pci_device *pci_dev;
> +
> +       igt_require(pci_dev = __igt_device_get_pci_device(fd));
> +
> +       return pci_dev;
> +}

That looks a lot more orderly and controlled. Style-wise, probably best
to avoid assignment inside conditionals.

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris