[v3,1/8] platform/x86/intel/ifs: Reorganize driver data

  The struct holding device driver data contained both read only(ro)
and read write(rw) fields.

Separating ro fields from rw fields was recommended as
a preferable design pattern during review[1].

Group the rw fields into a separate struct whose memory is allocated
during driver_init(). Associate it to the miscdevice being registered
by keeping it in the same container struct as the miscdevice.

Also in prepration to supporting additional tests, move ifs_pkg_auth
to a global as it is only applicable for the first test type.

Link: https://lore.kernel.org/lkml/Y+9H9otxLYPqMkUh@kroah.com/ [1]

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 drivers/platform/x86/intel/ifs/ifs.h  | 19 +++++++++-------
 drivers/platform/x86/intel/ifs/core.c | 31 ++++++++++++++++++---------
 drivers/platform/x86/intel/ifs/load.c |  8 +++----
 3 files changed, 36 insertions(+), 22 deletions(-)
  

Hi Jithu,

On 3/1/23 02:59, Jithu Joseph wrote:
> The struct holding device driver data contained both read only(ro)
> and read write(rw) fields.
> 
> Separating ro fields from rw fields was recommended as
> a preferable design pattern during review[1].
> 
> Group the rw fields into a separate struct whose memory is allocated
> during driver_init(). Associate it to the miscdevice being registered
> by keeping it in the same container struct as the miscdevice.
> 
> Also in prepration to supporting additional tests, move ifs_pkg_auth
> to a global as it is only applicable for the first test type.

If you are writing "Also ..." into a commit message and the
changes for the "Also ..." are more then a single line change,
then that change really should be split out into a separate patch.

Please split the "move ifs_pkg_auth to a global" changes into their
own separate patch.

> 
> Link: https://lore.kernel.org/lkml/Y+9H9otxLYPqMkUh@kroah.com/ [1]
> 
> Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> ---
>  drivers/platform/x86/intel/ifs/ifs.h  | 19 +++++++++-------
>  drivers/platform/x86/intel/ifs/core.c | 31 ++++++++++++++++++---------
>  drivers/platform/x86/intel/ifs/load.c |  8 +++----
>  3 files changed, 36 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index 046e39304fd5..e07463c794d4 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -197,22 +197,23 @@ union ifs_status {
>  #define IFS_SW_TIMEOUT				0xFD
>  #define IFS_SW_PARTIAL_COMPLETION		0xFE
>  
> +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;
};


