diff mbox series

[3/5] platform/x86/intel/ifs: Sysfs interface for Array BIST

Message ID 20230131234302.3997223-4-jithu.joseph@intel.com (mailing list archive)
State Changes Requested, archived
Headers show
Series Add Array BIST test support to IFS | expand

Commit Message

Joseph, Jithu Jan. 31, 2023, 11:43 p.m. UTC 
The interface to trigger Array BIST test and obtain its result
is similar to the existing scan test. The only notable
difference is that, Array BIST doesn't require any test content
to be loaded. So binary load related options are not needed for
this test.

Add sysfs interface array BIST test, the testing support will
be added by subsequent patch.

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 drivers/platform/x86/intel/ifs/ifs.h     |  1 +
 drivers/platform/x86/intel/ifs/core.c    | 18 +++++++++++++-----
 drivers/platform/x86/intel/ifs/runtest.c | 11 ++++++++++-
 drivers/platform/x86/intel/ifs/sysfs.c   | 17 ++++++++++++++++-
 4 files changed, 40 insertions(+), 7 deletions(-)

Comments

Greg Kroah-Hartman Feb. 1, 2023, 5:04 a.m. UTC | #1 
On Tue, Jan 31, 2023 at 03:43:00PM -0800, Jithu Joseph wrote:
> The interface to trigger Array BIST test and obtain its result
> is similar to the existing scan test. The only notable
> difference is that, Array BIST doesn't require any test content
> to be loaded. So binary load related options are not needed for
> this test.
> 
> Add sysfs interface array BIST test, the testing support will
> be added by subsequent patch.

What is "sysfs interface array" exactly?

Where is the new Documentation/ABI/ entries for these new sysfs files
you added?

