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] [day] [month] [year] [list]

Message-ID: <7370c697-6c71-47ff-843c-e6c20cebbb5e@linux.intel.com>
Date: Tue, 14 Jan 2025 15:13:43 -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

Hi,

Thanks for the feedback. I will send out a new version on rc1 of 6.14 cycle.

Xi

On 1/14/2025 8:39 AM, Xi Pardee wrote:
>
> 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
>
>