Message-ID: <5df82b97-c317-6dbf-d068-29e39ccdf3bc@intel.com>
Date:   Wed, 1 Feb 2023 12:55:42 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH 3/5] platform/x86/intel/ifs: Sysfs interface for Array
 BIST
Content-Language: en-US
To:     Greg KH <gregkh@linuxfoundation.org>
CC:     <hdegoede@redhat.com>, <markgross@kernel.org>,
        <tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>,
        <dave.hansen@linux.intel.com>, <x86@kernel.org>, <hpa@zytor.com>,
        <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-4-jithu.joseph@intel.com>
 <Y9nyxNesVHCUXAcH@kroah.com>
From:   "Joseph, Jithu" <jithu.joseph@intel.com>
In-Reply-To: <Y9nyxNesVHCUXAcH@kroah.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?MVB1YjZUeUtZaGwyY0JNRkpvdkE1STB2bG9GN2lLU3RXeC94cmNZajIrbDUr?=
 =?utf-8?B?SWlWNlhCVkVkVHN0VktrN1Uwa0Rsa3pZVzZTQWU4dmY2VWNUWWNRelcxOGJ5?=
 =?utf-8?B?dEloTS9JeGxjNVVtaUdKTFZuR2l1RUxPemtsRTM3YkxBWHlOT1ltaGtoWEhz?=
 =?utf-8?B?RGdiaDBQVkRRK0VOUG93aU9OTjNNTEQ2MlFUZXU0bmt1UVJ6d3RuV0ozdFI0?=
 =?utf-8?B?WUcrb3kxNkhkM3ZUTlRMbkFZajFhdmdWRy9oR2tEK01QUzU0N2ltc2gvbFIv?=
 =?utf-8?B?SmtvL2VkSTREUDJFRnFFdG5Sci84dGtqWTZTWjNHR0t2YUpOQjRQbWR5V0JY?=
 =?utf-8?B?eTZQR3cyZkFnZmFTanlTZnZSc3JobE5jai9VMHBENUNEemI1REJad1A5U1dZ?=
 =?utf-8?B?OW5MaGlqZTU5Z3F5TWRLL2F4K2V5b3FXemJHS2ZJYmUzM3Z4a1JZd08wZXFa?=
 =?utf-8?B?OU1FSFVMWDlIVkFWWkN0Vm43WC9MUHBIbEZmVFhKdk9SYW1qdWk4TEVLTmpS?=
 =?utf-8?B?ZC9TYWt6YVI0ZGE3VWM4RGl3b3FGOUZ3YjltaGltS21CRGxpU3VjenpXdG44?=
 =?utf-8?B?S3VmKzEvbU50Qks5VWhraVJRTDBFaS8rNzJVSG5hU0xmUUVhRXdZRmxjdG5S?=
 =?utf-8?B?L243cU1rTHovZDlNVjV2eXdqclJQcVZ6YWZ2U24wVGluZmpYZFZ4TU8zU1pa?=
 =?utf-8?B?R0pwSVhMQnZ5L3dtL1VZY2kwQXI3YWtqcjVPQ3F5MUttN0poQW94Y2Z3SG5W?=
 =?utf-8?B?V2Y1RDNkeUJaK0xRS3ptaGNIQ3hZeFlIekJQRjdSV2xleEJKTnJGb0F5eTBM?=
 =?utf-8?B?aFZXa1dGYXFod3p3OUNlQndpOFZCdGZTRmUwaVFSR3BBTXRjV2VIbHJxMXNs?=
 =?utf-8?B?T1VHdHhCTHZlUGN1Um10ejlSZU9QcVFvSU9KZ3NGOVo3bWZPalRnSVJ6Njk4?=
 =?utf-8?B?LzEwR2dhSjlKTmV5Q05PdjlLZG80SlVKSEZaK2pZdm1UUU1DRE1mV1IrSlBU?=
 =?utf-8?B?L3hnUnZaZlBSNm9CZXExYkVyTDUrbjBrcFRjeS85RW1UMlRqU2FSMU9LckUv?=
 =?utf-8?B?Sy9BUXJuZlo2MU42VUkvTFhPK2dkL0Nwanp1K2tnREZ1ZzltMjU2eEJROFg0?=
 =?utf-8?B?RUUyVGZ4aWhMbjZQVVNvN0c3bTVKQWdQeE9Gd0dJNmZjY1RvRGRGRm84U21M?=
 =?utf-8?B?OVQ2S25nRFFjTWRCY2ZWTnN2MW1QekRib1k4K3QvMitDS3BUbjRvaU9pc1VP?=
 =?utf-8?B?SVZLSXduSkFKWUJBWERDMlovN1BLblh2ekllQmJiajFQSjZUa3E2c2NkSU5T?=
 =?utf-8?B?blREMlFCVkZ4L0IxOFJrMFVMK09IamFHcUdiNytkU2J4bUIwTmU4RDBQMEwz?=
 =?utf-8?B?VHhHdmRrTlZzQStubmI4dU0wUkcwRCsreHR5a0VPWWN2eFJaeHJnSDdNT2dk?=
 =?utf-8?B?dDBxZVJIWUplV09lYjBrRUx1OUVjUWI1YVZ1cDJrazNEQzVZTnNPWFhRL3RF?=
 =?utf-8?B?c2c1RU85UTVxUVJWSlRMUlpzRjZYZVFpU2k5aEM2M01YOFF2ZjhGYXE0dXRs?=
 =?utf-8?B?S3UyLzFJRVhEZ1lLa0xKc3ZjT3V3cFd4UnZhUEdHN3NuM0w2blQzUk1lWXYr?=
 =?utf-8?B?dVFhTlNGdTlNNmpJN0FkbWw0Um5FVFhsQjJsZG1HQ05RaERhNC9abnR5UmZi?=
 =?utf-8?B?VzB5ZGd1TGFGMWxPdm9xUTliYmdVVkh2UmRnVkE1cythaDJyWjVaamtCaXZt?=
 =?utf-8?B?OXR3ditZd3BtdkxrM3Z2TGhUV2NranB5d3hLcWt1YTlzMWtUYkszRFg3b2hF?=
 =?utf-8?B?cHFjMlFLMnVZeENvdnNSRDhYSERjNk5ZV2U4ZzFlNEtha291ejVLUDFIOHlj?=
 =?utf-8?B?TUtTZmNXZ3NPNitKbW9sMzVTRTMrOFhua3piNUQzTGxSVHFHOHhMNk1XNm1W?=
 =?utf-8?B?WFFSL05pd1g2NzhwTWllell1ZTlkRHpma0V0eFBZOTdnUkJGSjJ0VzdoaE41?=
 =?utf-8?B?N2dxSldqeWZqWFMrLzI2RVZPWVR0bUNKeW82eXlaVHQvTWRMQktZZTZtNEVm?=
 =?utf-8?B?U2lEaW5ISVRDNHp6RVgxR0pxR1JtRjRlMGRHNU5nM20rNFVpOXprWjVITCtE?=
 =?utf-8?B?Z21uK2V0THhaSXBmZVc2K0JRRTQ5MS9tb1VsNkxXZ3FnOHJXd1R0Y1M4M2Jk?=
 =?utf-8?B?alE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 00add4cb-4d2b-4784-ce2d-08db0496afed
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3768.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Feb 2023 20:55:44.8447
 (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: md14vRMQ026J7CAgn3aue4kDPQLhy958+HHYbhFrPqN6pkHzV8inPkhGoGw9R5H8dKLOjhDHTbNmhEuvu3SoIQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB6395
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 1/31/2023 9:04 PM, Greg KH wrote:
> On Tue, Jan 31, 2023 at 03:43:00PM -0800, Jithu Joseph wrote:
>> The interface to trigger Array BIST test and obtain its result
>> is similar to the existing scan test. The only notable
>> difference is that, Array BIST doesn't require any test content
>> to be loaded. So binary load related options are not needed for
>> this test.
>>
>> Add sysfs interface array BIST test, the testing support will
>> be added by subsequent patch.
> 
> What is "sysfs interface array" exactly?

Pardon the typo ... meant to write "sysfs interface for array BIST test ..."
sysfs entries for this new test will appear under "/sys/devices/virtual/misc/intel_ifs_1/"
(whereas the existing scan test entries will be under "/sys/devices/virtual/misc/intel_ifs_0/")

> 
> Where is the new Documentation/ABI/ entries for these new sysfs files
> you added?
> 

Current documentation and ABI entries are somewhat generalized (we mention intel_ifs_<n>). 
Pasting a small snippet below for reference from Documentation/ABI/testing/sysfs-platform-intel-ifs

<snip>
What:		/sys/devices/virtual/misc/intel_ifs_<N>/run_test
Date:		Nov 16 2022
KernelVersion:	6.2
Contact:	"Jithu Joseph" <jithu.joseph@intel.com>
Description:	Write <cpu#> to trigger IFS test for one online core.
		Note that the test is per core. The cpu# can be
                ...
</snip>

I can take another pass through both, to call out the differences wherever applicable
(like test image loading is not applicable for array BIST test)
...

>> @@ -156,3 +156,18 @@ const struct attribute_group **ifs_get_groups(void)
>>  {
>>  	return plat_ifs_groups;
>>  }
>> +
>> +/* global array sysfs attributes */
>> +static struct attribute *plat_ifs_array_attrs[] = {
>> +	&dev_attr_details.attr,
>> +	&dev_attr_status.attr,
>> +	&dev_attr_run_test.attr,
>> +	NULL
>> +};
>> +
>> +ATTRIBUTE_GROUPS(plat_ifs_array);
>> +
>> +const struct attribute_group **ifs_get_array_groups(void)
>> +{
>> +	return plat_ifs_array_groups;
>> +}
> 
> Why do you need a function to get access to a static variable?  Just
> make the variable not static.

The above was in-line with the existing driver style for the first test 
But if it is more appropriate, I can move "ATTRIBUTE_GROUPS(plat_ifs_array)" to core.c (where it is consumed) and drop the function

Jithu