Message-ID: <c04c1aee-b84c-324c-e7db-c004e778ef4e@intel.com>
Date:   Wed, 1 Feb 2023 11:56:51 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; 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:     Dave Hansen <dave.hansen@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>
 <6a675f9b-2bbb-9317-2015-0b3362e49fdc@intel.com>
From:   "Joseph, Jithu" <jithu.joseph@intel.com>
In-Reply-To: <6a675f9b-2bbb-9317-2015-0b3362e49fdc@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QnBncmJySFlPck1hTTNDRm1aUWVkQ3kraVgzc3JsSENQMWNWU0lEcFhDMk51?=
 =?utf-8?B?cXdqMm0rNmhpdUZ6WGl6dkZ4MU5EV3lwelpqT3paSUpWcU4xbWFsMEdCRk5i?=
 =?utf-8?B?YWlMZjNNa1FsZlIwNWdNQ24zUGU1WFVUNGVmUklDRUE4OWJVT0dzSEFLUGIv?=
 =?utf-8?B?NnhVbVlMeUFyV0xvcnJOM2VYcXcydkZOWkV4SVJVNUtQVmlrelphZ2hyUkxr?=
 =?utf-8?B?emFLanV3dUp5N2tvZC94UDFKbkgvdTU1cFZSVC9IOUJaZUFEQW5uVVNtZG4r?=
 =?utf-8?B?SDJQVTlzRkE0TG1yL3pYN05Bc1A4MGJzQXo5VWZaY0hmRzQrc09UKzJUM1BW?=
 =?utf-8?B?cFBkalRyRTRiWG9MaFhQQ1Y4SEJ1Ulc1WVhlVUlzQ1BWcEtPMUtrOUp6elVm?=
 =?utf-8?B?azFPN3J2UThIek4vU1dQUW9XTHNMYXBDN1RxRmZlUGlHRWZlQTlObHJLQVp2?=
 =?utf-8?B?MjN3UGphendCSndVVE95QmF5ZXdlcXdMaDhScGpCbXZNNUJ1NFJOR2MyK2cv?=
 =?utf-8?B?VEV6NHJQb0puWm12T1lMMHhpMTR5OHk4WlIxS09NZGdxZDYvZllUdFc3Q1V4?=
 =?utf-8?B?WGtONlpLZWQwbE5ocDgwVWs2ZHY3b2x4YUFZY3ZxMWJhbEdxVUJhMExET1hM?=
 =?utf-8?B?SFBrYzhJNVQzRmtTbXBtRXR1TnpNVGVtbDhyYkJqQ2FLSHUvdjMyMXAwMWNv?=
 =?utf-8?B?YTF5UjZMR1B6LzRUWkszejhoOFY5QUZvMVpKUDZpbFRnZjlSVVZkUSt2Unpw?=
 =?utf-8?B?OEFCS205VkEyQ0E0eXMxVlJYMVY0N2J4R1hmTW5wNHFkRktGQ3RlYUx2Qkxy?=
 =?utf-8?B?SThsNnRTL3NkTU8wa1pSWk9rZFZ1VEFKUUptdEtqaG1pV0NDekJwRmVuMmU0?=
 =?utf-8?B?SWR1c21oWFdMcHVEUDhuVlRzRWpUWTFvK2pjQmE2VVVRNEJUQlNGSXBtNksw?=
 =?utf-8?B?VFJvZHJhbWFDRHhqYnhiVXZuQVd0QWZ5OHFqS1dIaTFyUjBESGRvNVhibGt0?=
 =?utf-8?B?eTB0ZWcxd0k4RTc5VG1KMHhIUFdGaFBadEJES25Nb2JzNjdrSGpaNmI2eVZu?=
 =?utf-8?B?YVY1dWJJQXc2by9EYkZ1KzNxWkNMbmJDeDdROWZlRFRHN3MyVzlFRVc3MVBL?=
 =?utf-8?B?cTVaYnl3K0hsNis3K2t0QjZYcGR0OHNSYzJYK09lRHdTWFhlYXpISjU1QUhW?=
 =?utf-8?B?bXEvVllMWnczVWlwSFBPaFo0TkU0RkI5V3QyaS9nZDJLdjFXejlkcmRKdERG?=
 =?utf-8?B?REk2Yi95c3dXbjNNNjdSelIwcWNvVDNYU2lmRmY2RjVHUlpSZ3ZONEJoa2Vr?=
 =?utf-8?B?eWlDMCtqeWV2M0pPZGhTSlNZMFJ6OG9VMnpFS0VVUkFRc3RzVnlhcGsxRmdS?=
 =?utf-8?B?L0xSQXBqK1A1dHdhT25VRTlWU2t6NUtCSFIrUENSRTJOTjM2Zzg4L2N3R1Z3?=
 =?utf-8?B?TFc5aGEvQXp3QnRRVXIwUmsydDVSS1RjZ2xLOEVzelFkY1UzU00xR095VlFL?=
 =?utf-8?B?SVRuL2JGS0tHOW55YWtxcjdIYXUzZWNTNEhubUVjY1VIclNDeWdRWEo1K3Fs?=
 =?utf-8?B?RURuNU56U2kxcE85ZHljQ2tVZDIvWTFtUGdSSklhWnRtOUpocnZjTDVZMkxZ?=
 =?utf-8?B?QTJhd25ISjltL2Y3dUNpQy9WVDdxdlB6dGRtWVpGSTlBaGRqampTZTN5Vi93?=
 =?utf-8?B?Q3dxWUQzODdRTnAzTXIyZkk2UHVSMzkvQmdkbyt4WHlHZ2NtM1NvdTZQYVlt?=
 =?utf-8?B?SE10TjdVUkdnVzJLeHdVN1JwQUI0WC9LellzWnBHUEV6Qk55NEZON2Y3dExa?=
 =?utf-8?B?a1QxY0ZGVHVSeWt6TnhobHI2RTFrQXFjK0VJWkJMeWlvOTdsTWQwYjI0cElK?=
 =?utf-8?B?M3h2bitSZW9IOE5rL1dMUzVqUUpyalNOaU5vNHkrM2ltRUs4aGEwM25QTkVk?=
 =?utf-8?B?czViaVdQZElUTjFtc3pCSThkUlJqTXNDdmVOdEFQNWwyRytLMUFVVTVTV2dK?=
 =?utf-8?B?N1hBU0t4OVJSNUFhdjdib0hCY1dCRGZqTlpENHNoRmNyZkhkQzhkTTEvaE4y?=
 =?utf-8?B?eDI4WGpRV2oySnRjVmowNWJCdlVCSHRNRkZyWGNVVlU2ZkJUemF6M3lWWWlj?=
 =?utf-8?B?elJJN092bHVGcDlqYmVGemk0VTVMY2xieWR4cTdqUXNNUzNweUZaNlRLTHNR?=
 =?utf-8?B?eGc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: fa610188-2b27-44ce-d9a0-08db048e7771
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3768.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Feb 2023 19:56:54.1686
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: cDivw8zhSP4ZRj001sux9izYmTC4L5fII2m0sapgv5/j2gYp3YZlnEpeJhMb5Lh46a0w4A5yiUQD69RyWxPX3g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB6375
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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