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