Date:   Mon, 13 Mar 2023 15:46:13 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Jithu Joseph <jithu.joseph@...el.com>, markgross@...nel.org
Cc:     tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
        gregkh@...uxfoundation.org, rostedt@...dmis.org,
        ashok.raj@...el.com, tony.luck@...el.com,
        linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        patches@...ts.linux.dev, ravi.v.shankar@...el.com,
        thiago.macieira@...el.com, athenas.jimenez.gonzalez@...el.com,
        sohil.mehta@...el.com
Subject: Re: [PATCH v3 1/8] platform/x86/intel/ifs: Reorganize driver data

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@...el.com>
> Reviewed-by: Tony Luck <tony.luck@...el.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);

Powered by blists - more mailing lists