Date: Mon, 27 May 2024 13:01:07 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
cc: Hans de Goede <hdegoede@...hat.com>, 
    Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
    platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] platform/x86: ISST: Add model specific loading for
 common module

On Thu, 9 May 2024, Srinivas Pandruvada wrote:

> SST common module is loaded when model specific or TPMI SST driver
> registers for services. There are model specific features used in SST
> common modules which are checked with a CPU model list. So, this module
> is model specific.
> 
> There are some use cases where loading the common module independently
> only on the supported CPU models helps. The first use case is for
> preventing SST TPMI module loading if the model specific features are
> not implemented. The second use case for presenting information to
> user space when SST is used in OOB (Out of Band) mode.
> 
> 1.
> With TPMI, SST interface is architectural. This means that no need to add
> new PCI device IDs for new CPU models. This means that there can be lag
> in adding CPU models for the model specific features in the common
> module. For example, before adding CPU model to GRANITERAPIDS_D to
> hpm_cpu_ids[], SST is still functional for some features and but will
> get/set wrong data for features like SST-CP. This is because IOCTL
> ISST_IF_GET_PHY_ID, will not give correct mapping for newer CPU models.
> So adding explicit model check during load time will prevent such cases.
> For unsupported CPU models, common driver will fail to load and hence
> dependent modules will not be loaded.
> 
> 2.
> When the SST TPMI features are controlled by some OOB agent (not from OS
> interface), even if the CPU model is supported, there will be no user
> space interface available for tools as SST TPMI modules will not
> be loaded. User space interface is registered when TPMI modules call
> isst_if_cdev_register(). Even in this case user space orchestrator
> software needs to get power domain information to schedule workload and
> get/set turbo ratio limits. This information is exposed by the common
> module using IOCTLs ISST_IF_GET_PHY_ID and ISST_IF_MSR_COMMAND
> respectively. Since the user space MSR access can be locked, direct MSR
> access from the user space is not an option using /dev/cpu/*/msr.
> 
> Converge all the existing model checks to one common place and
> use driver data to differentiate. On successful model check
> call isst_misc_reg().
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
> Reviewed-by: Zhang Rui <rui.zhang@...el.com
> ---
> Note:
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> Andy's Reviewed-by was for one version before Rafael and Rui's
> comments were addressed. So didn't add it before "---". 
> We can add it if there is no issue from Andy.
> 
> Also incorporated changes suggested by "Wysocki, Rafael J"
> <rafael.j.wysocki@...el.com>.
> 
> We have all the models supported till date are already in 6.9
> kernel. So this is more for future proofing for the first use case.
> 
> 
>  .../intel/speed_select_if/isst_if_common.c    | 80 +++++++++++++------
>  1 file changed, 56 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/speed_select_if/isst_if_common.c b/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
> index 88a17be7cb7e..f886f9369fad 100644
> --- a/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
> +++ b/drivers/platform/x86/intel/speed_select_if/isst_if_common.c
> @@ -718,12 +718,6 @@ static struct miscdevice isst_if_char_driver = {
>  	.fops		= &isst_if_char_driver_ops,
>  };
>  
> -static const struct x86_cpu_id hpm_cpu_ids[] = {
> -	X86_MATCH_INTEL_FAM6_MODEL(GRANITERAPIDS_X,	NULL),
> -	X86_MATCH_INTEL_FAM6_MODEL(ATOM_CRESTMONT_X,	NULL),
> -	{}
> -};
> -
>  static int isst_misc_reg(void)
>  {
>  	mutex_lock(&punit_misc_dev_reg_lock);
> @@ -731,12 +725,6 @@ static int isst_misc_reg(void)
>  		goto unlock_exit;
>  
>  	if (!misc_usage_count) {
> -		const struct x86_cpu_id *id;
> -
> -		id = x86_match_cpu(hpm_cpu_ids);
> -		if (id)
> -			isst_hpm_support = true;
> -
>  		misc_device_ret = isst_if_cpu_info_init();
>  		if (misc_device_ret)
>  			goto unlock_exit;
> @@ -784,8 +772,6 @@ static void isst_misc_unreg(void)
>   */
>  int isst_if_cdev_register(int device_type, struct isst_if_cmd_cb *cb)
>  {
> -	int ret;
> -
>  	if (device_type >= ISST_IF_DEV_MAX)
>  		return -EINVAL;
>  
> @@ -803,15 +789,6 @@ int isst_if_cdev_register(int device_type, struct isst_if_cmd_cb *cb)
>  	punit_callbacks[device_type].registered = 1;
>  	mutex_unlock(&punit_misc_dev_open_lock);
>  
> -	ret = isst_misc_reg();
> -	if (ret) {
> -		/*
> -		 * No need of mutex as the misc device register failed
> -		 * as no one can open device yet. Hence no contention.
> -		 */
> -		punit_callbacks[device_type].registered = 0;
> -		return ret;
> -	}
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(isst_if_cdev_register);
> @@ -827,7 +804,6 @@ EXPORT_SYMBOL_GPL(isst_if_cdev_register);
>   */
>  void isst_if_cdev_unregister(int device_type)
>  {
> -	isst_misc_unreg();
>  	mutex_lock(&punit_misc_dev_open_lock);
>  	punit_callbacks[device_type].def_ioctl = NULL;
>  	punit_callbacks[device_type].registered = 0;
> @@ -837,5 +813,61 @@ void isst_if_cdev_unregister(int device_type)
>  }
>  EXPORT_SYMBOL_GPL(isst_if_cdev_unregister);
>  
> +#define SST_HPM_SUPPORTED		0x01
> +#define SST_MBOX_SUPPORTED		0x02
> +
> +#define MSR_OS_MAILBOX_INTERFACE        0xB0
> +#define MSR_OS_MAILBOX_DATA             0xB1
> +
> +static const struct x86_cpu_id isst_cpu_ids[] = {
> +	X86_MATCH_INTEL_FAM6_MODEL(ATOM_CRESTMONT,	SST_HPM_SUPPORTED),
> +	X86_MATCH_INTEL_FAM6_MODEL(ATOM_CRESTMONT_X,	SST_HPM_SUPPORTED),
> +	X86_MATCH_INTEL_FAM6_MODEL(EMERALDRAPIDS_X,	0),
> +	X86_MATCH_INTEL_FAM6_MODEL(GRANITERAPIDS_D,	SST_HPM_SUPPORTED),
> +	X86_MATCH_INTEL_FAM6_MODEL(GRANITERAPIDS_X,	SST_HPM_SUPPORTED),
> +	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_D,		0),
> +	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		0),
> +	X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X,	0),
> +	X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X,		SST_MBOX_SUPPORTED),
> +	{}
> +};
> +MODULE_DEVICE_TABLE(x86cpu, isst_cpu_ids);
> +
> +static int __init isst_if_common_init(void)
> +{
> +	const struct x86_cpu_id *id;
> +
> +	id = x86_match_cpu(isst_cpu_ids);
> +	if (!id)
> +		return -ENODEV;
> +
> +	if (!id->driver_data)
> +		goto misc_reg;
> +
> +	if (id->driver_data == SST_HPM_SUPPORTED) {
> +		isst_hpm_support = true;
> +		goto misc_reg;
> +	}
> +
> +	if (id->driver_data == SST_MBOX_SUPPORTED) {
> +		u64 data;
> +
> +		/* Can fail only on some Skylake-X generations */
> +		if (rdmsrl_safe(MSR_OS_MAILBOX_INTERFACE, &data) ||
> +		    rdmsrl_safe(MSR_OS_MAILBOX_DATA, &data))
> +			return -ENODEV;

This Skylake bit seems to come out of nowhere which is odd for a refactor 
patch, can it be in own patch with proper commit message written for it?

-- 
 i.

> +	}
> +
> +misc_reg:
> +	return isst_misc_reg();
> +}
> +module_init(isst_if_common_init)
> +
> +static void __exit isst_if_common_exit(void)
> +{
> +	isst_misc_unreg();
> +}
> +module_exit(isst_if_common_exit)
> +
>  MODULE_DESCRIPTION("ISST common interface module");
>  MODULE_LICENSE("GPL v2");
> 

Powered by blists - more mailing lists