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

[Joseph, Jithu]

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@xxxxxxxxx>
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