Re: [PATCH 4/5] platform/x86/intel/ifs: Implement Array BIST test

[Joseph, Jithu]

On 2/1/2023 11:45 AM, Dave Hansen wrote:
> On 1/31/23 15:43, Jithu Joseph wrote:
>> +static void ifs_array_test_core(int cpu, struct device *dev)
>> +{
>> +	union ifs_array activate, status;
>> +	bool timed_out = false;
>> +	struct ifs_data *ifsd;
>> +	unsigned long timeout;
>> +	u64 msrvals[2];
>> +
>> +	ifsd = ifs_get_data(dev);
>> +
>> +	activate.data = 0;
>> +	activate.array_bitmask = ~0U;
>> +	activate.ctrl_result = 0;
> 
> I think this whole 'ifs_array' as a union thing is bogus.  It's actually
> obfuscating and *COMPLICATING* the code more than anything.  Look what
> you have:
> 
> 	union ifs_array activate; // declare it on the stack, unzeroed
> 
> 	activate.data = 0; // zero the structure;
> 	activate.array_bitmask = ~0U; // set one field
> 	activate.ctrl_result = 0; // set the field to zero again???
> 
> Can we make it less obfuscated?  How about:
> 
> 	struct ifs_array activate = {}; // zero it
> 	...
> 	activate.array_bitmask = ~0U; // set the only nonzero field
> 
> Voila!  Less code, less obfuscation, less duplicated effort.  Or, worst

Agreed, will modify it as you suggest above to remove the duplicate zero assignments

> case:
> 
> 	struct ifs_array activate;
> 	...
> 	memset(&activate, 0, sizeof(activate));
> 	activate.array_bitmask = ~0U;
> 
> That's sane and everyone knows what it does and doesn't have to know
> what unions are involved or how they are used.  It's correct code no
> matter *WHAT* craziness lies within 'activate'.