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

List:       fwts-devel
Subject:    ACK: [PATCH] pci: aspm: Add segment support
From:       Alex Hung <alex.hung () canonical ! com>
Date:       2016-09-07 2:16:47
Message-ID: 19ced4cf-aa49-940b-c2b7-bb0cb79c9735 () canonical ! com
[Download RAW message or body]

On 2016-09-07 03:42 AM, Jeffrey Hugo wrote:
> PCI segement groups are the first level of relationship between PCI devices
> in a system.  PCI segments allow bus numbers to be reused as a particular
> bus ID must be unique only within the segment upon which it resides.
> 
> Segment support is important as a system may have multiple root ports and
> attached devices across multiple segements, with different ASPM settings
> for each port/device set.  If the test is not aware of segments, it may
> incorrectly match a root port from segment A with a device from segment B
> and cause a test failure if that port and device happen to not have
> matching ASPM settings.
> 
> Change-Id: I2aea72d12ca1dc945a4f73cf5cc652c6ffc62654
> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
> ---
> src/pci/aspm/aspm.c | 36 ++++++++++++++++++++----------------
> 1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/src/pci/aspm/aspm.c b/src/pci/aspm/aspm.c
> index 9c3c143..72bee44 100644
> --- a/src/pci/aspm/aspm.c
> +++ b/src/pci/aspm/aspm.c
> @@ -108,29 +108,29 @@ static int pcie_compare_rp_dev_aspm_registers(fwts_framework \
> *fw, 
> 	if (((rp_cap->link_cap & FWTS_PCIE_ASPM_SUPPORT_L0_FIELD) >> 10) !=
> 		(rp_cap->link_contrl & FWTS_PCIE_ASPM_CONTROL_L0_FIELD)) {
> -		fwts_warning(fw, "RP %02Xh:%02Xh.%02Xh L0s not enabled.",
> -			 rp->bus, rp->dev, rp->func);
> +		fwts_warning(fw, "RP %04Xh:%02Xh:%02Xh.%02Xh L0s not enabled.",
> +			 rp->segment, rp->bus, rp->dev, rp->func);
> 		l0s_disabled = true;
> 	}
> 
> 	if (((rp_cap->link_cap & FWTS_PCIE_ASPM_SUPPORT_L1_FIELD) >> 10) !=
> 		(rp_cap->link_contrl & FWTS_PCIE_ASPM_CONTROL_L1_FIELD)) {
> -		fwts_warning(fw, "RP %02Xh:%02Xh.%02Xh L1 not enabled.",
> -			rp->bus, rp->dev, rp->func);
> +		fwts_warning(fw, "RP %04Xh:%02Xh:%02Xh.%02Xh L1 not enabled.",
> +			rp->segment, rp->bus, rp->dev, rp->func);
> 		l1_disabled = true;
> 	}
> 
> 	if (((device_cap->link_cap & FWTS_PCIE_ASPM_SUPPORT_L0_FIELD) >> 10) !=
> 		(device_cap->link_contrl & FWTS_PCIE_ASPM_CONTROL_L0_FIELD)) {
> -		fwts_warning(fw, "Device %02Xh:%02Xh.%02Xh L0s not enabled.",
> -			dev->bus, dev->dev, dev->func);
> +		fwts_warning(fw, "Device %04Xh:%02Xh:%02Xh.%02Xh L0s not enabled.",
> +			dev->segment, dev->bus, dev->dev, dev->func);
> 		l0s_disabled = true;
> 	}
> 
> 	if (((device_cap->link_cap & FWTS_PCIE_ASPM_SUPPORT_L1_FIELD) >> 10) !=
> 		(device_cap->link_contrl & FWTS_PCIE_ASPM_CONTROL_L1_FIELD)) {
> -		fwts_warning(fw, "Device %02Xh:%02Xh.%02Xh L1 not enabled.",
> -			dev->bus, dev->dev, dev->func);
> +		fwts_warning(fw, "Device %04Xh:%02Xh:%02Xh.%02Xh L1 not enabled.",
> +			dev->segment, dev->bus, dev->dev, dev->func);
> 		l1_disabled = true;
> 	}
> 
> @@ -157,10 +157,10 @@ static int pcie_compare_rp_dev_aspm_registers(fwts_framework \
> *fw,  if (rp_aspm_cntrl != device_aspm_cntrl) {
> 		fwts_failed(fw, LOG_LEVEL_MEDIUM, "PCIEASPM_Unmatched",
> 			"PCIe ASPM setting was not matched.");
> -		fwts_log_error(fw, "RP %02Xh:%02Xh.%02Xh has ASPM = %02Xh."
> -			, rp->bus, rp->dev, rp->func, rp_aspm_cntrl);
> -		fwts_log_error(fw, "Device %02Xh:%02Xh.%02Xh has ASPM = %02Xh.",
> -			dev->bus, dev->dev, dev->func, device_aspm_cntrl);
> +		fwts_log_error(fw, "RP %04Xh:%02Xh:%02Xh.%02Xh has ASPM = %02Xh."
> +			, rp->segment, rp->bus, rp->dev, rp->func, rp_aspm_cntrl);
> +		fwts_log_error(fw, "Device %04Xh:%02Xh:%02Xh.%02Xh has ASPM = %02Xh.",
> +			dev->segment, dev->bus, dev->dev, dev->func, device_aspm_cntrl);
> 		fwts_advice(fw,
> 			"ASPM control registers between root port and device "
> 			"must match in order for ASPM to be active. Unmatched "
> @@ -189,11 +189,12 @@ static int pcie_check_aspm_registers(fwts_framework *fw)
> 	}
> 	while ((entry = readdir(dirp)) != NULL) {
> 		uint8_t bus, dev, func;
> +		uint16_t segment;
> 
> 		if (entry->d_name[0] == '.')
> 			continue;
> 
> -		if (sscanf(entry->d_name, "%*x:%" SCNx8 ":%" SCNx8 ".%" SCNx8, &bus, &dev, \
> &func) == 3) { +		if (sscanf(entry->d_name, "%" SCNx16 ":%" SCNx8 ":%" SCNx8 ".%" \
> SCNx8, &segment, &bus, &dev, &func) == 4) {  int fd;
> 			char path[PATH_MAX];
> 			struct pci_device *device;
> @@ -204,6 +205,7 @@ static int pcie_check_aspm_registers(fwts_framework *fw)
> 				closedir(dirp);
> 				return FWTS_ERROR;
> 			}
> +			device->segment = segment;
> 			device->bus = bus;
> 			device->dev = dev;
> 			device->func = func;
> @@ -235,9 +237,11 @@ static int pcie_check_aspm_registers(fwts_framework *fw)
> 		if (cur->config[FWTS_PCI_CONFIG_HEADER_TYPE] & 0x01) {
> 			for (ltarget = dev_list.head; ltarget; ltarget = ltarget->next) {
> 				struct pci_device *target = (struct pci_device *)ltarget->data;
> -				if (target->bus == cur->config[FWTS_PCI_CONFIG_TYPE1_SECONDARY_BUS_NUMBER]) {
> -					pcie_compare_rp_dev_aspm_registers(fw, cur, target);
> -					break;
> +				if (target->segment == cur->segment) {
> +					if (target->bus == cur->config[FWTS_PCI_CONFIG_TYPE1_SECONDARY_BUS_NUMBER]) {
> +						pcie_compare_rp_dev_aspm_registers(fw, cur, target);
> +						break;
> +					}
> 				}
> 			}
> 		}
> 

Acked-by: Alex Hung <alex.hung@canonical.com>

-- 
fwts-devel mailing list
fwts-devel@lists.ubuntu.com
Modify settings or unsubscribe at: \
https://lists.ubuntu.com/mailman/listinfo/fwts-devel


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