Feel free to add my r-b to this patch.

On 2018-06-25 19:36, Emil Velikov wrote:

From: Emil Velikov emil.velikov@collabora.com

Currently we match the opened drmDevice fd with each drmDevice we process.

Move that after all the devices are processed and folded, via the drm_device_has_rdev(). This makes the code easier to follow and allows us to unify the massive process loop across drmGetDevice2 and drmGetDevices2. That in itself is coming with a later commit.

Signed-off-by: Emil Velikov emil.velikov@collabora.com

xf86drm.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c index e1bbbe99..cbc0a408 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -3705,6 +3705,21 @@ drm_device_validate_flags(uint32_t flags) return (flags & ~DRM_DEVICE_GET_PCI_REVISION); }

+static bool +drm_device_has_rdev(drmDevicePtr device, dev_t find_rdev) +{

• struct stat sbuf;
• for (int i = 0; i < DRM_NODE_MAX; i++) {

•    if (device->available_nodes & 1 << i) {
  

•        if (stat(device->nodes[i], &sbuf) == 0 &&
  

•            sbuf.st_rdev == find_rdev)
  

•            return true;
  

•    }
  

• }
• return false;

+}

• /**
  • Get information about the opened drm device

@@ -3889,21 +3904,22 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) local_devices = temp; }

•    /* store target at local_devices[0] for ease to use below */
  

•    if (find_rdev == sbuf.st_rdev && i) {
  

•        local_devices[i] = local_devices[0];
  

•        local_devices[0] = d;
  

•    }
  

•    else
  

•        local_devices[i] = d;
  

•    local_devices[i] = d;
      i++;
  }
  node_count = i;
  
  drmFoldDuplicatedDevices(local_devices, node_count);
  
  

• *device = local_devices[0];
• drmFreeDevices(&local_devices[1], node_count - 1);

• for (i = 0; i < node_count; i++) {

•    if (!local_devices[i])
  

•        continue;
  

•    if (drm_device_has_rdev(local_devices[i], find_rdev))
  

•        *device = local_devices[i];
  

•    else
  

•        drmFreeDevice(&local_devices[i]);
  

• }

  closedir(sysdir); free(local_devices);

Feel free to add my r-b to this patch.

On 2018-06-25 19:36, Emil Velikov wrote:

From: Emil Velikov emil.velikov@collabora.com

Don't the duplicate (nearly) identical code across the two call sites. It improves legibility and the diff stat seems nice.

Signed-off-by: Emil Velikov emil.velikov@collabora.com

xf86drm.c | 159 ++++++++++++++++++------------------------------------ 1 file changed, 51 insertions(+), 108 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c index cbc0a408..114cf855 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -3676,6 +3676,52 @@ free_device: return ret; }

+static int +process_device(drmDevicePtr *device, const char *d_name,

•           int req_subsystem_type,
  

•           bool fetch_deviceinfo, uint32_t flags)
  

+{

• struct stat sbuf;
• char node[PATH_MAX + 1];
• int node_type, subsystem_type;
• unsigned int maj, min;
• node_type = drmGetNodeType(d_name);
• if (node_type < 0)

•    return -1;
  

• snprintf(node, PATH_MAX, "%s/%s", DRM_DIR_NAME, d_name);
• if (stat(node, &sbuf))

•    return -1;
  

• maj = major(sbuf.st_rdev);
• min = minor(sbuf.st_rdev);
• if (maj != DRM_MAJOR || !S_ISCHR(sbuf.st_mode))

•    return -1;
  

• subsystem_type = drmParseSubsystemType(maj, min);
• if (req_subsystem_type != -1 && req_subsystem_type != subsystem_type)

•    return -1;
  

• switch (subsystem_type) {
• case DRM_BUS_PCI:

•    return drmProcessPciDevice(device, node, node_type, maj, min,
  

•                               fetch_deviceinfo, flags);
  

• case DRM_BUS_USB:

•    return drmProcessUsbDevice(device, node, node_type, maj, min,
  

•                               fetch_deviceinfo, flags);
  

• case DRM_BUS_PLATFORM:

•    return drmProcessPlatformDevice(device, node, node_type, maj, min,
  

•                                    fetch_deviceinfo, flags);
  

• case DRM_BUS_HOST1X:

•    return drmProcessHost1xDevice(device, node, node_type, maj, min,
  

•                                  fetch_deviceinfo, flags);
  

• default:

•    return -1;
  

• }

+}

