| Message ID | 20230301015942.462799-2-jithu.joseph@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add Array BIST test support to IFS | expand |
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 >
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; +}; + /** * 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; }; @@ -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);