linux-kernel - Re: [PATCH v4 4/6] platform/x86/intel/pmc: Create architecture specific callback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a6239ca1-8eae-48e3-ba49-0bab81438b68@linux.intel.com>
Date: Tue, 14 Jan 2025 08:39:35 -0800
From: Xi Pardee <xi.pardee@...ux.intel.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: rajvi0912@...il.com, irenic.rajneesh@...il.com,
 david.e.box@...ux.intel.com, Hans de Goede <hdegoede@...hat.com>,
 platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
 linux-pm@...r.kernel.org
Subject: Re: [PATCH v4 4/6] platform/x86/intel/pmc: Create architecture
 specific callback


On 1/10/2025 3:50 AM, Ilpo Järvinen wrote:
> On Thu, 9 Jan 2025, Xi Pardee wrote:
>
>> Add architecture specific callback field in pmc_dev_info structure.
>> Architecture specific action could be handled in this callback instead
>> of per architecture init functions. Convert Arrow Lake, Lunar Lake,
>> Meteor Lake and Tiger Lake platforms to use this field.
>>
>> Signed-off-by: Xi Pardee <xi.pardee@...ux.intel.com>
>> ---
>>   drivers/platform/x86/intel/pmc/arl.c  | 15 +++++++--------
>>   drivers/platform/x86/intel/pmc/core.c |  3 +++
>>   drivers/platform/x86/intel/pmc/core.h |  7 +++++++
>>   drivers/platform/x86/intel/pmc/lnl.c  | 15 +++++++--------
>>   drivers/platform/x86/intel/pmc/mtl.c  | 15 +++++++--------
>>   drivers/platform/x86/intel/pmc/tgl.c  | 15 +++++----------
>>   6 files changed, 36 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmc/arl.c b/drivers/platform/x86/intel/pmc/arl.c
>> index dedf752237ca0..9ff90d32a635f 100644
>> --- a/drivers/platform/x86/intel/pmc/arl.c
>> +++ b/drivers/platform/x86/intel/pmc/arl.c
>> @@ -698,16 +698,15 @@ static struct pmc_dev_info arl_pmc_dev = {
>>   	.map = &arl_socs_reg_map,
>>   	.suspend = cnl_suspend,
>>   	.resume = arl_resume,
>> +	.arch_specific = arl_specific_init,
>>   };
>>   
>> -int arl_core_init(struct pmc_dev *pmcdev)
>> +void arl_specific_init(struct pmc_dev *pmcdev)
>>   {
>> -	int ret;
>> -
>> -	ret = generic_core_init(pmcdev, &arl_pmc_dev);
>> -	if (ret)
>> -		return ret;
>> -
>>   	arl_d3_fixup();
>> -	return 0;
>> +}
> As I tried to explain already earlier I think the older form is better
> here because it would allow arch specific things in any order:
>
> void xx_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info)
> {
> 	int ret;
>
> 	xx_pre_fixup();
>
> 	ret = generic_core_init(pmcdev, pmc_dev_info);
> 	if (ret)
> 		return ret;
>
> 	xx_post_fixup();
>
> 	return 0;
> }
>
> If you make it a callback, you have pick either pre or post but cannot do
> both with a single callback. My suggestion would also allow replacing
> generic_core_init() completely if needed in future.
>
> Note how I pass the info parameter above to xx_init() so it can call into
> generic_core_init(). In the core, you'd do this to pick which init
> function to use:
>
> 	if (pmc_dev_info->init)
> 		ret = pmc_dev_info->init(pmcdev, pmc_dev_info);
> 	else
> 		ret = generic_core_init(pmcdev, pmc_dev_info);


Thanks for the feedback. I will change back to init function per 
architect in next version.

Xi