Patchwork pci: aspm: remove the need to run and parse lspci output (LP: #1227853)

login
register
mail settings
Submitter Colin King
Date Sept. 19, 2013, 7:46 p.m.
Message ID <1379620016-8494-1-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/276049/
State Rejected
Headers show

Comments

Colin King - Sept. 19, 2013, 7:46 p.m.
From: Colin Ian King <colin.king@canonical.com>

We can simplify the ASPM test by removing the need to run lspci and parse
the output.  Instead, just scan /sys/bus/pci/devices and read the PCI
config from the relevant config file.

While we are at it, also remove the redundant aspm parameter from
facp_get_aspm_control as we don't do anything with it.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/pci/aspm/aspm.c | 159 +++++++++++++++++++---------------------------------
 1 file changed, 59 insertions(+), 100 deletions(-)
Colin King - Sept. 20, 2013, 8:57 a.m.
On 19/09/13 20:46, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> We can simplify the ASPM test by removing the need to run lspci and parse
> the output.  Instead, just scan /sys/bus/pci/devices and read the PCI
> config from the relevant config file.
> 
> While we are at it, also remove the redundant aspm parameter from
> facp_get_aspm_control as we don't do anything with it.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/pci/aspm/aspm.c | 159 +++++++++++++++++++---------------------------------
>  1 file changed, 59 insertions(+), 100 deletions(-)
> 
> diff --git a/src/pci/aspm/aspm.c b/src/pci/aspm/aspm.c
> index be8ee5b..018ebbd 100644
> --- a/src/pci/aspm/aspm.c
> +++ b/src/pci/aspm/aspm.c
> @@ -18,16 +18,20 @@
>   */
>  #include <stdlib.h>
>  #include <stdio.h>
> +#include <stdint.h>
>  #include <errno.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <sys/wait.h>
> +#include <dirent.h>
>  #include <unistd.h>
>  #include <string.h>
>  #include <fcntl.h>
>  #include <limits.h>
>  #include "fwts.h"
>  
> +#define PCI_DEV_PATH			"/sys/bus/pci/devices"
> +
>  /* PCI Confiiguration Space for Type 0 & 1 */
>  #define FWTS_PCI_HEADER_TYPE		0x0E
>  #define FWTS_PCI_CAPABILITIES_POINTER	0x34
> @@ -48,17 +52,16 @@
>  #define FWTS_PCIE_ASPM_CONTROL_L1_FIELD	0x0002
>  #define FWTS_PCIE_ASPM_CONTROL_FIELD	(FWTS_PCIE_ASPM_CONTROL_L0_FIELD | FWTS_PCIE_ASPM_CONTROL_L1_FIELD)
>  
> -
>  struct pci_device {
>  	uint8_t segment;
>  	uint8_t bus;
>  	uint8_t dev;
>  	uint8_t func;
>  	uint8_t config[256];
> -	struct pci_device *next;
>  };
>  
> -/* PCI Express Capability Structure is defined in Section 7.8
> +/* 
> + * PCI Express Capability Structure is defined in Section 7.8
>   * of PCI Express®i Base Specification Revision 2.0
>   */
>  typedef struct {
> @@ -88,7 +91,7 @@ typedef struct {
>  	uint16_t slot_status2;
>  } __attribute__ ((packed)) pcie_capability;
>  
> -static int facp_get_aspm_control(fwts_framework *fw, int *aspm)
> +static int facp_get_aspm_control(fwts_framework *fw)
>  {
>  	fwts_acpi_table_info *table;
>  	fwts_acpi_table_fadt *fadt;
> @@ -99,10 +102,8 @@ static int facp_get_aspm_control(fwts_framework *fw, int *aspm)
>  	fadt = (fwts_acpi_table_fadt*)table->data;
>  
>  	if ((fadt->iapc_boot_arch & FWTS_FACP_IAPC_BOOT_ARCH_PCIE_ASPM_CONTROLS) == 0) {
> -		*aspm = 1;
>  		fwts_log_info(fw, "PCIe ASPM is controlled by Linux kernel.");
>  	} else {
> -		*aspm = 0;
>  		fwts_log_info(fw, "PCIe ASPM is not controlled by Linux kernel.");
>  		fwts_advice(fw,
>  			"BIOS reports that Linux kernel should not modify ASPM "
> @@ -211,126 +212,84 @@ static int pcie_compare_rp_dev_aspm_registers(fwts_framework *fw,
>  	return ret;
>  }
>  
> -static void pci_device_list_free(struct pci_device *head)
> -{
> -	struct pci_device *next;
> -
> -	while (head) {
> -		next = head->next;
> -		free(head);
> -		head = next;
> -	}
> -}
> -
>  static int pcie_check_aspm_registers(fwts_framework *fw)
>  {
> -	fwts_list *lspci_output;
> -	fwts_list_link *item;
> -	struct pci_device *head = NULL, *cur = NULL, *device = NULL;
> -	char command[PATH_MAX];
> -	int ret = FWTS_OK;
> -	int status;
> +	DIR *dirp;
> +	struct dirent *entry;
> +	fwts_list dev_list;
> +	fwts_list_link *lcur, *ltarget;
>  
> -	snprintf(command, sizeof(command), "%s", fw->lspci);
> +	fwts_list_init(&dev_list);
>  
> -	if (fwts_pipe_exec(command, &lspci_output, &status) != FWTS_OK) {
> -		fwts_log_warning(fw, "Could not execute %s.", command);
> +	if ((dirp = opendir(PCI_DEV_PATH)) == NULL) {
> +		fwts_log_warning(fw, "Could not open %s.", PCI_DEV_PATH);
>  		return FWTS_ERROR;
>  	}
> -	if (lspci_output == NULL) {
> -		fwts_log_warning(fw, "Output from lspci was empty.");
> -		return FWTS_ERROR;
> -	}
> -
> -	/* Get the list of pci devices and their configuration */
> -	fwts_list_foreach(item, lspci_output) {
> -		char *line = fwts_text_list_text(item);
> -		char *pEnd;
> -
> -		device = (struct pci_device *) calloc(1, sizeof(*device));
> -		if (device == NULL) {
> -			pci_device_list_free(head);
> -			fwts_text_list_free(lspci_output);
> -			return FWTS_ERROR;
> -		}
> -
> -		device->bus = strtol(line, &pEnd, 16);
> -		device->dev = strtol(pEnd + 1, &pEnd, 16);
> -		device->func = strtol(pEnd + 1, &pEnd, 16);
> -
> -		if (head == NULL)
> -			head = device;
> -		else
> -			cur->next = device;
> -
> -		cur = device;
> -	}
> -	fwts_text_list_free(lspci_output);
> -
> -	for (cur = head; cur; cur = cur->next) {
> -		int reg_loc = 0, reg_val = 0;
> -		int i;
> -		int status;
> -
> -		snprintf(command, sizeof(command), "%s -s %02X:%02X.%02X -xxx",
> -			fw->lspci, cur->bus, cur->dev, cur->func);
> -		if (fwts_pipe_exec(command, &lspci_output, &status) != FWTS_OK) {
> -			fwts_log_warning(fw, "Could not execute %s", command);
> -			pci_device_list_free(head);
> -			return FWTS_ERROR;
> -		}
> -		if (lspci_output == NULL) {
> -			pci_device_list_free(head);
> -			return FWTS_ERROR;
> -		}
> -
> -		fwts_list_foreach(item, lspci_output) {
> -			char *line = fwts_text_list_text(item);
> -			char *pEnd;
> +	while ((entry = readdir(dirp)) != NULL) {
> +		uint8_t bus, dev, func;
> +
> +		if (entry->d_name[0] == '.')
> +			continue;
> +
> +		if (sscanf(entry->d_name, "%*x:%" SCNx8 ":%" SCNx8 ".%" SCNx8, &bus, &dev, &func) == 3) {
> +			int fd;
> +			char path[PATH_MAX];
> +			struct pci_device *device;
> +
> +			device = (struct pci_device *)calloc(1, sizeof(struct pci_device));
> +			if (device == NULL) {
> +				fwts_list_free(&dev_list, free);
> +				closedir(dirp);
> +				return FWTS_ERROR;
> +			}
> +			device->bus = bus;
> +			device->dev = dev;
> +			device->func = func;
> +
> +			snprintf(path, sizeof(path), PCI_DEV_PATH "/%s/config", entry->d_name);
> +			if ((fd = open(path, O_RDONLY)) < 0) {
> +				fwts_log_warning(fw, "Could not open config from PCI device %s\n", entry->d_name);
> +				free(device);
> +				continue;
> +			}
>  
> -			if (strlen(line) >= 3 && line[3] == ' ') {
> -				reg_val = strtol(line, &pEnd, 16);
> -				for (i = 0; reg_loc < 256 && i < 16; i++) {
> -					reg_val = strtol(pEnd + 1, &pEnd, 16);
> -					cur->config[reg_loc] = reg_val;
> -					reg_loc++;
> -				}
> +			if (read(fd, device->config, sizeof(device->config)) < 0) {
> +				fwts_log_warning(fw, "Could not read config from PCI device %s\n", entry->d_name);
> +				free(device);
> +				close(fd);
> +				continue;
>  			}
> +			close(fd);
> +			fwts_list_append(&dev_list, device);
>  		}
> -		fwts_text_list_free(lspci_output);
>  	}
> +	closedir(dirp);
>  
>  	/* Check aspm registers from the list of pci devices */
> -	for (cur = head; cur; cur = cur->next) {
> -		struct pci_device *target;
> +	for (lcur = dev_list.head; lcur; lcur = lcur->next) {
> +		struct pci_device *target, *cur = (struct pci_device *)lcur->data;
>  
>  		/* Find PCI Bridge (PCIE Root Port) and the attached device  */
>  		if (cur->config[FWTS_PCI_HEADER_TYPE] & 0x01) {
> -			target = head;
> -			while (target != NULL) {
> -				if (target->bus == cur->config[FWTS_PCI_SECONDARD_BUS_NUMBER])
> +			for (ltarget = dev_list.head; ltarget; ltarget = ltarget->next) {
> +				target = (struct pci_device *)ltarget->data;
> +				if (target->bus == cur->config[FWTS_PCI_SECONDARD_BUS_NUMBER]) {
> +					pcie_compare_rp_dev_aspm_registers(fw, cur, target);
>  					break;
> -				target = target->next;
> -			}
> -			if (target == NULL) {
> -				continue;
> +				}
>  			}
> -
> -			pcie_compare_rp_dev_aspm_registers(fw, cur, target);
>  		}
>  	}
> +	fwts_list_free_items(&dev_list, free);
>  
> -	pci_device_list_free(head);
> -
> -	return ret;
> +	return FWTS_OK;
>  }
>  
>  static int aspm_check_configuration(fwts_framework *fw)
>  {
>  	int ret;
> -	int aspm_facp;
>  
> -	ret = facp_get_aspm_control(fw, &aspm_facp);
> +	ret = facp_get_aspm_control(fw);
>  	if (ret == FWTS_ERROR)
>  		fwts_skipped(fw, "No valid FACP information present: cannot test ASPM.");
>  
> 
NACK, I'll re-send the patch later with a fix for build failures that
occur on older systems.

Colin

Patch

diff --git a/src/pci/aspm/aspm.c b/src/pci/aspm/aspm.c
index be8ee5b..018ebbd 100644
--- a/src/pci/aspm/aspm.c
+++ b/src/pci/aspm/aspm.c
@@ -18,16 +18,20 @@ 
  */
 #include <stdlib.h>
 #include <stdio.h>
+#include <stdint.h>
 #include <errno.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/wait.h>
+#include <dirent.h>
 #include <unistd.h>
 #include <string.h>
 #include <fcntl.h>
 #include <limits.h>
 #include "fwts.h"
 
+#define PCI_DEV_PATH			"/sys/bus/pci/devices"
+
 /* PCI Confiiguration Space for Type 0 & 1 */
 #define FWTS_PCI_HEADER_TYPE		0x0E
 #define FWTS_PCI_CAPABILITIES_POINTER	0x34
@@ -48,17 +52,16 @@ 
 #define FWTS_PCIE_ASPM_CONTROL_L1_FIELD	0x0002
 #define FWTS_PCIE_ASPM_CONTROL_FIELD	(FWTS_PCIE_ASPM_CONTROL_L0_FIELD | FWTS_PCIE_ASPM_CONTROL_L1_FIELD)
 
-
 struct pci_device {
 	uint8_t segment;
 	uint8_t bus;
 	uint8_t dev;
 	uint8_t func;
 	uint8_t config[256];
-	struct pci_device *next;
 };
 
-/* PCI Express Capability Structure is defined in Section 7.8
+/* 
+ * PCI Express Capability Structure is defined in Section 7.8
  * of PCI Express®i Base Specification Revision 2.0
  */
 typedef struct {
@@ -88,7 +91,7 @@  typedef struct {
 	uint16_t slot_status2;
 } __attribute__ ((packed)) pcie_capability;
 
-static int facp_get_aspm_control(fwts_framework *fw, int *aspm)
+static int facp_get_aspm_control(fwts_framework *fw)
 {
 	fwts_acpi_table_info *table;
 	fwts_acpi_table_fadt *fadt;
@@ -99,10 +102,8 @@  static int facp_get_aspm_control(fwts_framework *fw, int *aspm)
 	fadt = (fwts_acpi_table_fadt*)table->data;
 
 	if ((fadt->iapc_boot_arch & FWTS_FACP_IAPC_BOOT_ARCH_PCIE_ASPM_CONTROLS) == 0) {
-		*aspm = 1;
 		fwts_log_info(fw, "PCIe ASPM is controlled by Linux kernel.");
 	} else {
-		*aspm = 0;
 		fwts_log_info(fw, "PCIe ASPM is not controlled by Linux kernel.");
 		fwts_advice(fw,
 			"BIOS reports that Linux kernel should not modify ASPM "
@@ -211,126 +212,84 @@  static int pcie_compare_rp_dev_aspm_registers(fwts_framework *fw,
 	return ret;
 }
 
-static void pci_device_list_free(struct pci_device *head)
-{
-	struct pci_device *next;
-
-	while (head) {
-		next = head->next;
-		free(head);
-		head = next;
-	}
-}
-
 static int pcie_check_aspm_registers(fwts_framework *fw)
 {
-	fwts_list *lspci_output;
-	fwts_list_link *item;
-	struct pci_device *head = NULL, *cur = NULL, *device = NULL;
-	char command[PATH_MAX];
-	int ret = FWTS_OK;
-	int status;
+	DIR *dirp;
+	struct dirent *entry;
+	fwts_list dev_list;
+	fwts_list_link *lcur, *ltarget;
 
-	snprintf(command, sizeof(command), "%s", fw->lspci);
+	fwts_list_init(&dev_list);
 
-	if (fwts_pipe_exec(command, &lspci_output, &status) != FWTS_OK) {
-		fwts_log_warning(fw, "Could not execute %s.", command);
+	if ((dirp = opendir(PCI_DEV_PATH)) == NULL) {
+		fwts_log_warning(fw, "Could not open %s.", PCI_DEV_PATH);
 		return FWTS_ERROR;
 	}
-	if (lspci_output == NULL) {
-		fwts_log_warning(fw, "Output from lspci was empty.");
-		return FWTS_ERROR;
-	}
-
-	/* Get the list of pci devices and their configuration */
-	fwts_list_foreach(item, lspci_output) {
-		char *line = fwts_text_list_text(item);
-		char *pEnd;
-
-		device = (struct pci_device *) calloc(1, sizeof(*device));
-		if (device == NULL) {
-			pci_device_list_free(head);
-			fwts_text_list_free(lspci_output);
-			return FWTS_ERROR;
-		}
-
-		device->bus = strtol(line, &pEnd, 16);
-		device->dev = strtol(pEnd + 1, &pEnd, 16);
-		device->func = strtol(pEnd + 1, &pEnd, 16);
-
-		if (head == NULL)
-			head = device;
-		else
-			cur->next = device;
-
-		cur = device;
-	}
-	fwts_text_list_free(lspci_output);
-
-	for (cur = head; cur; cur = cur->next) {
-		int reg_loc = 0, reg_val = 0;
-		int i;
-		int status;
-
-		snprintf(command, sizeof(command), "%s -s %02X:%02X.%02X -xxx",
-			fw->lspci, cur->bus, cur->dev, cur->func);
-		if (fwts_pipe_exec(command, &lspci_output, &status) != FWTS_OK) {
-			fwts_log_warning(fw, "Could not execute %s", command);
-			pci_device_list_free(head);
-			return FWTS_ERROR;
-		}
-		if (lspci_output == NULL) {
-			pci_device_list_free(head);
-			return FWTS_ERROR;
-		}
-
-		fwts_list_foreach(item, lspci_output) {
-			char *line = fwts_text_list_text(item);
-			char *pEnd;
+	while ((entry = readdir(dirp)) != NULL) {
+		uint8_t bus, dev, func;
+
+		if (entry->d_name[0] == '.')
+			continue;
+
+		if (sscanf(entry->d_name, "%*x:%" SCNx8 ":%" SCNx8 ".%" SCNx8, &bus, &dev, &func) == 3) {
+			int fd;
+			char path[PATH_MAX];
+			struct pci_device *device;
+
+			device = (struct pci_device *)calloc(1, sizeof(struct pci_device));
+			if (device == NULL) {
+				fwts_list_free(&dev_list, free);
+				closedir(dirp);
+				return FWTS_ERROR;
+			}
+			device->bus = bus;
+			device->dev = dev;
+			device->func = func;
+
+			snprintf(path, sizeof(path), PCI_DEV_PATH "/%s/config", entry->d_name);
+			if ((fd = open(path, O_RDONLY)) < 0) {
+				fwts_log_warning(fw, "Could not open config from PCI device %s\n", entry->d_name);
+				free(device);
+				continue;
+			}
 
-			if (strlen(line) >= 3 && line[3] == ' ') {
-				reg_val = strtol(line, &pEnd, 16);
-				for (i = 0; reg_loc < 256 && i < 16; i++) {
-					reg_val = strtol(pEnd + 1, &pEnd, 16);
-					cur->config[reg_loc] = reg_val;
-					reg_loc++;
-				}
+			if (read(fd, device->config, sizeof(device->config)) < 0) {
+				fwts_log_warning(fw, "Could not read config from PCI device %s\n", entry->d_name);
+				free(device);
+				close(fd);
+				continue;
 			}
+			close(fd);
+			fwts_list_append(&dev_list, device);
 		}
-		fwts_text_list_free(lspci_output);
 	}
+	closedir(dirp);
 
 	/* Check aspm registers from the list of pci devices */
-	for (cur = head; cur; cur = cur->next) {
-		struct pci_device *target;
+	for (lcur = dev_list.head; lcur; lcur = lcur->next) {
+		struct pci_device *target, *cur = (struct pci_device *)lcur->data;
 
 		/* Find PCI Bridge (PCIE Root Port) and the attached device  */
 		if (cur->config[FWTS_PCI_HEADER_TYPE] & 0x01) {
-			target = head;
-			while (target != NULL) {
-				if (target->bus == cur->config[FWTS_PCI_SECONDARD_BUS_NUMBER])
+			for (ltarget = dev_list.head; ltarget; ltarget = ltarget->next) {
+				target = (struct pci_device *)ltarget->data;
+				if (target->bus == cur->config[FWTS_PCI_SECONDARD_BUS_NUMBER]) {
+					pcie_compare_rp_dev_aspm_registers(fw, cur, target);
 					break;
-				target = target->next;
-			}
-			if (target == NULL) {
-				continue;
+				}
 			}
-
-			pcie_compare_rp_dev_aspm_registers(fw, cur, target);
 		}
 	}
+	fwts_list_free_items(&dev_list, free);
 
-	pci_device_list_free(head);
-
-	return ret;
+	return FWTS_OK;
 }
 
 static int aspm_check_configuration(fwts_framework *fw)
 {
 	int ret;
-	int aspm_facp;
 
-	ret = facp_get_aspm_control(fw, &aspm_facp);
+	ret = facp_get_aspm_control(fw);
 	if (ret == FWTS_ERROR)
 		fwts_skipped(fw, "No valid FACP information present: cannot test ASPM.");