| Message ID | 20230214234426.344960-3-jithu.joseph@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add Array BIST test support to IFS | expand |
On Tue, Feb 14, 2023 at 03:44:21PM -0800, Jithu Joseph wrote: > Array BIST is a new type of core test introduced under the Intel Infield > Scan (IFS) suite of tests. > > Emerald Rapids (EMR) is the first CPU to support Array BIST. > Array BIST performs tests on some portions of the core logic such as > caches and register files. These are different portions of the silicon > compared to the parts tested by the first test type > i.e Scan at Field (SAF). > > Make changes in the device driver init flow to register this new test > type with the device driver framework. Each test will have its own > sysfs directory (intel_ifs_0 , intel_ifs_1) under misc hierarchy to > accommodate for the differences in test type and how they are initiated. > > Upcoming patches will add actual support. > > Signed-off-by: Jithu Joseph <jithu.joseph@intel.com> > Reviewed-by: Tony Luck <tony.luck@intel.com> > --- > drivers/platform/x86/intel/ifs/ifs.h | 5 ++ > drivers/platform/x86/intel/ifs/core.c | 70 ++++++++++++++++++--------- > 2 files changed, 52 insertions(+), 23 deletions(-) > > diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h > index 046e39304fd5..2cef88a88aa9 100644 > --- a/drivers/platform/x86/intel/ifs/ifs.h > +++ b/drivers/platform/x86/intel/ifs/ifs.h > @@ -137,6 +137,11 @@ > #define SCAN_TEST_PASS 1 > #define SCAN_TEST_FAIL 2 > > +enum test_types { > + IFS_SAF, > + IFS_ARRAY, As you are using this enum to index an array, don't you need to set the starting value to be sure it's 0? But note, that's a horrid name of an enumerated type that is in a .h file, either put it only in the .c file, or give it a name that makes more sense that it belongs only to this driver. Yes, naming is hard. Wait, you don't even use the enumerated type anywhere in this patch series only the value, did you mean for this to happen? Why name it anything? > +}; > + > /* MSR_SCAN_HASHES_STATUS bit fields */ > union ifs_scan_hashes_status { > u64 data; > diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c > index 206a617c2e02..ab234620ef4c 100644 > --- a/drivers/platform/x86/intel/ifs/core.c > +++ b/drivers/platform/x86/intel/ifs/core.c > @@ -16,27 +16,44 @@ > > static const struct x86_cpu_id ifs_cpu_ids[] __initconst = { > X86_MATCH(SAPPHIRERAPIDS_X), > + X86_MATCH(EMERALDRAPIDS_X), > {} > }; > MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids); > > -static struct ifs_device ifs_device = { > - .data = { > - .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT, > - .test_num = 0, > +static struct ifs_device ifs_devices[] = { > + [IFS_SAF] = { > + .data = { > + .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT, > + .test_num = IFS_SAF, > + }, > + .misc = { > + .name = "intel_ifs_0", > + .nodename = "intel_ifs/0", I just noticed this, a device node called "0" is not good, why do you need this in a subdir at all? > + .minor = MISC_DYNAMIC_MINOR, > + }, > }, > - .misc = { > - .name = "intel_ifs_0", > - .nodename = "intel_ifs/0", > - .minor = MISC_DYNAMIC_MINOR, > + [IFS_ARRAY] = { > + .data = { > + .integrity_cap_bit = MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT, > + .test_num = IFS_ARRAY, > + }, > + .misc = { > + .name = "intel_ifs_1", > + .nodename = "intel_ifs/1", Again, a device node called "1"? > + .minor = MISC_DYNAMIC_MINOR, > + }, > }, > }; > > +#define IFS_NUMTESTS ARRAY_SIZE(ifs_devices) Don't do this, just have a list with a NULL entry at the end, makes things much simpler and easier over time. > + > static int __init ifs_init(void) > { > const struct x86_cpu_id *m; > + int ndevices = 0; > u64 msrval; > - int ret; > + int i; > > m = x86_match_cpu(ifs_cpu_ids); > if (!m) > @@ -51,28 +68,35 @@ static int __init ifs_init(void) > if (rdmsrl_safe(MSR_INTEGRITY_CAPS, &msrval)) > return -ENODEV; > > - ifs_device.misc.groups = ifs_get_groups(); > - > - if (!(msrval & BIT(ifs_device.data.integrity_cap_bit))) > - return -ENODEV; > + for (i = 0; i < IFS_NUMTESTS; i++) { > + if (!(msrval & BIT(ifs_devices[i].data.integrity_cap_bit))) > + continue; > > - ifs_device.data.pkg_auth = kmalloc_array(topology_max_packages(), sizeof(bool), GFP_KERNEL); > - if (!ifs_device.data.pkg_auth) > - return -ENOMEM; > + ifs_devices[i].data.pkg_auth = kmalloc_array(topology_max_packages(), > + sizeof(bool), GFP_KERNEL); > + if (!ifs_devices[i].data.pkg_auth) > + continue; You have a static array of a structure that contains both things that describe the devices being used, as well as dynamic data with no real lifespan rules. Please don't perputate this common design pattern mistake. Always try to make static data constant and make dynamic data dynamic with proper reference counted lifetime rules. People converting this code into rust in the future will thank you :) thanks, greg k-h
>> +enum test_types { >> + IFS_SAF, >> + IFS_ARRAY, > > As you are using this enum to index an array, don't you need to set the > starting value to be sure it's 0? C standard says: "The value of the first enumerator (if it does not use = constant-expression) is zero." >> +#define IFS_NUMTESTS ARRAY_SIZE(ifs_devices) > > Don't do this, just have a list with a NULL entry at the end, makes > things much simpler and easier over time. Maintainers seem to be divided over this. Personally, I'm also in favor of a NULL entry to mark the last entry (I think it was one of Kernighan and Plauger's style guides "End markers are better than counts"). But the tiny footprint folks have beaten me up in the past for wasting a whole extra structure element just to include a terminator. :-( -Tony
On 2/16/2023 4:40 AM, Greg KH wrote: > On Tue, Feb 14, 2023 at 03:44:21PM -0800, Jithu Joseph wrote: > But note, that's a horrid name of an enumerated type that is in a .h > file, either put it only in the .c file, or give it a name that makes > more sense that it belongs only to this driver. > > Yes, naming is hard. Even for a driver local header, I agree that "enum ifs_test_type" would have been more appropriate than "enum test_types > > Wait, you don't even use the enumerated type anywhere in this patch > series only the value, did you mean for this to happen? Why name it > anything? > Since I am only using the values as you said, I will remove the type and use #defines >> +static struct ifs_device ifs_devices[] = { >> + [IFS_SAF] = { >> + .data = { >> + .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT, >> + .test_num = IFS_SAF, >> + }, >> + .misc = { >> + .name = "intel_ifs_0", >> + .nodename = "intel_ifs/0", > > I just noticed this, a device node called "0" is not good, why do you > need this in a subdir at all? There is no need for the device nodename in /dev to have a distinct ".nodename" from the sysfs "name" I will remove the separate .nodename line so that /dev entries are created with the same name as the sysfs directories. > >> + .minor = MISC_DYNAMIC_MINOR, >> + }, >> }, >> - .misc = { >> - .name = "intel_ifs_0", >> - .nodename = "intel_ifs/0", >> - .minor = MISC_DYNAMIC_MINOR, >> + [IFS_ARRAY] = { >> + .data = { >> + .integrity_cap_bit = MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT, >> + .test_num = IFS_ARRAY, >> + }, >> + .misc = { >> + .name = "intel_ifs_1", >> + .nodename = "intel_ifs/1", > > Again, a device node called "1"? Will remove this distinct "nodename" too > >> + >> static int __init ifs_init(void) >> { >> const struct x86_cpu_id *m; >> + int ndevices = 0; >> u64 msrval; >> - int ret; >> + int i; >> >> m = x86_match_cpu(ifs_cpu_ids); >> if (!m) >> @@ -51,28 +68,35 @@ static int __init ifs_init(void) >> if (rdmsrl_safe(MSR_INTEGRITY_CAPS, &msrval)) >> return -ENODEV; >> >> - ifs_device.misc.groups = ifs_get_groups(); >> - >> - if (!(msrval & BIT(ifs_device.data.integrity_cap_bit))) >> - return -ENODEV; >> + for (i = 0; i < IFS_NUMTESTS; i++) { >> + if (!(msrval & BIT(ifs_devices[i].data.integrity_cap_bit))) >> + continue; >> >> - ifs_device.data.pkg_auth = kmalloc_array(topology_max_packages(), sizeof(bool), GFP_KERNEL); >> - if (!ifs_device.data.pkg_auth) >> - return -ENOMEM; >> + ifs_devices[i].data.pkg_auth = kmalloc_array(topology_max_packages(), >> + sizeof(bool), GFP_KERNEL); >> + if (!ifs_devices[i].data.pkg_auth) >> + continue; > > You have a static array of a structure that contains both things that > describe the devices being used, as well as dynamic data with no real > lifespan rules. Please don't perputate this common design pattern > mistake. > > Always try to make static data constant and make dynamic data dynamic > with proper reference counted lifetime rules. People converting this > code into rust in the future will thank you :) I may not have fully understood your comment. So pardon me if the following description on the lifecycle of the dynamic allocated memory is not to the point. The lifetime of this allocation matches the load time of the driver (allocated on init, freed on exit). There are no further / allocations or freeing anywhere within the driver. There is only a single place where this memory is used, whose access is serialized via a semaphore. Given the above, does moving this one and only allocation to use a global pointer instead of one embedded in the structure is what is needed. ? Jithu
On Thu, Feb 16, 2023 at 02:57:13PM -0800, Joseph, Jithu wrote: > On 2/16/2023 4:40 AM, Greg KH wrote: > >> + ifs_devices[i].data.pkg_auth = kmalloc_array(topology_max_packages(), > >> + sizeof(bool), GFP_KERNEL); > >> + if (!ifs_devices[i].data.pkg_auth) > >> + continue; > > > > You have a static array of a structure that contains both things that > > describe the devices being used, as well as dynamic data with no real > > lifespan rules. Please don't perputate this common design pattern > > mistake. > > > > Always try to make static data constant and make dynamic data dynamic > > with proper reference counted lifetime rules. People converting this > > code into rust in the future will thank you :) > > I may not have fully understood your comment. So pardon me if the following description > on the lifecycle of the dynamic allocated memory is not to the point. > > The lifetime of this allocation matches the load time of the driver (allocated on init, freed on exit). > There are no further / allocations or freeing anywhere within the driver. > There is only a single place where this memory is used, whose access is serialized via a semaphore. But the memory is associated with "something" that has a lifetime, right? This is either a misc device, or a cpu, or a platform device, or something that you have to determine that you need to allocate it. So use that as the thing you hang your dynamic data off of, don't use a static array. Allow that static array to be put into read-only memory (i.e. it is const and can not be changed by your code accidentally or on purpose.) Does that help explain things better? thanks, greg k-h
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h index 046e39304fd5..2cef88a88aa9 100644 --- a/drivers/platform/x86/intel/ifs/ifs.h +++ b/drivers/platform/x86/intel/ifs/ifs.h @@ -137,6 +137,11 @@ #define SCAN_TEST_PASS 1 #define SCAN_TEST_FAIL 2 +enum test_types { + IFS_SAF, + IFS_ARRAY, +}; + /* MSR_SCAN_HASHES_STATUS bit fields */ union ifs_scan_hashes_status { u64 data; diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c index 206a617c2e02..ab234620ef4c 100644 --- a/drivers/platform/x86/intel/ifs/core.c +++ b/drivers/platform/x86/intel/ifs/core.c @@ -16,27 +16,44 @@ static const struct x86_cpu_id ifs_cpu_ids[] __initconst = { X86_MATCH(SAPPHIRERAPIDS_X), + X86_MATCH(EMERALDRAPIDS_X), {} }; MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids); -static struct ifs_device ifs_device = { - .data = { - .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT, - .test_num = 0, +static struct ifs_device ifs_devices[] = { + [IFS_SAF] = { + .data = { + .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT, + .test_num = IFS_SAF, + }, + .misc = { + .name = "intel_ifs_0", + .nodename = "intel_ifs/0", + .minor = MISC_DYNAMIC_MINOR, + }, }, - .misc = { - .name = "intel_ifs_0", - .nodename = "intel_ifs/0", - .minor = MISC_DYNAMIC_MINOR, + [IFS_ARRAY] = { + .data = { + .integrity_cap_bit = MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT, + .test_num = IFS_ARRAY, + }, + .misc = { + .name = "intel_ifs_1", + .nodename = "intel_ifs/1", + .minor = MISC_DYNAMIC_MINOR, + }, }, }; +#define IFS_NUMTESTS ARRAY_SIZE(ifs_devices) + static int __init ifs_init(void) { const struct x86_cpu_id *m; + int ndevices = 0; u64 msrval; - int ret; + int i; m = x86_match_cpu(ifs_cpu_ids); if (!m) @@ -51,28 +68,35 @@ static int __init ifs_init(void) if (rdmsrl_safe(MSR_INTEGRITY_CAPS, &msrval)) return -ENODEV; - ifs_device.misc.groups = ifs_get_groups(); - - if (!(msrval & BIT(ifs_device.data.integrity_cap_bit))) - return -ENODEV; + for (i = 0; i < IFS_NUMTESTS; i++) { + if (!(msrval & BIT(ifs_devices[i].data.integrity_cap_bit))) + continue; - ifs_device.data.pkg_auth = kmalloc_array(topology_max_packages(), sizeof(bool), GFP_KERNEL); - if (!ifs_device.data.pkg_auth) - return -ENOMEM; + ifs_devices[i].data.pkg_auth = kmalloc_array(topology_max_packages(), + sizeof(bool), GFP_KERNEL); + if (!ifs_devices[i].data.pkg_auth) + continue; + ifs_devices[i].misc.groups = ifs_get_groups(); - ret = misc_register(&ifs_device.misc); - if (ret) { - kfree(ifs_device.data.pkg_auth); - return ret; + if (misc_register(&ifs_devices[i].misc)) + kfree(ifs_devices[i].data.pkg_auth); + else + ndevices++; } - return 0; + return ndevices ? 0 : -ENODEV; } static void __exit ifs_exit(void) { - misc_deregister(&ifs_device.misc); - kfree(ifs_device.data.pkg_auth); + int i; + + for (i = 0; i < IFS_NUMTESTS; i++) { + if (ifs_devices[i].misc.this_device) { + misc_deregister(&ifs_devices[i].misc); + kfree(ifs_devices[i].data.pkg_auth); + } + } } module_init(ifs_init);