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

[Dave Hansen]

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
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'.