[prev in list] [next in list] [prev in thread] [next in thread] 

List:       fwts-devel
Subject:    NACK: [PATCH][RESEND] pci: aspm: remove the need to run and parse lspci output (LP: #1227853)
From:       Colin Ian King <colin.king () canonical ! com>
Date:       2013-09-20 16:37:20
Message-ID: 523C79C0.2020809 () canonical ! com
[Download RAW message or body]

On 20/09/13 10:06, 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 | 160 ++++++++++++++++++++--------------------------------
> 1 file changed, 60 insertions(+), 100 deletions(-)
> 
> diff --git a/src/pci/aspm/aspm.c b/src/pci/aspm/aspm.c
> index be8ee5b..7a431de 100644
> --- a/src/pci/aspm/aspm.c
> +++ b/src/pci/aspm/aspm.c
> @@ -18,16 +18,21 @@
> */
> #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 <inttypes.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 +53,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 +92,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 +103,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 +213,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.");
> 
Just spotted that the error path is free'ing incorrectly. Nack again.

> 


[prev in list] [next in list] [prev in thread] [next in thread]