Re: [PATCH v2 2/7] platform/x86/intel/ifs: Introduce Array Scan test to IFS

[Joseph, Jithu]

On 2/16/2023 4:40 AM, Greg KH wrote:
> On Tue, Feb 14, 2023 at 03:44:21PM -0800, Jithu Joseph wrote:

> But note, that's a horrid name of an enumerated type that is in a .h
> file, either put it only in the .c file, or give it a name that makes
> more sense that it belongs only to this driver.
> 
> Yes, naming is hard.

Even for a driver local header, I agree that "enum ifs_test_type" would have been
more appropriate than "enum test_types

> 
> Wait, you don't even use the enumerated type anywhere in this patch
> series only the value, did you mean for this to happen?  Why name it
> anything?
> 

Since I am only using the values as you said, I will remove the type and
use #defines


>> +static struct ifs_device ifs_devices[] = {
>> +	[IFS_SAF] = {
>> +		.data = {
>> +			.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
>> +			.test_num = IFS_SAF,
>> +		},
>> +		.misc = {
>> +			.name = "intel_ifs_0",
>> +			.nodename = "intel_ifs/0",
> 
> I just noticed this, a device node called "0" is not good, why do you
> need this in a subdir at all?

There is no need for the device nodename in /dev to have a distinct ".nodename" from the sysfs "name"
I will remove the separate .nodename line so that /dev entries are created with the same name as
the sysfs directories.

> 
>> +			.minor = MISC_DYNAMIC_MINOR,
>> +		},
>>  	},
>> -	.misc = {
>> -		.name = "intel_ifs_0",
>> -		.nodename = "intel_ifs/0",
>> -		.minor = MISC_DYNAMIC_MINOR,
>> +	[IFS_ARRAY] = {
>> +		.data = {
>> +			.integrity_cap_bit = MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT,
>> +			.test_num = IFS_ARRAY,
>> +		},
>> +		.misc = {
>> +			.name = "intel_ifs_1",
>> +			.nodename = "intel_ifs/1",
> 
> Again, a device node called "1"?

Will remove this distinct "nodename" too


> 
>> +
>>  static int __init ifs_init(void)
>>  {
>>  	const struct x86_cpu_id *m;
>> +	int ndevices = 0;
>>  	u64 msrval;
>> -	int ret;
>> +	int i;
>>  
>>  	m = x86_match_cpu(ifs_cpu_ids);
>>  	if (!m)
>> @@ -51,28 +68,35 @@ static int __init ifs_init(void)
>>  	if (rdmsrl_safe(MSR_INTEGRITY_CAPS, &msrval))
>>  		return -ENODEV;
>>  
>> -	ifs_device.misc.groups = ifs_get_groups();
>> -
>> -	if (!(msrval & BIT(ifs_device.data.integrity_cap_bit)))
>> -		return -ENODEV;
>> +	for (i = 0; i < IFS_NUMTESTS; i++) {
>> +		if (!(msrval & BIT(ifs_devices[i].data.integrity_cap_bit)))
>> +			continue;
>>  
>> -	ifs_device.data.pkg_auth = kmalloc_array(topology_max_packages(), sizeof(bool), GFP_KERNEL);
>> -	if (!ifs_device.data.pkg_auth)
>> -		return -ENOMEM;
>> +		ifs_devices[i].data.pkg_auth = kmalloc_array(topology_max_packages(),
>> +							     sizeof(bool), GFP_KERNEL);
>> +		if (!ifs_devices[i].data.pkg_auth)
>> +			continue;
> 
> You have a static array of a structure that contains both things that
> describe the devices being used, as well as dynamic data with no real
> lifespan rules.  Please don't perputate this common design pattern
> mistake.
> 
> Always try to make static data constant and make dynamic data dynamic
> with proper reference counted lifetime rules.  People converting this
> code into rust in the future will thank you :)

I may not have fully understood your comment. So pardon me if the following description
on the lifecycle of the dynamic allocated memory is not to the point.

The lifetime of this allocation matches the load time of the driver (allocated on init, freed on exit).
There are no further / allocations or freeing anywhere within the driver.
There is only a single place where this memory is used, whose access is serialized via a semaphore.

Given the above, does moving this one and only allocation to use a global pointer instead of one embedded in the structure is what is needed. ?

Jithu