• /* Consider devices located on the same bus as duplicate and fold the respective
  • entries into a single one.

@@ -3805,8 +3851,7 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) DIR *sysdir; struct dirent *dent; struct stat sbuf;

• char node[PATH_MAX + 1];
• int node_type, subsystem_type;

• int subsystem_type; int maj, min; int ret, i, node_count; int max_count = 16;

@@ -3844,55 +3889,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)

  i = 0;
  while ((dent = readdir(sysdir))) {

•    node_type = drmGetNodeType(dent->d_name);
  

•    if (node_type < 0)
  

•        continue;
  

•    snprintf(node, PATH_MAX, "%s/%s", DRM_DIR_NAME, dent->d_name);
  

•    if (stat(node, &sbuf))
  

•        continue;
  

•    maj = major(sbuf.st_rdev);
  

•    min = minor(sbuf.st_rdev);
  

•    if (maj != DRM_MAJOR || !S_ISCHR(sbuf.st_mode))
  

•        continue;
  

•    if (drmParseSubsystemType(maj, min) != subsystem_type)
  

•        continue;
  

•    switch (subsystem_type) {
  

•    case DRM_BUS_PCI:
  

•        ret = drmProcessPciDevice(&d, node, node_type, maj, min, true, flags);
  

•        if (ret)
  

•            continue;
  

•        break;
  

•    case DRM_BUS_USB:
  

•        ret = drmProcessUsbDevice(&d, node, node_type, maj, min, true, flags);
  

•        if (ret)
  

•            continue;
  

•        break;
  

•    case DRM_BUS_PLATFORM:
  

•        ret = drmProcessPlatformDevice(&d, node, node_type, maj, min, true, flags);
  

•        if (ret)
  

•            continue;
  

•        break;
  

•    case DRM_BUS_HOST1X:
  

•        ret = drmProcessHost1xDevice(&d, node, node_type, maj, min, true, flags);
  

•        if (ret)
  

•            continue;
  

•        break;
  

•    default:
  

•    ret = process_device(&d, dent->d_name, subsystem_type, true, flags);
  

•    if (ret)
          continue;
  

•    }
  
      if (i >= max_count) {
          drmDevicePtr *temp;
  

@@ -3973,10 +3972,6 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices) drmDevicePtr device; DIR *sysdir; struct dirent *dent;

• struct stat sbuf;
• char node[PATH_MAX + 1];
• int node_type, subsystem_type;
• int maj, min; int ret, i, node_count, device_count; int max_count = 16;

@@ -3995,61 +3990,9 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)

  i = 0;
  while ((dent = readdir(sysdir))) {

•    node_type = drmGetNodeType(dent->d_name);
  

•    if (node_type < 0)
  

•        continue;
  

•    snprintf(node, PATH_MAX, "%s/%s", DRM_DIR_NAME, dent->d_name);
  

•    if (stat(node, &sbuf))
  

•        continue;
  

•    maj = major(sbuf.st_rdev);
  

•    min = minor(sbuf.st_rdev);
  

•    if (maj != DRM_MAJOR || !S_ISCHR(sbuf.st_mode))
  

•        continue;
  

•    subsystem_type = drmParseSubsystemType(maj, min);
  

•    if (subsystem_type < 0)
  

•        continue;
  

•    switch (subsystem_type) {
  

•    case DRM_BUS_PCI:
  

•        ret = drmProcessPciDevice(&device, node, node_type,
  

•                                  maj, min, devices != NULL, flags);
  

•        if (ret)
  

•            continue;
  

•        break;
  

•    case DRM_BUS_USB:
  

•        ret = drmProcessUsbDevice(&device, node, node_type, maj, min,
  

•                                  devices != NULL, flags);
  

•        if (ret)
  

•            continue;
  

•        break;
  

•    case DRM_BUS_PLATFORM:
  

•        ret = drmProcessPlatformDevice(&device, node, node_type, maj, min,
  

•                                       devices != NULL, flags);
  

•        if (ret)
  

•            continue;
  

•        break;
  

•    case DRM_BUS_HOST1X:
  

•        ret = drmProcessHost1xDevice(&device, node, node_type, maj, min,
  

•                                     devices != NULL, flags);
  

•        if (ret)
  

•            continue;
  

•        break;
  

•    default:
  

•    ret = process_device(&device, dent->d_name, -1, devices != NULL, flags);
  

•    if (ret)
          continue;
  

•    }
  
      if (i >= max_count) {
          drmDevicePtr *temp;
  
  

Hey Emil,

On 2018-06-25 19:36, Emil Velikov wrote:

From: Emil Velikov emil.velikov@collabora.com

Currently we dynamically allocate 16 pointers and reallocate more as needed.

Instead, allocate the maximum number (256) on stack - the number is small enough and is unlikely to change in the foreseeable future.

This allows us to simplify the error handling and even shed a few bytes off the final binary.

Signed-off-by: Emil Velikov emil.velikov@collabora.com

xf86drm.c | 64 ++++++------------------------------------------------- 1 file changed, 6 insertions(+), 58 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c index 114cf855..d4810740 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -3846,7 +3846,7 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)

  return 0;

#else

• drmDevicePtr *local_devices;

• drmDevicePtr local_devices[256];

This number is seen later on in this patch, maybe it should be broken out into a define, since it's reused later on too at [1].

  drmDevicePtr d;
  DIR *sysdir;
  struct dirent *dent;

@@ -3854,7 +3854,6 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) int subsystem_type; int maj, min; int ret, i, node_count;

• int max_count = 16; dev_t find_rdev;

  if (drm_device_validate_flags(flags))

@@ -3877,15 +3876,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) if (subsystem_type < 0) return subsystem_type;

• local_devices = calloc(max_count, sizeof(drmDevicePtr));
• if (local_devices == NULL)

•    return -ENOMEM;
  

• sysdir = opendir(DRM_DIR_NAME);
  

• if (!sysdir) {

•    ret = -errno;
  

•    goto free_locals;
  

• }

• if (!sysdir)

•    return -errno;
  
  i = 0;
  while ((dent = readdir(sysdir))) {
  

@@ -3893,16 +3886,6 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) if (ret) continue;

•    if (i >= max_count) {
  

Is this check really not needded what exactly is it that defines 256 as the maximum?

From what I understand readdir(sysdir) is the call that defines how many devices will be looked through, and as far as I understand it can return an arbitrary number of files.

•        drmDevicePtr *temp;
  

•        max_count += 16;
  

•        temp = realloc(local_devices, max_count * sizeof(drmDevicePtr));
  

•        if (!temp)
  

•            goto free_devices;
  

•        local_devices = temp;
  

•    }
  

•     local_devices[i] = d;
      i++;
  }
  

@@ -3921,18 +3904,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) }

  closedir(sysdir);

• free(local_devices); if (*device == NULL) return -ENODEV; return 0;

-free_devices:

• drmFreeDevices(local_devices, i);
• closedir(sysdir);

-free_locals:

• free(local_devices);
• return ret; #endif }

@@ -3968,25 +3942,18 @@ int drmGetDevice(int fd, drmDevicePtr *device) */ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices) {

• drmDevicePtr *local_devices;

• drmDevicePtr local_devices[256];'

[1]

  drmDevicePtr device;
  DIR *sysdir;
  struct dirent *dent;
  int ret, i, node_count, device_count;

• int max_count = 16;

  if (drm_device_validate_flags(flags)) return -EINVAL;

• local_devices = calloc(max_count, sizeof(drmDevicePtr));

• if (local_devices == NULL)

•    return -ENOMEM;
  

• sysdir = opendir(DRM_DIR_NAME);
  

• if (!sysdir) {

•    ret = -errno;
  

•    goto free_locals;
  

• }

• if (!sysdir)

•    return -errno;
  
  i = 0;
  while ((dent = readdir(sysdir))) {
  

@@ -3994,16 +3961,6 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices) if (ret) continue;

•    if (i >= max_count) {
  

This out of bounds check should be dealt with too, if the above bounds check should be.

•        drmDevicePtr *temp;
  

•        max_count += 16;
  

•        temp = realloc(local_devices, max_count * sizeof(drmDevicePtr));
  

•        if (!temp)
  

•            goto free_devices;
  

•        local_devices = temp;
  

•    }
  

•     local_devices[i] = device;
      i++;
  }
  

