Patchwork bios: mtrr: make prefetchable checks handle lspci exec failures

login
register
mail settings
Submitter Colin King
Date Dec. 7, 2012, 2:25 p.m.
Message ID <1354890330-31847-1-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/204499/
State Accepted
Headers show

Comments

Colin King - Dec. 7, 2012, 2:25 p.m.
From: Colin Ian King <colin.king@canonical.com>

The is_prefetchable() check just ignored lspci exec failures which
can lead to false results.  Handle this correctly by passing back
any failures up the chain back to the the test and skip the test if
we detect lspci failing.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/bios/mtrr/mtrr.c | 50 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 12 deletions(-)
Alex Hung - Dec. 13, 2012, 3:12 a.m.
On 12/07/2012 10:25 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The is_prefetchable() check just ignored lspci exec failures which
> can lead to false results.  Handle this correctly by passing back
> any failures up the chain back to the the test and skip the test if
> we detect lspci failing.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/bios/mtrr/mtrr.c | 50 ++++++++++++++++++++++++++++++++++++++------------
>   1 file changed, 38 insertions(+), 12 deletions(-)
>
> diff --git a/src/bios/mtrr/mtrr.c b/src/bios/mtrr/mtrr.c
> index 62a91c7..285d284 100644
> --- a/src/bios/mtrr/mtrr.c
> +++ b/src/bios/mtrr/mtrr.c
> @@ -193,38 +193,51 @@ restart:
>   	return type;
>   }
>
> -static bool is_prefetchable(fwts_framework *fw, char *device, uint64_t address)
> +static int check_prefetchable(
> +	fwts_framework *fw,
> +	char *device,
> +	uint64_t address,
> +	bool *pref)
>   {
> -	bool pref = false;
>   	char line[4096];
>   	fwts_list *lspci_output;
>   	fwts_list_link *item;
>   	int status;
>
> +	*pref = false;
> +
>   	memset(line,0,4096);
>
>   	snprintf(line, sizeof(line), "%s -v -s %s", fw->lspci, device);
> -	fwts_pipe_exec(line, &lspci_output, &status);
> +	if (fwts_pipe_exec(line, &lspci_output, &status) != FWTS_OK)
> +		return FWTS_ERROR;
> +
> +	/* Nothing for this decice? That seems like a failure */
>   	if (lspci_output == NULL)
> -		return pref;
> +		return FWTS_ERROR;
>
>   	fwts_list_foreach(item, lspci_output) {
>   		char *str = strstr(fwts_text_list_text(item), "Memory at ");
>   		if (str && strtoull(str+10, NULL, 16) == address) {
>   			if (strstr(str, "non-prefetchable"))
> -				pref = false;
> +				*pref = false;
>   			else if (strstr(str, "(prefetchable"))
> -				pref = true;
> +				*pref = true;
>   			else if (strstr(str, ", prefetchable"))
> -				pref = true;
> +				*pref = true;
>   		}
>   	}
>   	fwts_list_free(lspci_output, free);
>
> -	return pref;
> +	return FWTS_OK;
>   }
>
> -static void guess_cache_type(fwts_framework *fw, char *string, int *must, int *mustnot, uint64_t address)
> +static int guess_cache_type(
> +	fwts_framework *fw,
> +	char *string,
> +	int *must,
> +	int *mustnot,
> +	uint64_t address)
>   {
>   	*must = 0;
>   	*mustnot = 0;
> @@ -232,11 +245,16 @@ static void guess_cache_type(fwts_framework *fw, char *string, int *must, int *m
>   	if (strstr(string, "System RAM")) {
>   		*must = WRITE_BACK;
>   		*mustnot = ~WRITE_BACK;
> -		return;
> +		return FWTS_OK;
>   	}
>   	/* if it's PCI mmio -> uncached typically except for video */
>   	if (strstr(string, "0000:")) {
> -		if (is_prefetchable(fw, string, address)) {
> +		bool pref;
> +
> +		if (check_prefetchable(fw, string, address, &pref) != FWTS_OK)
> +			return FWTS_ERROR;
> +
> +		if (pref) {
>   			*must = 0;
>   			*mustnot = WRITE_BACK | UNCACHED;
>   		} else {
> @@ -244,6 +262,8 @@ static void guess_cache_type(fwts_framework *fw, char *string, int *must, int *m
>   			*mustnot = (~UNCACHED) & (~DEFAULT);
>   		}
>   	}
> +
> +	return FWTS_OK;
>   }
>
>   static int validate_iomem(fwts_framework *fw)
> @@ -310,7 +330,13 @@ static int validate_iomem(fwts_framework *fw)
>
>   		type = cache_types(start, end);
>
> -		guess_cache_type(fw, c2, &type_must, &type_mustnot, start);
> +		if (guess_cache_type(fw, c2, &type_must, &type_mustnot, start) != FWTS_OK) {
> +			/*  This has failed, give up at this point */
> +			fwts_skipped(fw,
> +				"Could not guess cache type, lspci seems to be failing.");
> +			fclose(file);
> +			return FWTS_ERROR;
> +		}
>
>   		if ((type & type_mustnot)!=0) {
>   			failed++;
>
Acked-by: Alex Hung <alex.hung@canonical.com>
Ivan Hu - Dec. 14, 2012, 12:06 a.m.
On 12/07/2012 10:25 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The is_prefetchable() check just ignored lspci exec failures which
> can lead to false results.  Handle this correctly by passing back
> any failures up the chain back to the the test and skip the test if
> we detect lspci failing.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/bios/mtrr/mtrr.c | 50 ++++++++++++++++++++++++++++++++++++++------------
>   1 file changed, 38 insertions(+), 12 deletions(-)
>
> diff --git a/src/bios/mtrr/mtrr.c b/src/bios/mtrr/mtrr.c
> index 62a91c7..285d284 100644
> --- a/src/bios/mtrr/mtrr.c
> +++ b/src/bios/mtrr/mtrr.c
> @@ -193,38 +193,51 @@ restart:
>   	return type;
>   }
>
> -static bool is_prefetchable(fwts_framework *fw, char *device, uint64_t address)
> +static int check_prefetchable(
> +	fwts_framework *fw,
> +	char *device,
> +	uint64_t address,
> +	bool *pref)
>   {
> -	bool pref = false;
>   	char line[4096];
>   	fwts_list *lspci_output;
>   	fwts_list_link *item;
>   	int status;
>
> +	*pref = false;
> +
>   	memset(line,0,4096);
>
>   	snprintf(line, sizeof(line), "%s -v -s %s", fw->lspci, device);
> -	fwts_pipe_exec(line, &lspci_output, &status);
> +	if (fwts_pipe_exec(line, &lspci_output, &status) != FWTS_OK)
> +		return FWTS_ERROR;
> +
> +	/* Nothing for this decice? That seems like a failure */
>   	if (lspci_output == NULL)
> -		return pref;
> +		return FWTS_ERROR;
>
>   	fwts_list_foreach(item, lspci_output) {
>   		char *str = strstr(fwts_text_list_text(item), "Memory at ");
>   		if (str && strtoull(str+10, NULL, 16) == address) {
>   			if (strstr(str, "non-prefetchable"))
> -				pref = false;
> +				*pref = false;
>   			else if (strstr(str, "(prefetchable"))
> -				pref = true;
> +				*pref = true;
>   			else if (strstr(str, ", prefetchable"))
> -				pref = true;
> +				*pref = true;
>   		}
>   	}
>   	fwts_list_free(lspci_output, free);
>
> -	return pref;
> +	return FWTS_OK;
>   }
>
> -static void guess_cache_type(fwts_framework *fw, char *string, int *must, int *mustnot, uint64_t address)
> +static int guess_cache_type(
> +	fwts_framework *fw,
> +	char *string,
> +	int *must,
> +	int *mustnot,
> +	uint64_t address)
>   {
>   	*must = 0;
>   	*mustnot = 0;
> @@ -232,11 +245,16 @@ static void guess_cache_type(fwts_framework *fw, char *string, int *must, int *m
>   	if (strstr(string, "System RAM")) {
>   		*must = WRITE_BACK;
>   		*mustnot = ~WRITE_BACK;
> -		return;
> +		return FWTS_OK;
>   	}
>   	/* if it's PCI mmio -> uncached typically except for video */
>   	if (strstr(string, "0000:")) {
> -		if (is_prefetchable(fw, string, address)) {
> +		bool pref;
> +
> +		if (check_prefetchable(fw, string, address, &pref) != FWTS_OK)
> +			return FWTS_ERROR;
> +
> +		if (pref) {
>   			*must = 0;
>   			*mustnot = WRITE_BACK | UNCACHED;
>   		} else {
> @@ -244,6 +262,8 @@ static void guess_cache_type(fwts_framework *fw, char *string, int *must, int *m
>   			*mustnot = (~UNCACHED) & (~DEFAULT);
>   		}
>   	}
> +
> +	return FWTS_OK;
>   }
>
>   static int validate_iomem(fwts_framework *fw)
> @@ -310,7 +330,13 @@ static int validate_iomem(fwts_framework *fw)
>
>   		type = cache_types(start, end);
>
> -		guess_cache_type(fw, c2, &type_must, &type_mustnot, start);
> +		if (guess_cache_type(fw, c2, &type_must, &type_mustnot, start) != FWTS_OK) {
> +			/*  This has failed, give up at this point */
> +			fwts_skipped(fw,
> +				"Could not guess cache type, lspci seems to be failing.");
> +			fclose(file);
> +			return FWTS_ERROR;
> +		}
>
>   		if ((type & type_mustnot)!=0) {
>   			failed++;
>
Acked-by: Ivan Hu <ivan.hu@canonical.com>

Patch

diff --git a/src/bios/mtrr/mtrr.c b/src/bios/mtrr/mtrr.c
index 62a91c7..285d284 100644
--- a/src/bios/mtrr/mtrr.c
+++ b/src/bios/mtrr/mtrr.c
@@ -193,38 +193,51 @@  restart:
 	return type;
 }
 
-static bool is_prefetchable(fwts_framework *fw, char *device, uint64_t address)
+static int check_prefetchable(
+	fwts_framework *fw,
+	char *device,
+	uint64_t address,
+	bool *pref)
 {
-	bool pref = false;
 	char line[4096];
 	fwts_list *lspci_output;
 	fwts_list_link *item;
 	int status;
 
+	*pref = false;
+
 	memset(line,0,4096);
 
 	snprintf(line, sizeof(line), "%s -v -s %s", fw->lspci, device);
-	fwts_pipe_exec(line, &lspci_output, &status);
+	if (fwts_pipe_exec(line, &lspci_output, &status) != FWTS_OK)
+		return FWTS_ERROR;
+
+	/* Nothing for this decice? That seems like a failure */
 	if (lspci_output == NULL)
-		return pref;
+		return FWTS_ERROR;
 
 	fwts_list_foreach(item, lspci_output) {
 		char *str = strstr(fwts_text_list_text(item), "Memory at ");
 		if (str && strtoull(str+10, NULL, 16) == address) {
 			if (strstr(str, "non-prefetchable"))
-				pref = false;
+				*pref = false;
 			else if (strstr(str, "(prefetchable"))
-				pref = true;
+				*pref = true;
 			else if (strstr(str, ", prefetchable"))
-				pref = true;
+				*pref = true;
 		}
 	}
 	fwts_list_free(lspci_output, free);
 
-	return pref;
+	return FWTS_OK;
 }
 
-static void guess_cache_type(fwts_framework *fw, char *string, int *must, int *mustnot, uint64_t address)
+static int guess_cache_type(
+	fwts_framework *fw,
+	char *string,
+	int *must,
+	int *mustnot,
+	uint64_t address)
 {
 	*must = 0;
 	*mustnot = 0;
@@ -232,11 +245,16 @@  static void guess_cache_type(fwts_framework *fw, char *string, int *must, int *m
 	if (strstr(string, "System RAM")) {
 		*must = WRITE_BACK;
 		*mustnot = ~WRITE_BACK;
-		return;
+		return FWTS_OK;
 	}
 	/* if it's PCI mmio -> uncached typically except for video */
 	if (strstr(string, "0000:")) {
-		if (is_prefetchable(fw, string, address)) {
+		bool pref;
+
+		if (check_prefetchable(fw, string, address, &pref) != FWTS_OK)
+			return FWTS_ERROR;
+
+		if (pref) {
 			*must = 0;
 			*mustnot = WRITE_BACK | UNCACHED;
 		} else {
@@ -244,6 +262,8 @@  static void guess_cache_type(fwts_framework *fw, char *string, int *must, int *m
 			*mustnot = (~UNCACHED) & (~DEFAULT);
 		}
 	}
+
+	return FWTS_OK;
 }
 
 static int validate_iomem(fwts_framework *fw)
@@ -310,7 +330,13 @@  static int validate_iomem(fwts_framework *fw)
 
 		type = cache_types(start, end);
 
-		guess_cache_type(fw, c2, &type_must, &type_mustnot, start);
+		if (guess_cache_type(fw, c2, &type_must, &type_mustnot, start) != FWTS_OK) {
+			/*  This has failed, give up at this point */
+			fwts_skipped(fw, 
+				"Could not guess cache type, lspci seems to be failing.");
+			fclose(file);
+			return FWTS_ERROR;
+		}
 
 		if ((type & type_mustnot)!=0) {
 			failed++;