Message-ID: <2ae25756-7403-ad5f-548a-50e633040bf9@intel.com>
Date:   Mon, 13 Mar 2023 14:34:47 -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 1/8] platform/x86/intel/ifs: Reorganize driver data
Content-Language: en-US
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-2-jithu.joseph@intel.com>
 <a6ae936e-effd-3794-e6fb-11b33a4d6eb1@redhat.com>
From:   "Joseph, Jithu" <jithu.joseph@intel.com>
In-Reply-To: <a6ae936e-effd-3794-e6fb-11b33a4d6eb1@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?WE8vYUxaakhBQjZkK3JMVXhEbmd1N1lJaHVIYnZKUlVtV2Fhb25HK2RGVHZn?=
 =?utf-8?B?bHNKdDNHK24xU01nRE0rek5xV29Pc0ZQbTdzZ2ZDenVWRnB6M0F5R2dicEF5?=
 =?utf-8?B?SW5tQ3NzU2txZ04vTkd4QTFQQll1VFk0ZDlqYWt5d2o2K2t1SFFzeXVIa2lC?=
 =?utf-8?B?UUlDQ2ZoUUpwc1pIQTE0V3JwNXoyUnZDUytyVWIvK21UV0lVZURsc2tsaGN3?=
 =?utf-8?B?TWlLdmphQk56SlBxc1FmaTcwdUtRVHlwZWg4V1VGNzE3N0l1a0xDTitKcXJo?=
 =?utf-8?B?blE3N3dIZVFUSXpnWWxFZ2dySlhVcm5nSzdrWUhwMGl0WDJjSHJKN1pMNWRt?=
 =?utf-8?B?d1VVYzQ1cy9POUJ5aUVlSzBxNk1sbVhweGw5R3FBd01lemZYK0QzcWwzcEZi?=
 =?utf-8?B?VUZFdU8wY29HRWIxdEIrZ1hEQzk3NG8zVFkwM0FWaUNtLytkR2krdW0ycnlT?=
 =?utf-8?B?eFVscklwVlVSdjRDbHB5aC8xc1gyR0ZLSDNTN2FNTXNPaXExQlN1NGF4ME9O?=
 =?utf-8?B?V1ppQUpVSUpmajN4WW11L0xnUkdFNndSdzlQakMyY2ZJLy9ydFdMcUtTUUVm?=
 =?utf-8?B?Qm5NMktIZGRkd0tleDN0dm0ycUFhMGU0RGk2cngydlN1b2FiMGt0UkZocVp2?=
 =?utf-8?B?K1oyVjJSb2RXOFNhUHRVR2tiQmY5QWgwaFFnZUpTSERwZFBSUThoN2hLT2pJ?=
 =?utf-8?B?c3NITzdPOEVxZ0pOditNazZjMEppbUo0ZGJDVFYxZEh1dy9KVzVBMkhFejVQ?=
 =?utf-8?B?dTJEOXduRDhPNTB1NmV3NDY2dVQwKysrdlZVRU5ncHFnMENtbU8xdzFLTEZR?=
 =?utf-8?B?ZjZpWmVXRWdqN1dOaXY1U1FjQXdVZG5xbkx5dld2UldxbHR2T2JaOVJnRXB2?=
 =?utf-8?B?QkExYW5ITktBb3dqWXNDTHd6UExPc3JmWU5TWXc1VzhYYmN1UUVVamZ5eEw5?=
 =?utf-8?B?YnEvOEZlK0tyODRwRDVYTjNxa0FjWFk5NFZIeVVEbnlaemtFbnBLemxWUVZQ?=
 =?utf-8?B?V0UrMVlQM0JCYldaNkFFcXN5NEZvbTNTdG9zLzhYYkZhOXNhY0tTWlo1S1Fq?=
 =?utf-8?B?eGRqMWcvRldKdFhJaWlYdlgzL2J0bDU2QmdxT1hCUVVwVjJ4dG1RTjdENnZt?=
 =?utf-8?B?YjlURDBpYUZFaVF3QTRRVkQrSEdJQ1hSWU5QbGVNcUgvVkpycFY5MFZRdzlE?=
 =?utf-8?B?WlB2blNvbENVbzVGTG0xWThUdkwvYWRUeFBWSVVsNHVDL25oNnN2RXppcDlp?=
 =?utf-8?B?OHBibWpGYU5mYnFhY3E1d1VjRmlWNkZOa1AwWGowMEpMWmpHZGNZV0xtc2Vu?=
 =?utf-8?B?ZXRJWVRRTlpTbkZyRmhwcnZtdmxBeCt5Y3IrSWlIRkl4c0dHZENGcU9aK3lO?=
 =?utf-8?B?cCtBSFgxaitua3ZtQ3VXVEg1eDAwRTU0d1lCY0Flc0Rkb3NudTlVajd6SllT?=
 =?utf-8?B?bzdMZnlpcUxxcGxzM0hvZW5GWE1KbG9VMUFlQmhrcFdHeGtOOW5hVkF1N3Na?=
 =?utf-8?B?d3VDRmtjNUFCeWhzT05TMkgvRm9iNUU3WkNvbUZHTTVyZzVPY01ncmVYK3ds?=
 =?utf-8?B?L3BsWmRXLzRNVWZScnJGY2g3dGxNTk1UOGRXYXZxaHlmOGhEUmNjRXg1c3li?=
 =?utf-8?B?R0Z5QW1kdkN0TFk5RnE0b3BRbnMwc1RtbmllUzU3SHN4THRtNWx1SjJscUlW?=
 =?utf-8?B?MlM3cVRLdjgvUEhTV2R3OUVoeHlpcVlZUUozc283cFdSNnpkV0c5Q3ZMNzlO?=
 =?utf-8?B?ZDFjN041NGZ5eWZpT0tvdjZsRmRzTXdYU3ZzVmxkZkt5VVJNc1JNL0FIaGds?=
 =?utf-8?B?aXRnU1RvUXpYSlhrN0FQV21CMStNT01lbjJ1eXRsektLN0NvaERJaHFOQTFI?=
 =?utf-8?B?blE0U3o3bnhFRC9BcUxEZG1FLzBQVHV0K2gyTWlNclZPbHNTUkVLSm92d1hu?=
 =?utf-8?B?cmJheEhHdHM0ODhqeXk5bWNqOE5PTmt2VUJMUFJXcDR6TGhVZXowbVpZeFNU?=
 =?utf-8?B?Q01yak9uM0RPMW9EYkNkeEhkZStWcUhGenkrT1Q2SzF3b3lzU2xlRlp1TnFw?=
 =?utf-8?B?RDZlR29aMkhOYXd6RFozd0tFMlFnUVNnaDVlZ0NWRUdIQ29KR1ZqYVFwS3Ey?=
 =?utf-8?B?SDBGb1ZMVjRyTkpiL3J3ZTM3ck03U1NpeDZ2NWVvOEEwUjdaemNCcFJyKzdp?=
 =?utf-8?B?S3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 602bdd33-bf8d-45f3-df98-08db240ac717
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3768.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Mar 2023 21:34:51.3591
 (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: dc3wlQmDZz1LoK9Jp2esffrw7/GlVvg6mXSOaFaTLRy61wOhAwDwwmmCRDTcKZpvpzlJ1/2l58Wswyr3k1GwGA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5165
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 3/13/2023 7:46 AM, Hans de Goede wrote:
> Hi Jithu,
> 
> On 3/1/23 02:59, Jithu Joseph wrote:

>>  
>> +struct ifs_const_data {
>> +	int	integrity_cap_bit;
>> +	int	test_num;
>> +};
>> +
> 
> This is a description of the specific capabilties / bits of
> the IFS on e.g. Saphire Rapids, so please name this appropriately
> for example:
> 
> struct ifs_hw_caps  {
> 	int	integrity_cap_bit;
> 	int	test_num;
> };

This can be renamed to ifs_test_caps as it holds test specific fields.

...

> 
> You got this exactly the wrong way around, there should be a single
> 
> static const struct ifs_hw_caps saphire_rapids_caps = {
> 	.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
> 	.test_num = 0,
> };
> 
> And then struct ifs_device { } should have a "const struct ifs_hw_caps *hw_caps"
> which gets initialized to point to &saphire_rapids_caps. So that your const
> data is actually const.
> 
> Where as since the r/w data's lifetime is couple to the misc-device lifetime
> there is no need to dynamically allocate it just keep that embedded, so that
> together you get:

Noted 

> 
> struct ifs_device {
> 	const struct ifs_hw_caps *hw_caps;
> 	struct ifs_data data;
> 	struct miscdevice misc;
> };
> 

The initialization portion, taking into account your suggestion above, translates to:

static const struct ifs_test_caps scan_test = {
	.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
	.test_num = IFS_TYPE_SAF,
};

static const struct ifs_test_caps array_test = {
	.integrity_cap_bit = MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT,
	.test_num = IFS_TYPE_ARRAY_BIST,
};

static struct ifs_device ifs_devices[] = {
	[IFS_TYPE_SAF] = {
		.test_caps = &scan_test,
		.misc = {
			.name = "intel_ifs_0",
			.minor = MISC_DYNAMIC_MINOR,
			.groups = plat_ifs_groups,
		},
	},
	[IFS_TYPE_ARRAY_BIST] = {
		.test_caps = &array_test,
		.misc = {
			.name = "intel_ifs_1",
			.minor = MISC_DYNAMIC_MINOR,
			.groups = plat_ifs_array_groups,
		},
	},
};

Jithu