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@xxxxxxxxx/ [1] > > Signed-off-by: Jithu Joseph <jithu.joseph@xxxxxxxxx> > Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx> > --- > 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);