@@ -4025,16 +3982,7 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices) }

  closedir(sysdir);

• free(local_devices); return device_count;

-free_devices:

• drmFreeDevices(local_devices, i);
• closedir(sysdir);

-free_locals:

• free(local_devices);

• return ret; }

  /**

From: Emil Velikov emil.velikov@collabora.com

Currently we dynamically allocate 16 pointers and reallocate more as needed.

Instead, allocate the maximum number (256) on stack - the number is small enough and is unlikely to change in the foreseeable future.

This allows us to simplify the error handling and even shed a few bytes off the final binary.

v2: - add a define & description behind the magic 256 (Rob) - report error to strerr and skip when over 256 device nodes (Rob)

Cc: Robert Foss robert.foss@collabora.com Signed-off-by: Emil Velikov emil.velikov@collabora.com Tested-by: Robert Foss robert.foss@collabora.com (v1) Reviewed-by: Robert Foss robert.foss@collabora.com (v1) Reviewed-by: Eric Engestrom eric@engestrom.ch (v1) --- xf86drm.c | 79 ++++++++++++++++--------------------------------------- 1 file changed, 23 insertions(+), 56 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c index 114cf855..02da3e1f 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -3766,6 +3766,13 @@ drm_device_has_rdev(drmDevicePtr device, dev_t find_rdev) return false; }

+/* + * The kernel drm core has a number of places that assume maximum of + * 3x64 devices nodes. That's 64 for each of primary, control and + * render nodes. Rounded it up to 256 for simplicity. + */ +#define MAX_DRM_NODES 256 + /** * Get information about the opened drm device * @@ -3846,7 +3853,7 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)

