[RFC,3/4] acpi: only run ACPI tests if we have ACPI

We only want to run ACPI tests if the firmware has ACPI, so add
fwts_firmware_has_feature(FWTS_FW_FEATURE_ACPI) checks.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

---
 src/acpi/acpitables/acpitables.c   |   10 +++++++++-
 src/acpi/checksum/checksum.c       |   10 +++++++++-
 src/acpi/method/method.c           |    3 +++
 src/acpi/syntaxcheck/syntaxcheck.c |    3 +++
 src/pci/aspm/aspm.c                |   10 +++++++++-
 5 files changed, 33 insertions(+), 3 deletions(-)

On 24/04/14 09:18, Jeremy Kerr wrote:
> We only want to run ACPI tests if the firmware has ACPI, so add
> fwts_firmware_has_feature(FWTS_FW_FEATURE_ACPI) checks.
> 
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> 
> ---
>  src/acpi/acpitables/acpitables.c   |   10 +++++++++-
>  src/acpi/checksum/checksum.c       |   10 +++++++++-
>  src/acpi/method/method.c           |    3 +++
>  src/acpi/syntaxcheck/syntaxcheck.c |    3 +++
>  src/pci/aspm/aspm.c                |   10 +++++++++-
>  5 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/src/acpi/acpitables/acpitables.c b/src/acpi/acpitables/acpitables.c
> index 439df2a..057e1e9 100644
> --- a/src/acpi/acpitables/acpitables.c
> +++ b/src/acpi/acpitables/acpitables.c
> @@ -605,6 +605,13 @@ static int acpi_table_check_test2(fwts_framework *fw)
>  	return FWTS_OK;
>  }
>  
> +static int acpi_table_check_init(fwts_framework *fw __attribute__((unused)))
> +{
> +	if (!fwts_firmware_has_feature(FWTS_FW_FEATURE_ACPI))
> +		return FWTS_SKIP;
> +	return FWTS_OK;
> +}
> +
>  static fwts_framework_minor_test acpi_table_check_tests[] = {
>  	{ acpi_table_check_test1, "Test ACPI tables." },
>  	{ acpi_table_check_test2, "Test ACPI headers." },
> @@ -613,7 +620,8 @@ static fwts_framework_minor_test acpi_table_check_tests[] = {
>  
>  static fwts_framework_ops acpi_table_check_ops = {
>  	.description = "ACPI table settings sanity tests.",
> -	.minor_tests = acpi_table_check_tests
> +	.minor_tests = acpi_table_check_tests,
> +	.init = acpi_table_check_init,
>  };
>  
>  FWTS_REGISTER("acpitables", &acpi_table_check_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH);
> diff --git a/src/acpi/checksum/checksum.c b/src/acpi/checksum/checksum.c
> index bac266d..deb10ae 100644
> --- a/src/acpi/checksum/checksum.c
> +++ b/src/acpi/checksum/checksum.c
> @@ -145,6 +145,13 @@ static int checksum_test1(fwts_framework *fw)
>  	return checksum_scan_tables(fw);
>  }
>  
> +static int checksum_init(fwts_framework *w __attribute__((unused)))
> +{
> +	if (!fwts_firmware_has_feature(FWTS_FW_FEATURE_ACPI))
> +		return FWTS_SKIP;
> +	return FWTS_OK;
> +}
> +
>  static fwts_framework_minor_test checksum_tests[] = {
>  	{ checksum_test1, "ACPI table checksum test." },
>  	{ NULL, NULL }
> @@ -152,7 +159,8 @@ static fwts_framework_minor_test checksum_tests[] = {
>  
>  static fwts_framework_ops checksum_ops = {
>  	.description = "ACPI table checksum test.",
> -	.minor_tests = checksum_tests
> +	.minor_tests = checksum_tests,
> +	.init = checksum_init,
>  };
>  
>  FWTS_REGISTER("checksum", &checksum_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH);
> diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c
> index 9b789cf..b288ad9 100644
> --- a/src/acpi/method/method.c
> +++ b/src/acpi/method/method.c
> @@ -375,6 +375,9 @@ static int method_init(fwts_framework *fw)
>  
>  	fadt_mobile_platform = false;
>  
> +	if (!fwts_firmware_has_feature(FWTS_FW_FEATURE_ACPI))
> +		return FWTS_SKIP;
> +
>  	/* Some systems have multiple FADTs, sigh */
>  	for (i = 0; i < 256; i++) {
>  		int ret = fwts_acpi_find_table(fw, "FACP", i, &info);
> diff --git a/src/acpi/syntaxcheck/syntaxcheck.c b/src/acpi/syntaxcheck/syntaxcheck.c
> index 504e5c6..a6ce2f1 100644
> --- a/src/acpi/syntaxcheck/syntaxcheck.c
> +++ b/src/acpi/syntaxcheck/syntaxcheck.c
> @@ -224,6 +224,9 @@ static syntaxcheck_error_map_item syntaxcheck_error_map[] = {
>  
>  static int syntaxcheck_init(fwts_framework *fw)
>  {
> +	if (!fwts_firmware_has_feature(FWTS_FW_FEATURE_ACPI))
> +		return FWTS_SKIP;
> +
>  	(void)syntaxcheck_load_advice(fw);
>  
>  	return FWTS_OK;
> diff --git a/src/pci/aspm/aspm.c b/src/pci/aspm/aspm.c
> index e798c26..e4e689a 100644
> --- a/src/pci/aspm/aspm.c
> +++ b/src/pci/aspm/aspm.c
> @@ -264,6 +264,13 @@ static int aspm_pcie_register_configuration(fwts_framework *fw)
>  	return ret;
>  }
>  
> +static int aspm_init(fwts_framework *fw __attribute__((unused)))
> +{
> +	if (!fwts_firmware_has_feature(FWTS_FW_FEATURE_ACPI))
> +		return FWTS_SKIP;

My initial thought was this needs some feedback in the log that this
test was skipped because it lacked the required feature.. e.g.
		
	if (!fwts_firmware_has_feature(FWTS_FW_FEATURE_ACPI)) {
		fwts_log_info(fw, "Cannot test, firmware lacks ACPI.");
		return FWTS_SKIP;
	}

..and then it struck me that this needs to be put into every test.. so..

> +	return FWTS_OK;
> +}
> +
>  static fwts_framework_minor_test aspm_tests[] = {
>  	{ aspm_check_configuration, "PCIe ASPM ACPI test." },
>  	{ aspm_pcie_register_configuration, "PCIe ASPM registers test." },
> @@ -272,7 +279,8 @@ static fwts_framework_minor_test aspm_tests[] = {
>  
>  static fwts_framework_ops aspm_ops = {
>  	.description = "PCIe ASPM test.",
> -	.minor_tests = aspm_tests
> +	.minor_tests = aspm_tests,
> +	.init = aspm_init,

..my next thought was to add a .fw_feature flag in the ops field for
each test:

	.fw_feature = FWTS_FW_FEATURE_ACPI;

..and then we modify fwts_framework_run_test() to check if the per test
feature is supported before invoking the test, something like:
	
	if (!fwts_firmware_has_feature(test->ops->fw_feature)) {
		fwts_log_info(fw, "Firmware is missing feature blah blah, test skipped.");
		fw->total.skipped += test->ops->total_tests;
		goto done;
		
	}

and perhaps if fw_feature is undefined (zero by default) we should
assume the test won't be skipped. E.g. for tests like the klog test.	


>  };
>  
>  FWTS_REGISTER("aspm", &aspm_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH | FWTS_FLAG_ROOT_PRIV);
>

Hi Colin,

Thanks for the quick feedback!

> ..my next thought was to add a .fw_feature flag in the ops field for
> each test:
>
> 	.fw_feature = FWTS_FW_FEATURE_ACPI;

Yep, I was thinking this might be a better way to do it. My only 
suggestion would be to make this a mask of required features, and we 
require all of the features to be present to run the tests. eg:

   .fw_features = FWTS_FW_FEATURE_ACPI | FWTS_FW_FEATURE_MAGIC_BEANS,

this would require a:

   bool fwts_firmware_has_all_features(int features)
   {
   	int fw_features;

   	/* populate fw_features based on the running firmware */
	[...]

   	return (fw_features & features) == features;
   }

- which would do the right thing if .fw_features is zero.

I'll also do up a function to convert a feature enum / mask to a string, 
so we can log the missing features nicely.

However, is the ops struct the correct place to put this?

Cheers,


Jeremy

On 24/04/14 13:53, Jeremy Kerr wrote:
> Hi Colin,
> 
> Thanks for the quick feedback!
> 
>> ..my next thought was to add a .fw_feature flag in the ops field for
>> each test:
>>
>>     .fw_feature = FWTS_FW_FEATURE_ACPI;
> 
> Yep, I was thinking this might be a better way to do it. My only
> suggestion would be to make this a mask of required features, and we
> require all of the features to be present to run the tests. eg:
> 
>   .fw_features = FWTS_FW_FEATURE_ACPI | FWTS_FW_FEATURE_MAGIC_BEANS,
> 
> this would require a:
> 
>   bool fwts_firmware_has_all_features(int features)
>   {
>       int fw_features;
> 
>       /* populate fw_features based on the running firmware */
>     [...]
> 
>       return (fw_features & features) == features;
>   }
> 
> - which would do the right thing if .fw_features is zero.

yep, makes sense, I was thinking of the features being a bit mask.

> 
> I'll also do up a function to convert a feature enum / mask to a string,
> so we can log the missing features nicely.
> 
> However, is the ops struct the correct place to put this?

Well, the ops struct is easiest as we can leave out the .fw_features
flag if we are lazy and don't want to modify every test in fwts.

Perhaps a better idea though would be to add the bit mask
FWTS_REGISTER() macro and shove this into a field the
fwts_framework_test struct.  This requires:

1. modifying all the tests to add the new .fw_feature setting to the
FWTS_REGISTER macro
2. passing the fw_feature into fwts_framework_test_add() and setting
this in new_test

..so it's a lot more effort, but probably a better way forward.

Colin


> 
> Cheers,
> 
> 
> Jeremy