>  /**
>   * struct ifs_data - attributes related to intel IFS driver
> - * @integrity_cap_bit: MSR_INTEGRITY_CAPS bit enumerating this test
>   * @loaded_version: stores the currently loaded ifs image version.
> - * @pkg_auth: array of bool storing per package auth status
>   * @loaded: If a valid test binary has been loaded into the memory
>   * @loading_error: Error occurred on another CPU while loading image
>   * @valid_chunks: number of chunks which could be validated.
>   * @status: it holds simple status pass/fail/untested
>   * @scan_details: opaque scan status code from h/w
>   * @cur_batch: number indicating the currently loaded test file
> - * @test_num: number indicating the test type
> + * @ro_info: ptr to struct holding fixed details
>   */
>  struct ifs_data {
> -	int	integrity_cap_bit;
> -	bool	*pkg_auth;
>  	int	loaded_version;
>  	bool	loaded;
>  	bool	loading_error;
> @@ -220,7 +221,7 @@ struct ifs_data {
>  	int	status;
>  	u64	scan_details;
>  	u32	cur_batch;
> -	int	test_num;
> +	struct ifs_const_data *ro_info;
>  };
>  
>  struct ifs_work {
> @@ -229,7 +230,8 @@ struct ifs_work {
>  };
>  
>  struct ifs_device {
> -	struct ifs_data data;
> +	struct ifs_const_data ro_data;
> +	struct ifs_data *rw_data;
>  	struct miscdevice misc;
>  };
>  

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:

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

Regards,

Hans


> @@ -238,9 +240,10 @@ static inline struct ifs_data *ifs_get_data(struct device *dev)
>  	struct miscdevice *m = dev_get_drvdata(dev);
>  	struct ifs_device *d = container_of(m, struct ifs_device, misc);
>  
> -	return &d->data;
> +	return d->rw_data;
>  }
>  
> +extern bool *ifs_pkg_auth;
>  int ifs_load_firmware(struct device *dev);
>  int do_core_test(int cpu, struct device *dev);
>  const struct attribute_group **ifs_get_groups(void);
> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index 206a617c2e02..b518b661daf0 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -20,8 +20,10 @@ static const struct x86_cpu_id ifs_cpu_ids[] __initconst = {
>  };
>  MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids);
>  
> +bool *ifs_pkg_auth;
> +
>  static struct ifs_device ifs_device = {
> -	.data = {
> +	.ro_data = {
>  		.integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT,
>  		.test_num = 0,
>  	},
> @@ -35,8 +37,8 @@ static struct ifs_device ifs_device = {
>  static int __init ifs_init(void)
>  {
>  	const struct x86_cpu_id *m;
> +	struct ifs_data *ifsd;
>  	u64 msrval;
> -	int ret;
>  
>  	m = x86_match_cpu(ifs_cpu_ids);
>  	if (!m)
> @@ -53,26 +55,35 @@ static int __init ifs_init(void)
>  
>  	ifs_device.misc.groups = ifs_get_groups();
>  
> -	if (!(msrval & BIT(ifs_device.data.integrity_cap_bit)))
> +	if (!(msrval & BIT(ifs_device.ro_data.integrity_cap_bit)))
>  		return -ENODEV;
>  
> -	ifs_device.data.pkg_auth = kmalloc_array(topology_max_packages(), sizeof(bool), GFP_KERNEL);
> -	if (!ifs_device.data.pkg_auth)
> +	ifs_pkg_auth = kmalloc_array(topology_max_packages(), sizeof(bool), GFP_KERNEL);
> +	if (!ifs_pkg_auth)
> +		return -ENOMEM;
> +
> +	ifsd = kzalloc(sizeof(*ifsd), GFP_KERNEL);
> +	if (!ifsd)
>  		return -ENOMEM;
>  
> -	ret = misc_register(&ifs_device.misc);
> -	if (ret) {
> -		kfree(ifs_device.data.pkg_auth);
> -		return ret;
> +	ifsd->ro_info = &ifs_device.ro_data;
> +	ifs_device.rw_data = ifsd;
> +
> +	if (misc_register(&ifs_device.misc)) {
> +		kfree(ifsd);
> +		kfree(ifs_pkg_auth);
> +		return -ENODEV;
>  	}
>  
>  	return 0;
> +
>  }
>  
>  static void __exit ifs_exit(void)
>  {
>  	misc_deregister(&ifs_device.misc);
> -	kfree(ifs_device.data.pkg_auth);
> +	kfree(ifs_device.rw_data);
> +	kfree(ifs_pkg_auth);
>  }
>  
>  module_init(ifs_init);
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index c5c24e6fdc43..cdec3316c08d 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -192,7 +192,7 @@ static int scan_chunks_sanity_check(struct device *dev)
>  	struct ifs_work local_work;
>  	int curr_pkg, cpu, ret;
>  
> -	memset(ifsd->pkg_auth, 0, (topology_max_packages() * sizeof(bool)));
> +	memset(ifs_pkg_auth, 0, (topology_max_packages() * sizeof(bool)));
>  	ret = validate_ifs_metadata(dev);
>  	if (ret)
>  		return ret;
> @@ -204,7 +204,7 @@ static int scan_chunks_sanity_check(struct device *dev)
>  	cpus_read_lock();
>  	for_each_online_cpu(cpu) {
>  		curr_pkg = topology_physical_package_id(cpu);
> -		if (ifsd->pkg_auth[curr_pkg])
> +		if (ifs_pkg_auth[curr_pkg])
>  			continue;
>  		reinit_completion(&ifs_done);
>  		local_work.dev = dev;
> @@ -215,7 +215,7 @@ static int scan_chunks_sanity_check(struct device *dev)
>  			ret = -EIO;
>  			goto out;
>  		}
> -		ifsd->pkg_auth[curr_pkg] = 1;
> +		ifs_pkg_auth[curr_pkg] = 1;
>  	}
>  	ret = 0;
>  out:
> @@ -263,7 +263,7 @@ int ifs_load_firmware(struct device *dev)
>  	int ret = -EINVAL;
>  
>  	snprintf(scan_path, sizeof(scan_path), "intel/ifs_%d/%02x-%02x-%02x-%02x.scan",
> -		 ifsd->test_num, boot_cpu_data.x86, boot_cpu_data.x86_model,
> +		 ifsd->ro_info->test_num, boot_cpu_data.x86, boot_cpu_data.x86_model,
>  		 boot_cpu_data.x86_stepping, ifsd->cur_batch);
>  
>  	ret = request_firmware_direct(&fw, scan_path, dev);
  

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
  

Hi,

On 3/13/23 22:34, Joseph, Jithu wrote:
> 
> 
> 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.

Ack.

>>
>> 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:

Yes, assuming we go with 1 ifs_device per test type.

Regards,

Hans


> 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
>