Message-ID: <6a675f9b-2bbb-9317-2015-0b3362e49fdc@intel.com>
Date:   Wed, 1 Feb 2023 11:45:44 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH 4/5] platform/x86/intel/ifs: Implement Array BIST test
Content-Language: en-US
To:     Jithu Joseph <jithu.joseph@intel.com>, hdegoede@redhat.com,
        markgross@kernel.org
Cc:     tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com,
        gregkh@linuxfoundation.org, rostedt@goodmis.org,
        ashok.raj@intel.com, tony.luck@intel.com,
        linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org,
        patches@lists.linux.dev, ravi.v.shankar@intel.com,
        thiago.macieira@intel.com, athenas.jimenez.gonzalez@intel.com,
        sohil.mehta@intel.com
References: <20230131234302.3997223-1-jithu.joseph@intel.com>
 <20230131234302.3997223-5-jithu.joseph@intel.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <20230131234302.3997223-5-jithu.joseph@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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