return 0; #else - drmDevicePtr *local_devices; + drmDevicePtr local_devices[MAX_DRM_NODES]; drmDevicePtr d; DIR *sysdir; struct dirent *dent; @@ -3854,7 +3861,6 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) int subsystem_type; int maj, min; int ret, i, node_count; - int max_count = 16; dev_t find_rdev;

if (drm_device_validate_flags(flags)) @@ -3877,15 +3883,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) if (subsystem_type < 0) return subsystem_type;

- local_devices = calloc(max_count, sizeof(drmDevicePtr)); - if (local_devices == NULL) - return -ENOMEM; - sysdir = opendir(DRM_DIR_NAME); - if (!sysdir) { - ret = -errno; - goto free_locals; - } + if (!sysdir) + return -errno;

i = 0; while ((dent = readdir(sysdir))) { @@ -3893,16 +3893,12 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) if (ret) continue;

- if (i >= max_count) { - drmDevicePtr *temp; - - max_count += 16; - temp = realloc(local_devices, max_count * sizeof(drmDevicePtr)); - if (!temp) - goto free_devices; - local_devices = temp; + if (i >= MAX_DRM_NODES) { + fprintf(stderr, "More than %d drm nodes detected. " + "Please report a bug - that should not happen.\n" + "Skipping extra nodes\n", MAX_DRM_NODES); + break; } - local_devices[i] = d; i++; } @@ -3921,18 +3917,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) }

closedir(sysdir); - free(local_devices); if (*device == NULL) return -ENODEV; return 0; - -free_devices: - drmFreeDevices(local_devices, i); - closedir(sysdir); - -free_locals: - free(local_devices); - return ret; #endif }

@@ -3968,25 +3955,18 @@ int drmGetDevice(int fd, drmDevicePtr *device) */ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices) { - drmDevicePtr *local_devices; + drmDevicePtr local_devices[MAX_DRM_NODES]; drmDevicePtr device; DIR *sysdir; struct dirent *dent; int ret, i, node_count, device_count; - int max_count = 16;

if (drm_device_validate_flags(flags)) return -EINVAL;

- local_devices = calloc(max_count, sizeof(drmDevicePtr)); - if (local_devices == NULL) - return -ENOMEM; - sysdir = opendir(DRM_DIR_NAME); - if (!sysdir) { - ret = -errno; - goto free_locals; - } + if (!sysdir) + return -errno;

i = 0; while ((dent = readdir(sysdir))) { @@ -3994,16 +3974,12 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices) if (ret) continue;

- if (i >= max_count) { - drmDevicePtr *temp; - - max_count += 16; - temp = realloc(local_devices, max_count * sizeof(drmDevicePtr)); - if (!temp) - goto free_devices; - local_devices = temp; + if (i >= MAX_DRM_NODES) { + fprintf(stderr, "More than %d drm nodes detected. " + "Please report a bug - that should not happen.\n" + "Skipping extra nodes\n", MAX_DRM_NODES); + break; } - local_devices[i] = device; i++; } @@ -4025,16 +4001,7 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices) }