> 
> Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> ---
>  drivers/platform/x86/intel/ifs/ifs.h     |  1 +
>  drivers/platform/x86/intel/ifs/core.c    | 18 +++++++++++++-----
>  drivers/platform/x86/intel/ifs/runtest.c | 11 ++++++++++-
>  drivers/platform/x86/intel/ifs/sysfs.c   | 17 ++++++++++++++++-
>  4 files changed, 40 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index 2cef88a88aa9..07423bc4e368 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -249,5 +249,6 @@ static inline struct ifs_data *ifs_get_data(struct device *dev)
>  int ifs_load_firmware(struct device *dev);
>  int do_core_test(int cpu, struct device *dev);
>  const struct attribute_group **ifs_get_groups(void);
> +const struct attribute_group **ifs_get_array_groups(void);
>  
>  #endif
> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index ab234620ef4c..2b7a49fd473d 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -25,6 +25,7 @@ static struct ifs_device ifs_devices[] = {
>  	[IFS_SAF] = {
>  		.data = {
>  			.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
> +			.pkg_auth = NULL,
>  			.test_num = IFS_SAF,
>  		},
>  		.misc = {
> @@ -36,6 +37,7 @@ static struct ifs_device ifs_devices[] = {
>  	[IFS_ARRAY] = {
>  		.data = {
>  			.integrity_cap_bit = MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT,
> +			.pkg_auth = NULL,
>  			.test_num = IFS_ARRAY,
>  		},
>  		.misc = {
> @@ -72,11 +74,17 @@ static int __init ifs_init(void)
>  		if (!(msrval & BIT(ifs_devices[i].data.integrity_cap_bit)))
>  			continue;
>  
> -		ifs_devices[i].data.pkg_auth = kmalloc_array(topology_max_packages(),
> -							     sizeof(bool), GFP_KERNEL);
> -		if (!ifs_devices[i].data.pkg_auth)
> -			continue;
> -		ifs_devices[i].misc.groups = ifs_get_groups();
> +		switch (ifs_devices[i].data.test_num) {
> +		case IFS_SAF:
> +			ifs_devices[i].data.pkg_auth = kmalloc_array(topology_max_packages(),
> +								     sizeof(bool), GFP_KERNEL);
> +			if (!ifs_devices[i].data.pkg_auth)
> +				continue;
> +			ifs_devices[i].misc.groups = ifs_get_groups();
> +			break;
> +		case IFS_ARRAY:
> +			ifs_devices[i].misc.groups = ifs_get_array_groups();
> +		}
>  
>  		if (misc_register(&ifs_devices[i].misc))
>  			kfree(ifs_devices[i].data.pkg_auth);
> diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
> index 0bfd8fcdd7e8..65e08af70994 100644
> --- a/drivers/platform/x86/intel/ifs/runtest.c
> +++ b/drivers/platform/x86/intel/ifs/runtest.c
> @@ -236,6 +236,7 @@ static void ifs_test_core(int cpu, struct device *dev)
>   */
>  int do_core_test(int cpu, struct device *dev)
>  {
> +	struct ifs_data *ifsd = ifs_get_data(dev);
>  	int ret = 0;
>  
>  	/* Prevent CPUs from being taken offline during the scan test */
> @@ -247,7 +248,15 @@ int do_core_test(int cpu, struct device *dev)
>  		goto out;
>  	}
>  
> -	ifs_test_core(cpu, dev);
> +	switch (ifsd->test_num) {
> +	case IFS_SAF:
> +		ifs_test_core(cpu, dev);
> +		break;
> +	case IFS_ARRAY:
> +	default:
> +		return -EINVAL;
> +	}
> +
>  out:
>  	cpus_read_unlock();
>  	return ret;
> diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
> index ee636a76b083..7cf32184ce6a 100644
> --- a/drivers/platform/x86/intel/ifs/sysfs.c
> +++ b/drivers/platform/x86/intel/ifs/sysfs.c
> @@ -75,7 +75,7 @@ static ssize_t run_test_store(struct device *dev,
>  	if (down_interruptible(&ifs_sem))
>  		return -EINTR;
>  
> -	if (!ifsd->loaded)
> +	if (ifsd->test_num != IFS_ARRAY && !ifsd->loaded)
>  		rc = -EPERM;
>  	else
>  		rc = do_core_test(cpu, dev);
> @@ -156,3 +156,18 @@ const struct attribute_group **ifs_get_groups(void)
>  {
>  	return plat_ifs_groups;
>  }
> +
> +/* global array sysfs attributes */
> +static struct attribute *plat_ifs_array_attrs[] = {
> +	&dev_attr_details.attr,
> +	&dev_attr_status.attr,
> +	&dev_attr_run_test.attr,
> +	NULL
> +};
> +
> +ATTRIBUTE_GROUPS(plat_ifs_array);
> +
> +const struct attribute_group **ifs_get_array_groups(void)
> +{
> +	return plat_ifs_array_groups;
> +}

Why do you need a function to get access to a static variable?  Just
make the variable not static.

thanks,

greg k-h
Joseph, Jithu Feb. 1, 2023, 8:55 p.m. UTC | #2 
On 1/31/2023 9:04 PM, Greg KH wrote:
> On Tue, Jan 31, 2023 at 03:43:00PM -0800, Jithu Joseph wrote:
>> The interface to trigger Array BIST test and obtain its result
>> is similar to the existing scan test. The only notable
>> difference is that, Array BIST doesn't require any test content
>> to be loaded. So binary load related options are not needed for
>> this test.
>>
>> Add sysfs interface array BIST test, the testing support will
>> be added by subsequent patch.
> 
> What is "sysfs interface array" exactly?

Pardon the typo ... meant to write "sysfs interface for array BIST test ..."
sysfs entries for this new test will appear under "/sys/devices/virtual/misc/intel_ifs_1/"
(whereas the existing scan test entries will be under "/sys/devices/virtual/misc/intel_ifs_0/")

> 
> Where is the new Documentation/ABI/ entries for these new sysfs files
> you added?
> 

Current documentation and ABI entries are somewhat generalized (we mention intel_ifs_<n>). 
Pasting a small snippet below for reference from Documentation/ABI/testing/sysfs-platform-intel-ifs

<snip>
What:		/sys/devices/virtual/misc/intel_ifs_<N>/run_test
Date:		Nov 16 2022
KernelVersion:	6.2
Contact:	"Jithu Joseph" <jithu.joseph@intel.com>
Description:	Write <cpu#> to trigger IFS test for one online core.
		Note that the test is per core. The cpu# can be
                ...
</snip>

I can take another pass through both, to call out the differences wherever applicable
(like test image loading is not applicable for array BIST test)
...

>> @@ -156,3 +156,18 @@ const struct attribute_group **ifs_get_groups(void)
>>  {
>>  	return plat_ifs_groups;
>>  }
>> +
>> +/* global array sysfs attributes */
>> +static struct attribute *plat_ifs_array_attrs[] = {
>> +	&dev_attr_details.attr,
>> +	&dev_attr_status.attr,
>> +	&dev_attr_run_test.attr,
>> +	NULL
>> +};
>> +
>> +ATTRIBUTE_GROUPS(plat_ifs_array);
>> +
>> +const struct attribute_group **ifs_get_array_groups(void)
>> +{
>> +	return plat_ifs_array_groups;
>> +}
> 
> Why do you need a function to get access to a static variable?  Just
> make the variable not static.

The above was in-line with the existing driver style for the first test 
But if it is more appropriate, I can move "ATTRIBUTE_GROUPS(plat_ifs_array)" to core.c (where it is consumed) and drop the function

Jithu
diff mbox series

Patch

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 2cef88a88aa9..07423bc4e368 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -249,5 +249,6 @@  static inline struct ifs_data *ifs_get_data(struct device *dev)
 int ifs_load_firmware(struct device *dev);
 int do_core_test(int cpu, struct device *dev);
 const struct attribute_group **ifs_get_groups(void);
+const struct attribute_group **ifs_get_array_groups(void);
 
 #endif
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index ab234620ef4c..2b7a49fd473d 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -25,6 +25,7 @@  static struct ifs_device ifs_devices[] = {
 	[IFS_SAF] = {
 		.data = {
 			.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
+			.pkg_auth = NULL,
 			.test_num = IFS_SAF,
 		},
 		.misc = {
@@ -36,6 +37,7 @@  static struct ifs_device ifs_devices[] = {
 	[IFS_ARRAY] = {
 		.data = {
 			.integrity_cap_bit = MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT,
+			.pkg_auth = NULL,
 			.test_num = IFS_ARRAY,
 		},
 		.misc = {
@@ -72,11 +74,17 @@  static int __init ifs_init(void)
 		if (!(msrval & BIT(ifs_devices[i].data.integrity_cap_bit)))
 			continue;
 
-		ifs_devices[i].data.pkg_auth = kmalloc_array(topology_max_packages(),
-							     sizeof(bool), GFP_KERNEL);
-		if (!ifs_devices[i].data.pkg_auth)
-			continue;
-		ifs_devices[i].misc.groups = ifs_get_groups();
+		switch (ifs_devices[i].data.test_num) {
+		case IFS_SAF:
+			ifs_devices[i].data.pkg_auth = kmalloc_array(topology_max_packages(),
+								     sizeof(bool), GFP_KERNEL);
+			if (!ifs_devices[i].data.pkg_auth)
+				continue;
+			ifs_devices[i].misc.groups = ifs_get_groups();
+			break;
+		case IFS_ARRAY:
+			ifs_devices[i].misc.groups = ifs_get_array_groups();
+		}
 
 		if (misc_register(&ifs_devices[i].misc))
 			kfree(ifs_devices[i].data.pkg_auth);
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index 0bfd8fcdd7e8..65e08af70994 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -236,6 +236,7 @@  static void ifs_test_core(int cpu, struct device *dev)
  */
 int do_core_test(int cpu, struct device *dev)
 {
+	struct ifs_data *ifsd = ifs_get_data(dev);
 	int ret = 0;
 
 	/* Prevent CPUs from being taken offline during the scan test */
@@ -247,7 +248,15 @@  int do_core_test(int cpu, struct device *dev)
 		goto out;
 	}
 
-	ifs_test_core(cpu, dev);
+	switch (ifsd->test_num) {
+	case IFS_SAF:
+		ifs_test_core(cpu, dev);
+		break;
+	case IFS_ARRAY:
+	default:
+		return -EINVAL;
+	}
+
 out:
 	cpus_read_unlock();
 	return ret;
diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
index ee636a76b083..7cf32184ce6a 100644
--- a/drivers/platform/x86/intel/ifs/sysfs.c
+++ b/drivers/platform/x86/intel/ifs/sysfs.c
@@ -75,7 +75,7 @@  static ssize_t run_test_store(struct device *dev,
 	if (down_interruptible(&ifs_sem))
 		return -EINTR;
 
-	if (!ifsd->loaded)
+	if (ifsd->test_num != IFS_ARRAY && !ifsd->loaded)
 		rc = -EPERM;
 	else
 		rc = do_core_test(cpu, dev);
@@ -156,3 +156,18 @@  const struct attribute_group **ifs_get_groups(void)
 {
 	return plat_ifs_groups;
 }
+
+/* global array sysfs attributes */
+static struct attribute *plat_ifs_array_attrs[] = {
+	&dev_attr_details.attr,
+	&dev_attr_status.attr,
+	&dev_attr_run_test.attr,
+	NULL
+};
+
+ATTRIBUTE_GROUPS(plat_ifs_array);
+
+const struct attribute_group **ifs_get_array_groups(void)
+{
+	return plat_ifs_array_groups;
+}