Message-ID: <9dde72aa-a28d-2b0e-7b90-6b1996dbf202@intel.com>
Date:   Mon, 13 Mar 2023 09:37:32 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v3 6/8] platform/x86/intel/ifs: Implement Array BIST test
To:     Hans de Goede <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: <20230214234426.344960-1-jithu.joseph@intel.com>
 <20230301015942.462799-1-jithu.joseph@intel.com>
 <20230301015942.462799-7-jithu.joseph@intel.com>
 <628e092c-793e-96ce-71f5-80392ad69569@redhat.com>
Content-Language: en-US
From:   "Joseph, Jithu" <jithu.joseph@intel.com>
In-Reply-To: <628e092c-793e-96ce-71f5-80392ad69569@redhat.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?RncxSWpMTnBmc3ZTMW5POEtHT1JXcFRkOXE2VDRQTlYyRGdka2kvdVFzVGJt?=
 =?utf-8?B?N0ljZFFTekJjVmhVOU5oRmdvcVUrNHdJZFRWa3NDRTF5VmtHbGs3THhBUVRj?=
 =?utf-8?B?MmUreWJBb005dWlFZ1FubFNLc3FUU0laMW1BVkU2VmRBRmV6UUpUamxBMU9r?=
 =?utf-8?B?a1RXSjdNOXluaWNHSUkyQzFwSk1ya1B4T0xuQkRLdmVlN0RIUFE5NUd2UlRu?=
 =?utf-8?B?czVQNWNqSTZscTdmUkJpQSswTytSSjBNREJpbzVqeFpiVkNoS0FYa3pvakU4?=
 =?utf-8?B?dlp6dlBaczdPaTc4TnhUWGYvZ1hPVWFpdWdRa2dyZzRQa3NZSWdOMUlZNldl?=
 =?utf-8?B?NHZYL2Uvd3pZVkFjdDBEZFpCcVk4djRnVDBnNHpnVFVkNUNLOTlGdkIyRDJP?=
 =?utf-8?B?OGc4dFVNVGN2RzNtRXdMYm9TdHBQN0FiMnFRZE5LazRqVFpqSUJWQ3NiMElN?=
 =?utf-8?B?UlJvMGJ5VEtMYWVPMU1nc2RUNWxlZlNSem4rSTdCUnJZR29pR1dyaEFxajgy?=
 =?utf-8?B?dXZVNlRwZzR0YzdTVnhlN3ZHTnFRbUZ3VlJOWUFIOUpadENRSzI1YTBGejBR?=
 =?utf-8?B?MFliRXV3NWRncm1nYWNBb0lxcEZoRGFJTlZaVGZyWTNlOHdTNWYvNlZNczFt?=
 =?utf-8?B?ODhnajF3QWpZNTBzajBIby93Z3ZhcUlOTk5VYXFnWitwU0ZLcTNGNkRENHRG?=
 =?utf-8?B?Sm9kZFhxelpSQ0ZyeXM5cHNmWXJaZ1RUV1RER3puOEw5VkR1VU5YbStvd29x?=
 =?utf-8?B?S3JBK2ZZVHlxUDdGd0N3Sk50UHczTmVkWlpUV3Y2QWdTQ2ZiZzl4b09seHlD?=
 =?utf-8?B?dnJoaU9UcFhiTDdqUnIyWUVEbGltV1MzK3pkUXJBL1orQ3JnQ0xZbEpLOHlj?=
 =?utf-8?B?RXNBeTJFdzY4dWpFenR6Y3lpSm1NdHMyOGUzbURKYk1TTHI2Vng2OG1EanNL?=
 =?utf-8?B?SlgxN0Rhb1E3L3M2eGNxRmJ5L0RWRmxXcXlQd1Q2TktHL2t3WGFkeTBsaC9K?=
 =?utf-8?B?ODd1QkZyZmI4TE1VNWRZcG1CWjBTR3I0Z3pLbmsvWUlYbngxUkFXbGkxVUpr?=
 =?utf-8?B?YW5NTnFybGpKV1FKRXJKRDZzSFpVemY3S25nMWN1YjhuNzNxTGdxSTV0Z1BZ?=
 =?utf-8?B?K1hOTTNRR1lycHhUOW9qZGs5YTM3bkVERG1mZXl4UVZSQWJpcnIzaDRXaUov?=
 =?utf-8?B?M2VxZ3liMDBVM0VIekRlN04vUUwxTU9wNkVHZngxM01VQTlVcy9wTlpOc0Fj?=
 =?utf-8?B?TmNydEZ4NzhtZlVUUlBHR1gzM0lCUGlrYlBzOERCOU1MK1hEZWYycmtNYmNP?=
 =?utf-8?B?a2wzUFhPNFFVUFZxOURUUUN5MUNxWDRkbnBYVmpaS0hlTGhQeVlCQXhkN1JM?=
 =?utf-8?B?U2NOQTRycmhON2xaQmNSUU5xR3JqandtUGw4Q1Nqd1B5WGdpbUV6RVNCUE5M?=
 =?utf-8?B?UEtETGVwQk9wV1V5KzZkOGVhZjZET2VEU1R4YzR4YWV1Wml5WXJzMy84NTZV?=
 =?utf-8?B?K0VwNENlVHNaVFlVY1dCdnA3NTRKQnlaQzZlaEt2dEZkTmdHRStaZVlSeWty?=
 =?utf-8?B?MHZCTXVieXZHb3l1TTRkdmZjVFFiYUg5NnpxdWF0a29UU3hoQ1E5L1hQTWgy?=
 =?utf-8?B?cmY2bFNPaUQ5ZU42QnhnQ29KeGdKQSt2RkNtRVVBZGFiS3Q1aVV6RmdSQmdJ?=
 =?utf-8?B?VHBSdklTcmpOcDI3dVNGZzVLdzlBVE5FNVN2ekZyK3dyNStES2lHbkhOTFpH?=
 =?utf-8?B?M1UvQzNkQ1U5V0txTDQ2ZjM3dG9VMjNhWlU5dnE2VkNsQXFJa3JqakpjZnBD?=
 =?utf-8?B?M0RXd3QzdnRLaEU0ZUZ5SSsrVjlrSEZaa21PMjNvWmt1N1NhUnZ4dksrYVl5?=
 =?utf-8?B?YWhlNVFzaS8wbTQ4WHQvbkRQaVFSUzN0Z01FNXV3UXpucm5aY3lNTTJYWDRZ?=
 =?utf-8?B?RkpkbXk1OEtqeWt2d3Y1YnpBNjZ4dUhYdXJZcm5KazZGSWp1SWxnblJyeDEr?=
 =?utf-8?B?ZlpDZDlDTkx5SzBDUjlHckdnS0ZqRVZqSDVyYTJyMWg2eHVHK05ESlBQeUZz?=
 =?utf-8?B?aW5XemhYQTJ3TjR1amFCa2ZSZi9TZjR4US9TdFUrZUJPOUVUMEt1UmN1Smx4?=
 =?utf-8?B?dyt1MWV3a1E2Y2JWZDgzZzk4WXdpd1pmM2Q5UHhFWWUyZ3ZWTE8zZlhaMFlH?=
 =?utf-8?B?Ync9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5a28d1d5-6ad9-4e6b-1662-08db23e13faf
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3768.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Mar 2023 16:37:34.7984
 (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: PytOXEbJREPViPqjPc19QRPlpLvPv1MRPOV3DNADbPXhmQAuljBadu5e7UrsnB2v7yVoaLJpaCrt9B4EH6uYCw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV2PR11MB5976
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hans,

Thank-you very much for the review.

On 3/13/2023 9:24 AM, Hans de Goede wrote:


>>  
>> +#define SPINUNIT 100 /* 100 nsec */
>> +static atomic_t array_cpus_out;
> 
> This variable is only inc-ed + read, it is never reset to 0
> so the "while (atomic_read(t) < all_cpus)"
> check only works for the first test run.
> 

It is reset to zero as annotated below. Let me know if this doesn't address your concern.

> Also even static atomic_t variables must be initialized, you cannot
> assume that using using zeroed mem is a valid value for an atomic_t.
> 
> And this is also shared between all smt pairs, so if 2 "real"
> CPU cores with both 2 sibblings are asked to run IFS tests at
> the same time, then array_cpus_out will get increased 4 times
> in total, breaking the wait_for_sibbling loop as soon as
> the counter reaches 2, so before the tests are done.

Only one IFS test is allowed at a time. This is done using "ifs_sem" defined in sysfs.c

...

>> +static void ifs_array_test_core(int cpu, struct device *dev)
>> +{
>> +	union ifs_array command = {};
>> +	bool timed_out = false;
>> +	struct ifs_data *ifsd;
>> +	unsigned long timeout;
>> +
>> +	ifsd = ifs_get_data(dev);
>> +
>> +	command.array_bitmask = ~0U;
>> +	timeout = jiffies + HZ / 2;
>> +
>> +	do {
>> +		if (time_after(jiffies, timeout)) {
>> +			timed_out = true;
>> +			break;
>> +		}
>> +		atomic_set(&array_cpus_out, 0);

The above line is where the zero initialization happens before every test.

>> +		stop_core_cpuslocked(cpu, do_array_test, &command);
>> +
>> +		if (command.ctrl_result)
>> +			break;
>> +	} while (command.array_bitmask);
>> +
>> +	ifsd->scan_details = command.data;
>> +
>> +	if (command.ctrl_result)
>> +		ifsd->status = SCAN_TEST_FAIL;
>> +	else if (timed_out || command.array_bitmask)
>> +		ifsd->status = SCAN_NOT_TESTED;
>> +	else
>> +		ifsd->status = SCAN_TEST_PASS;
>> +}

Jithu