closedir(sysdir); - free(local_devices); return device_count; - -free_devices: - drmFreeDevices(local_devices, i); - closedir(sysdir); - -free_locals: - free(local_devices); - return ret; }

/**

From: Emil Velikov emil.velikov@collabora.com

Introduce a helper which gets the real sysfs path for the given pci device.

In other words, instead opening the /sys/dev/char/*/device symlink, we opt for the actual /sys/devices/pci*/*/

It folds three (nearly identical) snprintf's and paves the way of adding extra devices (see next patch) a piece of pie.

v2: use a caller (on stack) provided real_path (Eric)

Cc: Eric Engestrom eric@engestrom.ch Signed-off-by: Emil Velikov emil.velikov@collabora.com Tested-by: Robert Foss robert.foss@collabora.com (v1) Reviewed-by: Robert Foss robert.foss@collabora.com (v1) Reviewed-by: Eric Engestrom eric@engestrom.ch (v1) --- Eric, I couldn't quite remove all the error handling since on realpath() failure the output buffer contents are undefined :-\

xf86drm.c | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c index 02da3e1f..51e00d23 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -2992,16 +2992,29 @@ static int drmParseSubsystemType(int maj, int min) #endif }

+static char * +get_real_pci_path(int maj, int min, char *real_path) +{ + char path[PATH_MAX + 1]; + + snprintf(path, sizeof(path), "/sys/dev/char/%d:%d/device", maj, min); + if (!realpath(path, real_path)) + return NULL; + + return real_path; +} + static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info) { #ifdef __linux__ unsigned int domain, bus, dev, func; - char path[PATH_MAX + 1], *value; + char real_path[PATH_MAX + 1], *value; int num;

- snprintf(path, sizeof(path), "/sys/dev/char/%d:%d/device", maj, min); + if (get_real_pci_path(maj, min, real_path) == NULL) + return -ENOENT;

- value = sysfs_uevent_get(path, "PCI_SLOT_NAME"); + value = sysfs_uevent_get(real_path, "PCI_SLOT_NAME"); if (!value) return -ENOENT;

@@ -3114,14 +3127,16 @@ static int parse_separate_sysfs_files(int maj, int min, "subsystem_vendor", "subsystem_device", }; - char path[PATH_MAX + 1]; + char path[PATH_MAX + 1], real_path[PATH_MAX + 1]; unsigned int data[ARRAY_SIZE(attrs)]; FILE *fp; int ret;

+ if (get_real_pci_path(maj, min, real_path) == NULL) + return -ENOENT; + for (unsigned i = ignore_revision ? 1 : 0; i < ARRAY_SIZE(attrs); i++) { - snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/%s", maj, min, - attrs[i]); + snprintf(path, PATH_MAX, "%s/%s", real_path, attrs[i]); fp = fopen(path, "r"); if (!fp) return -errno; @@ -3145,11 +3160,14 @@ static int parse_separate_sysfs_files(int maj, int min, static int parse_config_sysfs_file(int maj, int min, drmPciDeviceInfoPtr device) { - char path[PATH_MAX + 1]; + char path[PATH_MAX + 1], real_path[PATH_MAX + 1]; unsigned char config[64]; int fd, ret;

- snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/config", maj, min); + if (get_real_pci_path(maj, min, real_path) == NULL) + return -ENOENT; + + snprintf(path, PATH_MAX, "%s/config", real_path); fd = open(path, O_RDONLY); if (fd < 0) return -errno;

On Monday, 2018-06-25 17:39:14 +0000, Emil Velikov wrote:

From: Emil Velikov emil.velikov@collabora.com

Introduce a helper which gets the real sysfs path for the given pci device.

In other words, instead opening the /sys/dev/char/*/device symlink, we opt for the actual /sys/devices/pci*/*/

It folds three (nearly identical) snprintf's and paves the way of adding extra devices (see next patch) a piece of pie.

Signed-off-by: Emil Velikov emil.velikov@collabora.com

xf86drm.c | 62 ++++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 12 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c index d4810740..8ccd528f 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -2992,16 +2992,37 @@ static int drmParseSubsystemType(int maj, int min) #endif }

+static char * +get_real_pci_path(int maj, int min) +{

• char path[PATH_MAX + 1];
• char *real_path = malloc(PATH_MAX);

Why not allocate this on the stack and pass it in? It would avoid the error handling you had to add in this patch :)

• if (!real_path)

•    return NULL;
  

• snprintf(path, sizeof(path), "/sys/dev/char/%d:%d/device", maj, min);
• if (!realpath(path, real_path)) {

•    free(real_path);
  

•    return NULL;
  

• }
• return real_path;

+}

static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info) { #ifdef __linux__ unsigned int domain, bus, dev, func;

• char path[PATH_MAX + 1], *value;

• char *real_path, *value; int num;

• snprintf(path, sizeof(path), "/sys/dev/char/%d:%d/device", maj, min);

• real_path = get_real_pci_path(maj, min);
• if (!real_path)

•    return -ENOENT;
  
  

• value = sysfs_uevent_get(path, "PCI_SLOT_NAME");

• value = sysfs_uevent_get(real_path, "PCI_SLOT_NAME");
• free(real_path); if (!value) return -ENOENT;

@@ -3114,24 +3135,32 @@ static int parse_separate_sysfs_files(int maj, int min, "subsystem_vendor", "subsystem_device", };

• char path[PATH_MAX + 1];

• char path[PATH_MAX + 1], *real_path; unsigned int data[ARRAY_SIZE(attrs)]; FILE *fp; int ret;

• real_path = get_real_pci_path(maj, min);

• if (!real_path)

•    return -ENOENT;
  

• for (unsigned i = ignore_revision ? 1 : 0; i < ARRAY_SIZE(attrs); i++) {

•    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/%s", maj, min,
  

•             attrs[i]);
  

•    snprintf(path, PATH_MAX, "%s/%s", real_path, attrs[i]);
     fp = fopen(path, "r");
  

•    if (!fp)
  

•    if (!fp) {
  

•        free(real_path);
         return -errno;
  

•    }
  
     ret = fscanf(fp, "%x", &data[i]);
     fclose(fp);
  

•    if (ret != 1)
  

•    if (ret != 1) {
  

•        free(real_path);
         return -errno;
  

•    }
  
  

  }

• free(real_path);

  device->revision_id = ignore_revision ? 0xff : data[0] & 0xff; device->vendor_id = data[1] & 0xffff;

@@ -3145,19 +3174,28 @@ static int parse_separate_sysfs_files(int maj, int min, static int parse_config_sysfs_file(int maj, int min, drmPciDeviceInfoPtr device) {

• char path[PATH_MAX + 1];

• char path[PATH_MAX + 1], *real_path; unsigned char config[64]; int fd, ret;

• snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/config", maj, min);

• real_path = get_real_pci_path(maj, min);
• if (!real_path)

•    return -ENOENT;
  

• snprintf(path, PATH_MAX, "%s/config", real_path); fd = open(path, O_RDONLY);

• if (fd < 0)

• if (fd < 0) {

•    free(real_path);
     return -errno;
  

• }

  ret = read(fd, config, sizeof(config)); close(fd);

• if (ret < 0)

• if (ret < 0) {

•    free(real_path);
     return -errno;
  

• }

• free(real_path);

  device->vendor_id = config[0] | (config[1] << 8); device->device_id = config[2] | (config[3] << 8);

-- 2.18.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Hi Eric,

On 28 June 2018 at 11:21, Eric Engestrom eric@engestrom.ch wrote:

...

@@ -2992,16 +2992,37 @@ static int drmParseSubsystemType(int maj, int min) #endif }

+static char * +get_real_pci_path(int maj, int min) +{

• char path[PATH_MAX + 1];
• char *real_path = malloc(PATH_MAX);

Why not allocate this on the stack and pass it in? It would avoid the error handling you had to add in this patch :)

As always, thanks for having a look.

I was mostly hesitant since some callers already have a char foo[PATH_MAX +1]. Thinking about it - the default stack size should be enough to accommodate an extra ~4k bytes.

Will fix.

Thanks Emil

On 28 June 2018 at 11:19, Eric Engestrom eric@engestrom.ch wrote:

On Monday, 2018-06-25 17:40:02 +0000, Emil Velikov wrote:

...

From: Emil Velikov emil.velikov@collabora.com

Making the output a little bit easier to parse by human beings.

Signed-off-by: Emil Velikov emil.velikov@collabora.com

tests/drmdevice.c | 78 +++++++++++++++++++++++------------------------ 1 file changed, 39 insertions(+), 39 deletions(-)

diff --git a/tests/drmdevice.c b/tests/drmdevice.c index 0d75836f..e9e9d7f1 100644 --- a/tests/drmdevice.c +++ b/tests/drmdevice.c @@ -36,67 +36,67 @@ static void print_device_info(drmDevicePtr device, int i, bool print_revision) { printf("device[%i]\n", i);

• printf("\tavailable_nodes %04x\n", device->available_nodes);
• printf("\tnodes\n");

• printf("+->available_nodes %#04x\n", device->available_nodes);
• printf("+->nodes\n");

Nit: I'd put a space between `>` and the text, for readability

Ack, will do.

Thanks Emil

From: Emil Velikov emil.velikov@collabora.com

Making the output a little bit easier to parse by human beings.

v2: Add extra whitespace (Eric)

Cc: Eric Engestrom eric@engestrom.ch Signed-off-by: Emil Velikov emil.velikov@collabora.com Tested-by: Robert Foss robert.foss@collabora.com (v1) Reviewed-by: Robert Foss robert.foss@collabora.com (v1) Reviewed-by: Eric Engestrom eric@engestrom.ch (v1) --- tests/drmdevice.c | 77 +++++++++++++++++++++++------------------------ 1 file changed, 38 insertions(+), 39 deletions(-)

diff --git a/tests/drmdevice.c b/tests/drmdevice.c index 0d75836f..049df3cf 100644 --- a/tests/drmdevice.c +++ b/tests/drmdevice.c @@ -36,67 +36,66 @@ static void print_device_info(drmDevicePtr device, int i, bool print_revision) { printf("device[%i]\n", i); - printf("\tavailable_nodes %04x\n", device->available_nodes); - printf("\tnodes\n"); + printf("+-> available_nodes %#04x\n", device->available_nodes); + printf("+-> nodes\n"); for (int j = 0; j < DRM_NODE_MAX; j++) if (device->available_nodes & 1 << j) - printf("\t\tnodes[%d] %s\n", j, device->nodes[j]); + printf("| +-> nodes[%d] %s\n", j, device->nodes[j]);

- printf("\tbustype %04x\n", device->bustype); - printf("\tbusinfo\n"); + printf("+-> bustype %04x\n", device->bustype); if (device->bustype == DRM_BUS_PCI) { - printf("\t\tpci\n"); - printf("\t\t\tdomain\t%04x\n",device->businfo.pci->domain); - printf("\t\t\tbus\t%02x\n", device->businfo.pci->bus); - printf("\t\t\tdev\t%02x\n", device->businfo.pci->dev); - printf("\t\t\tfunc\t%1u\n", device->businfo.pci->func); - - printf("\tdeviceinfo\n"); - printf("\t\tpci\n"); - printf("\t\t\tvendor_id\t%04x\n", device->deviceinfo.pci->vendor_id); - printf("\t\t\tdevice_id\t%04x\n", device->deviceinfo.pci->device_id); - printf("\t\t\tsubvendor_id\t%04x\n", device->deviceinfo.pci->subvendor_id); - printf("\t\t\tsubdevice_id\t%04x\n", device->deviceinfo.pci->subdevice_id); + printf("| +-> pci\n"); + printf("| +-> domain %04x\n",device->businfo.pci->domain); + printf("| +-> bus %02x\n", device->businfo.pci->bus); + printf("| +-> dev %02x\n", device->businfo.pci->dev); + printf("| +-> func %1u\n", device->businfo.pci->func); + + printf("+-> deviceinfo\n"); + printf(" +-> pci\n"); + printf(" +-> vendor_id %04x\n", device->deviceinfo.pci->vendor_id); + printf(" +-> device_id %04x\n", device->deviceinfo.pci->device_id); + printf(" +-> subvendor_id %04x\n", device->deviceinfo.pci->subvendor_id); + printf(" +-> subdevice_id %04x\n", device->deviceinfo.pci->subdevice_id); if (print_revision) - printf("\t\t\trevision_id\t%02x\n", device->deviceinfo.pci->revision_id); + printf(" +-> revision_id %02x\n", device->deviceinfo.pci->revision_id); else - printf("\t\t\trevision_id\tIGNORED\n"); + printf(" +-> revision_id IGNORED\n");

} else if (device->bustype == DRM_BUS_USB) { - printf("\t\tusb\n"); - printf("\t\t\tbus\t%03u\n", device->businfo.usb->bus); - printf("\t\t\tdev\t%03u\n", device->businfo.usb->dev); - - printf("\tdeviceinfo\n"); - printf("\t\tusb\n"); - printf("\t\t\tvendor\t%04x\n", device->deviceinfo.usb->vendor); - printf("\t\t\tproduct\t%04x\n", device->deviceinfo.usb->product); + printf("| +-> usb\n"); + printf("| +-> bus %03u\n", device->businfo.usb->bus); + printf("| +-> dev %03u\n", device->businfo.usb->dev); + + printf("+-> deviceinfo\n"); + printf(" +-> usb\n"); + printf(" +-> vendor %04x\n", device->deviceinfo.usb->vendor); + printf(" +-> product %04x\n", device->deviceinfo.usb->product); } else if (device->bustype == DRM_BUS_PLATFORM) { char **compatible = device->deviceinfo.platform->compatible;

- printf("\t\tplatform\n"); - printf("\t\t\tfullname\t%s\n", device->businfo.platform->fullname); + printf("| +-> platform\n"); + printf("| +-> fullname\t%s\n", device->businfo.platform->fullname);

- printf("\tdeviceinfo\n"); - printf("\t\tplatform\n"); - printf("\t\t\tcompatible\n"); + printf("+-> deviceinfo\n"); + printf(" +-> platform\n"); + printf(" +-> compatible\n");

while (*compatible) { - printf("\t\t\t\t%s\n", *compatible); + printf(" %s\n", *compatible); compatible++; } } else if (device->bustype == DRM_BUS_HOST1X) { char **compatible = device->deviceinfo.platform->compatible;

- printf("\t\thost1x\n"); - printf("\t\t\tfullname\t%s\n", device->businfo.host1x->fullname); + printf("| +-> host1x\n"); + printf("| +-> fullname\t%s\n", device->businfo.host1x->fullname);

- printf("\tdeviceinfo\n"); - printf("\t\tplatform\n"); - printf("\t\t\tcompatible\n"); + printf("+-> deviceinfo\n"); + printf(" +-> platform\n"); + printf(" +-> compatible\n");

while (*compatible) { - printf("\t\t\t\t%s\n", *compatible); + printf(" %s\n", *compatible); compatible++; } } else {

Nice catch!

Feel free to add my r-b to this patch.

On 2018-06-25 19:36, Emil Velikov wrote:

From: Emil Velikov emil.velikov@collabora.com

While fairly close, the host1x and platform are two separate things.

Signed-off-by: Emil Velikov emil.velikov@collabora.com

tests/drmdevice.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/drmdevice.c b/tests/drmdevice.c index e9e9d7f1..97ce8ff9 100644 --- a/tests/drmdevice.c +++ b/tests/drmdevice.c @@ -86,13 +86,13 @@ print_device_info(drmDevicePtr device, int i, bool print_revision) compatible++; } } else if (device->bustype == DRM_BUS_HOST1X) {

•    char **compatible = device->deviceinfo.platform->compatible;
  

•    char **compatible = device->deviceinfo.host1x->compatible;
  
      printf("|  +->host1x\n");
      printf("|     +->fullname\t%s\n", device->businfo.host1x->fullname);
  
      printf("+->deviceinfo\n");
  

•    printf("   +->platform\n");
  

•    printf("   +->host1x\n");
      printf("      +->compatible\n");
  
      while (*compatible) {