| Message ID | 20220419163859.2228874-4-tony.luck@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Introduce In Field Scan driver | expand |
On Tue, Apr 19, 2022 at 09:38:51AM -0700, Tony Luck wrote: > The initial implementation of IFS is model specific. Enumeration is > via a combination of family-model-stepping and a check for a bit in the > CORE_CAPABILITIES MSR. > > Linux has handled this lack of enumeration before with a code stub to > create a device. See arch/x86/kernel/pmem.c. Use the same approach > here. Ick, why? Why not just create a simple virtual device and use that? Do you really want to bind a driver to this? Or do you already "know" the only driver that you have will bind to this? pmem.c should not be used as a good example of anything, sorry. greg k-h > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Tony Luck <tony.luck@intel.com> > --- > MAINTAINERS | 7 +++ > drivers/platform/x86/intel/Kconfig | 1 + > drivers/platform/x86/intel/Makefile | 1 + > drivers/platform/x86/intel/ifs/Kconfig | 2 + > drivers/platform/x86/intel/ifs/Makefile | 1 + > .../platform/x86/intel/ifs/intel_ifs_device.c | 50 +++++++++++++++++++ > 6 files changed, 62 insertions(+) > create mode 100644 drivers/platform/x86/intel/ifs/Kconfig > create mode 100644 drivers/platform/x86/intel/ifs/Makefile > create mode 100644 drivers/platform/x86/intel/ifs/intel_ifs_device.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 40fa1955ca3f..9e372a960fa5 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -9861,6 +9861,13 @@ B: https://bugzilla.kernel.org > T: git git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git > F: drivers/idle/intel_idle.c > > +INTEL IN FIELD SCAN (IFS) DRIVER > +M: Jithu Joseph <jithu.joseph@intel.com> > +R: Ashok Raj <ashok.raj@intel.com> > +R: Tony Luck <tony.luck@intel.com> > +S: Maintained > +F: drivers/platform/x86/intel/ifs > + > INTEL INTEGRATED SENSOR HUB DRIVER > M: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > M: Jiri Kosina <jikos@kernel.org> > diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig > index 1f01a8a23c57..794968bda115 100644 > --- a/drivers/platform/x86/intel/Kconfig > +++ b/drivers/platform/x86/intel/Kconfig > @@ -4,6 +4,7 @@ > # > > source "drivers/platform/x86/intel/atomisp2/Kconfig" > +source "drivers/platform/x86/intel/ifs/Kconfig" > source "drivers/platform/x86/intel/int1092/Kconfig" > source "drivers/platform/x86/intel/int3472/Kconfig" > source "drivers/platform/x86/intel/pmc/Kconfig" > diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile > index c61bc3e97121..10285d0fd16a 100644 > --- a/drivers/platform/x86/intel/Makefile > +++ b/drivers/platform/x86/intel/Makefile > @@ -5,6 +5,7 @@ > # > > obj-$(CONFIG_INTEL_ATOMISP2_PDX86) += atomisp2/ > +obj-y += ifs/ > obj-$(CONFIG_INTEL_SAR_INT1092) += int1092/ > obj-$(CONFIG_INTEL_SKL_INT3472) += int3472/ > obj-$(CONFIG_INTEL_PMC_CORE) += pmc/ > diff --git a/drivers/platform/x86/intel/ifs/Kconfig b/drivers/platform/x86/intel/ifs/Kconfig > new file mode 100644 > index 000000000000..51325b699563 > --- /dev/null > +++ b/drivers/platform/x86/intel/ifs/Kconfig > @@ -0,0 +1,2 @@ > +config INTEL_IFS_DEVICE > + bool > diff --git a/drivers/platform/x86/intel/ifs/Makefile b/drivers/platform/x86/intel/ifs/Makefile > new file mode 100644 > index 000000000000..12c2f5ce9925 > --- /dev/null > +++ b/drivers/platform/x86/intel/ifs/Makefile > @@ -0,0 +1 @@ > +obj-$(CONFIG_INTEL_IFS_DEVICE) += intel_ifs_device.o > diff --git a/drivers/platform/x86/intel/ifs/intel_ifs_device.c b/drivers/platform/x86/intel/ifs/intel_ifs_device.c > new file mode 100644 > index 000000000000..64a143871d72 > --- /dev/null > +++ b/drivers/platform/x86/intel/ifs/intel_ifs_device.c > @@ -0,0 +1,50 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright(c) 2022 Intel Corporation. */ > + > +#include <linux/platform_device.h> > +#include <linux/init.h> > +#include <asm/cpu_device_id.h> > + > +#define MSR_IA32_CORE_CAPS_INTEGRITY_BIT 2 > +#define MSR_IA32_CORE_CAPS_INTEGRITY BIT(MSR_IA32_CORE_CAPS_INTEGRITY_BIT) > + > +#define X86_MATCH(model) \ > + X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6, \ > + INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, NULL) > + > +static const struct x86_cpu_id ifs_cpu_ids[] __initconst = { > + X86_MATCH(SAPPHIRERAPIDS_X), > + {} > +}; > + > +static __init int register_ifs_device(void) > +{ > + struct platform_device *pdev; > + const struct x86_cpu_id *m; > + u64 ia32_core_caps; > + > + m = x86_match_cpu(ifs_cpu_ids); > + if (!m) > + return -ENODEV; > + > + if (rdmsrl_safe(MSR_IA32_CORE_CAPS, &ia32_core_caps)) > + return -ENODEV; > + > + if (ia32_core_caps & MSR_IA32_CORE_CAPS_INTEGRITY) { > + pdev = platform_device_alloc("intel_ifs", 0); > + if (pdev) { > + if (platform_device_add(pdev)) > + platform_device_put(pdev); > + } > + } > + > + /* > + * Failure here will be visible by a missing device > + * in sysfs. Returning an error code would not make > + * that any easier to diagnose. Would also complicate > + * future implementations that may support a subset of > + * the types of tests. > + */ > + return 0; So even if everything fails, you succeed? But you are failing above for some cases, so why is creating the device somehow special here that you should succeed no matter what? thanks, greg k-h
On Tue, Apr 19, 2022 at 9:48 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Tue, Apr 19, 2022 at 09:38:51AM -0700, Tony Luck wrote: > > The initial implementation of IFS is model specific. Enumeration is > > via a combination of family-model-stepping and a check for a bit in the > > CORE_CAPABILITIES MSR. > > > > Linux has handled this lack of enumeration before with a code stub to > > create a device. See arch/x86/kernel/pmem.c. Use the same approach > > here. > > Ick, why? Why not just create a simple virtual device and use that? Do > you really want to bind a driver to this? Or do you already "know" the > only driver that you have will bind to this? With the realization that there may be multiple instances of an IFS-like capability going forward, and that ideally those capabilities would move away from a CPU capability bit to an ACPI description, then it seemed to me that a simulated platform_device for this is a reasonable fit. I.e. when / if an ACPI _HID is assigned for this capability the same platform_driver can be reused for those instances. > pmem.c should not be used as a good example of anything, sorry. Yes, the arch/x86/kernel/pmem.c hack was supplanted by an ACPI device description. There is no ACPI device description for the IFS capability, yet. So I saw these two cases as similar, that capabilities like this need enumeration besides a CPU-id bit or an E820 table entry, and when they move to an enumerable bus like ACPI a platform_driver is expected. > > greg k-h > > > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Tony Luck <tony.luck@intel.com> > > --- > > MAINTAINERS | 7 +++ > > drivers/platform/x86/intel/Kconfig | 1 + > > drivers/platform/x86/intel/Makefile | 1 + > > drivers/platform/x86/intel/ifs/Kconfig | 2 + > > drivers/platform/x86/intel/ifs/Makefile | 1 + > > .../platform/x86/intel/ifs/intel_ifs_device.c | 50 +++++++++++++++++++ > > 6 files changed, 62 insertions(+) > > create mode 100644 drivers/platform/x86/intel/ifs/Kconfig > > create mode 100644 drivers/platform/x86/intel/ifs/Makefile > > create mode 100644 drivers/platform/x86/intel/ifs/intel_ifs_device.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 40fa1955ca3f..9e372a960fa5 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -9861,6 +9861,13 @@ B: https://bugzilla.kernel.org > > T: git git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git > > F: drivers/idle/intel_idle.c > > > > +INTEL IN FIELD SCAN (IFS) DRIVER > > +M: Jithu Joseph <jithu.joseph@intel.com> > > +R: Ashok Raj <ashok.raj@intel.com> > > +R: Tony Luck <tony.luck@intel.com> > > +S: Maintained > > +F: drivers/platform/x86/intel/ifs > > + > > INTEL INTEGRATED SENSOR HUB DRIVER > > M: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > > M: Jiri Kosina <jikos@kernel.org> > > diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig > > index 1f01a8a23c57..794968bda115 100644 > > --- a/drivers/platform/x86/intel/Kconfig > > +++ b/drivers/platform/x86/intel/Kconfig > > @@ -4,6 +4,7 @@ > > # > > > > source "drivers/platform/x86/intel/atomisp2/Kconfig" > > +source "drivers/platform/x86/intel/ifs/Kconfig" > > source "drivers/platform/x86/intel/int1092/Kconfig" > > source "drivers/platform/x86/intel/int3472/Kconfig" > > source "drivers/platform/x86/intel/pmc/Kconfig" > > diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile > > index c61bc3e97121..10285d0fd16a 100644 > > --- a/drivers/platform/x86/intel/Makefile > > +++ b/drivers/platform/x86/intel/Makefile > > @@ -5,6 +5,7 @@ > > # > > > > obj-$(CONFIG_INTEL_ATOMISP2_PDX86) += atomisp2/ > > +obj-y += ifs/ > > obj-$(CONFIG_INTEL_SAR_INT1092) += int1092/ > > obj-$(CONFIG_INTEL_SKL_INT3472) += int3472/ > > obj-$(CONFIG_INTEL_PMC_CORE) += pmc/ > > diff --git a/drivers/platform/x86/intel/ifs/Kconfig b/drivers/platform/x86/intel/ifs/Kconfig > > new file mode 100644 > > index 000000000000..51325b699563 > > --- /dev/null > > +++ b/drivers/platform/x86/intel/ifs/Kconfig > > @@ -0,0 +1,2 @@ > > +config INTEL_IFS_DEVICE > > + bool > > diff --git a/drivers/platform/x86/intel/ifs/Makefile b/drivers/platform/x86/intel/ifs/Makefile > > new file mode 100644 > > index 000000000000..12c2f5ce9925 > > --- /dev/null > > +++ b/drivers/platform/x86/intel/ifs/Makefile > > @@ -0,0 +1 @@ > > +obj-$(CONFIG_INTEL_IFS_DEVICE) += intel_ifs_device.o > > diff --git a/drivers/platform/x86/intel/ifs/intel_ifs_device.c b/drivers/platform/x86/intel/ifs/intel_ifs_device.c > > new file mode 100644 > > index 000000000000..64a143871d72 > > --- /dev/null > > +++ b/drivers/platform/x86/intel/ifs/intel_ifs_device.c > > @@ -0,0 +1,50 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* Copyright(c) 2022 Intel Corporation. */ > > + > > +#include <linux/platform_device.h> > > +#include <linux/init.h> > > +#include <asm/cpu_device_id.h> > > + > > +#define MSR_IA32_CORE_CAPS_INTEGRITY_BIT 2 > > +#define MSR_IA32_CORE_CAPS_INTEGRITY BIT(MSR_IA32_CORE_CAPS_INTEGRITY_BIT) > > + > > +#define X86_MATCH(model) \ > > + X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6, \ > > + INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, NULL) > > + > > +static const struct x86_cpu_id ifs_cpu_ids[] __initconst = { > > + X86_MATCH(SAPPHIRERAPIDS_X), > > + {} > > +}; > > + > > +static __init int register_ifs_device(void) > > +{ > > + struct platform_device *pdev; > > + const struct x86_cpu_id *m; > > + u64 ia32_core_caps; > > + > > + m = x86_match_cpu(ifs_cpu_ids); > > + if (!m) > > + return -ENODEV; > > + > > + if (rdmsrl_safe(MSR_IA32_CORE_CAPS, &ia32_core_caps)) > > + return -ENODEV; > > + > > + if (ia32_core_caps & MSR_IA32_CORE_CAPS_INTEGRITY) { > > + pdev = platform_device_alloc("intel_ifs", 0); > > + if (pdev) { > > + if (platform_device_add(pdev)) > > + platform_device_put(pdev); > > + } > > + } > > + > > + /* > > + * Failure here will be visible by a missing device > > + * in sysfs. Returning an error code would not make > > + * that any easier to diagnose. Would also complicate > > + * future implementations that may support a subset of > > + * the types of tests. > > + */ > > + return 0; > > So even if everything fails, you succeed? But you are failing above for > some cases, so why is creating the device somehow special here that you > should succeed no matter what? My bad, this failure is not fatal to init and test execution tooling will notice the missing device, but yes this can just return the initcall error to be logged.
On Tue, Apr 19, 2022 at 11:09 AM Dan Williams <dan.j.williams@intel.com> wrote: > > On Tue, Apr 19, 2022 at 9:48 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Tue, Apr 19, 2022 at 09:38:51AM -0700, Tony Luck wrote: > > > The initial implementation of IFS is model specific. Enumeration is > > > via a combination of family-model-stepping and a check for a bit in the > > > CORE_CAPABILITIES MSR. > > > > > > Linux has handled this lack of enumeration before with a code stub to > > > create a device. See arch/x86/kernel/pmem.c. Use the same approach > > > here. > > > > Ick, why? Why not just create a simple virtual device and use that? Do > > you really want to bind a driver to this? Or do you already "know" the > > only driver that you have will bind to this? > > With the realization that there may be multiple instances of an > IFS-like capability going forward, and that ideally those capabilities > would move away from a CPU capability bit to an ACPI description, then > it seemed to me that a simulated platform_device for this is a > reasonable fit. I.e. when / if an ACPI _HID is assigned for this > capability the same platform_driver can be reused for those instances. Turns out the ACPI enumeration for this may not materialize, so this can indeed move to a simple / driver-less device.
On Tue, Apr 19, 2022 at 11:09:09AM -0700, Dan Williams wrote: > On Tue, Apr 19, 2022 at 9:48 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Tue, Apr 19, 2022 at 09:38:51AM -0700, Tony Luck wrote: > > > The initial implementation of IFS is model specific. Enumeration is > > > via a combination of family-model-stepping and a check for a bit in the > > > CORE_CAPABILITIES MSR. > > > > > > Linux has handled this lack of enumeration before with a code stub to > > > create a device. See arch/x86/kernel/pmem.c. Use the same approach > > > here. > > > > Ick, why? Why not just create a simple virtual device and use that? Do > > you really want to bind a driver to this? Or do you already "know" the > > only driver that you have will bind to this? > > With the realization that there may be multiple instances of an > IFS-like capability going forward, and that ideally those capabilities > would move away from a CPU capability bit to an ACPI description, then > it seemed to me that a simulated platform_device for this is a > reasonable fit. I.e. when / if an ACPI _HID is assigned for this > capability the same platform_driver can be reused for those instances. Don't write code today for stuff you do not have right now, you all know that. We can always revisit it in the future. thanks, greg k-h
On Tue, Apr 19, 2022 at 03:28:26PM -0700, Dan Williams wrote: > On Tue, Apr 19, 2022 at 11:09 AM Dan Williams <dan.j.williams@intel.com> wrote: > > > > On Tue, Apr 19, 2022 at 9:48 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > On Tue, Apr 19, 2022 at 09:38:51AM -0700, Tony Luck wrote: > > > > The initial implementation of IFS is model specific. Enumeration is > > > > via a combination of family-model-stepping and a check for a bit in the > > > > CORE_CAPABILITIES MSR. > > > > > > > > Linux has handled this lack of enumeration before with a code stub to > > > > create a device. See arch/x86/kernel/pmem.c. Use the same approach > > > > here. > > > > > > Ick, why? Why not just create a simple virtual device and use that? Do > > > you really want to bind a driver to this? Or do you already "know" the > > > only driver that you have will bind to this? > > > > With the realization that there may be multiple instances of an > > IFS-like capability going forward, and that ideally those capabilities > > would move away from a CPU capability bit to an ACPI description, then > > it seemed to me that a simulated platform_device for this is a > > reasonable fit. I.e. when / if an ACPI _HID is assigned for this > > capability the same platform_driver can be reused for those instances. > > Turns out the ACPI enumeration for this may not materialize, so this > can indeed move to a simple / driver-less device. Hey, see, doing extra work now was not a good idea :)
On Wed, Apr 20, 2022 at 09:48:58AM +0200, Greg KH wrote: > Don't write code today for stuff you do not have right now, you all know > that. We can always revisit it in the future. Direction check on the virtual device option. Is this what you are asking for in "core.c"? The second test type is happening internally right away ... so I put in some example code of how it can be added. Upstream submission will just have the one test that exists today. Static definition of: static struct ifs_data ifs_data[IFS_NUMTESTS]; keeps the code simpler (no need to have code to cleanup if dynamic allocation of this small structure fails). But if you feel strongly that all static allocation is bad, then I can kzallloc() per enumerated test type. With this change it is no longer a platform driver. So maybe doesn't belong in drivers/platform/x86/intel/ifs/* Any thoughts on where I should move it to? -Tony ---- core.c --- // SPDX-License-Identifier: GPL-2.0-only /* Copyright(c) 2022 Intel Corporation. */ #include <linux/module.h> #include <linux/device.h> #include <linux/kdev_t.h> #include <linux/semaphore.h> #include <asm/cpu_device_id.h> #include "ifs.h" enum test_types { IFS_SAF, IFS_ANOTHER, IFS_NUMTESTS }; static struct class *ifs_class; static struct ifs_data ifs_data[IFS_NUMTESTS]; #define MSR_IA32_CORE_CAPS_INTEGRITY_BIT 2 #define MSR_IA32_CORE_CAPS_INTEGRITY BIT(MSR_IA32_CORE_CAPS_INTEGRITY_BIT) #define X86_MATCH(model) \ X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6, \ INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, NULL) static const struct x86_cpu_id ifs_cpu_ids[] __initconst = { X86_MATCH(SAPPHIRERAPIDS_X), {} }; MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids); static int ifs_device_unregister(struct device *dev, void *data) { device_unregister(dev); return 0; } static int __init ifs_init(void) { const struct x86_cpu_id *m; u64 ia32_core_caps; struct device *dev; int ndevices = 0; int ret = 0; m = x86_match_cpu(ifs_cpu_ids); if (!m) return -ENODEV; if (rdmsrl_safe(MSR_IA32_CORE_CAPS, &ia32_core_caps)) return -ENODEV; ifs_class = class_create(THIS_MODULE, "intel_ifs"); if (IS_ERR(ifs_class)) return PTR_ERR(ifs_class); ret = ifs_setup_wq(); if (ret) goto class_cleanup; if (ia32_core_caps & MSR_IA32_CORE_CAPS_INTEGRITY) { dev = device_create_with_groups(ifs_class, NULL, MKDEV(0, 0), &ifs_data[IFS_SAF], plat_ifs_groups, "ifs%d", IFS_SAF); if (dev) { ndevices++; down(&ifs_sem); ifs_data[IFS_SAF].loaded = !ifs_load_firmware(dev); up(&ifs_sem); } } if (1) { // placeholder to test 2nd test dev = device_create_with_groups(ifs_class, NULL, MKDEV(0, 0), &ifs_data[IFS_ANOTHER], plat_ifs_groups, "ifs%d", IFS_ANOTHER); if (dev) ndevices++; } if (ndevices) goto done; ret = -ENODEV; class_for_each_device(ifs_class, NULL, NULL, ifs_device_unregister); class_cleanup: class_destroy(ifs_class); done: return ret; } static void __exit ifs_exit(void) { class_for_each_device(ifs_class, NULL, NULL, ifs_device_unregister); class_destroy(ifs_class); ifs_destroy_wq(); } module_init(ifs_init); module_exit(ifs_exit); MODULE_LICENSE("GPL"); MODULE_DESCRIPTION("Intel In Field Scan (IFS) driver");
On Wed, Apr 20, 2022 at 08:27:53AM -0700, Luck, Tony wrote: > On Wed, Apr 20, 2022 at 09:48:58AM +0200, Greg KH wrote: > > Don't write code today for stuff you do not have right now, you all know > > that. We can always revisit it in the future. > > Direction check on the virtual device option. Is this what > you are asking for in "core.c"? > > The second test type is happening internally right away ... so I > put in some example code of how it can be added. Upstream submission > will just have the one test that exists today. > > Static definition of: > > static struct ifs_data ifs_data[IFS_NUMTESTS]; > > keeps the code simpler (no need to have code to > cleanup if dynamic allocation of this small structure > fails). But if you feel strongly that all static allocation > is bad, then I can kzallloc() per enumerated test type. > > With this change it is no longer a platform driver. So maybe > doesn't belong in drivers/platform/x86/intel/ifs/* > > Any thoughts on where I should move it to? > > -Tony > > ---- core.c --- > > // SPDX-License-Identifier: GPL-2.0-only > /* Copyright(c) 2022 Intel Corporation. */ > > #include <linux/module.h> > #include <linux/device.h> > #include <linux/kdev_t.h> > #include <linux/semaphore.h> > > #include <asm/cpu_device_id.h> > > #include "ifs.h" > > enum test_types { > IFS_SAF, > IFS_ANOTHER, > IFS_NUMTESTS > }; > > static struct class *ifs_class; > static struct ifs_data ifs_data[IFS_NUMTESTS]; > > #define MSR_IA32_CORE_CAPS_INTEGRITY_BIT 2 > #define MSR_IA32_CORE_CAPS_INTEGRITY BIT(MSR_IA32_CORE_CAPS_INTEGRITY_BIT) > > #define X86_MATCH(model) \ > X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6, \ > INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, NULL) > > static const struct x86_cpu_id ifs_cpu_ids[] __initconst = { > X86_MATCH(SAPPHIRERAPIDS_X), > {} > }; > MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids); > > static int ifs_device_unregister(struct device *dev, void *data) > { > device_unregister(dev); > > return 0; > } > > static int __init ifs_init(void) > { > const struct x86_cpu_id *m; > u64 ia32_core_caps; > struct device *dev; > int ndevices = 0; > int ret = 0; > > m = x86_match_cpu(ifs_cpu_ids); > if (!m) > return -ENODEV; > > if (rdmsrl_safe(MSR_IA32_CORE_CAPS, &ia32_core_caps)) > return -ENODEV; > > ifs_class = class_create(THIS_MODULE, "intel_ifs"); Why do you need a class? Why not just use a misc device? Saves you loads of boilerplate code that is sometimes tricky to get correct. thanks, greg k-h
>> ifs_class = class_create(THIS_MODULE, "intel_ifs"); > > Why do you need a class? Why not just use a misc device? Saves you > loads of boilerplate code that is sometimes tricky to get correct. It didn't feel like a "ton" of boiler plate. Just class_create()/class_destroy() for the class itself. And class_for_each_device(ifs_class, NULL, NULL, ifs_device_unregister); to clean up devices on exit (or error cleanup in init()). I thought I needed a class to make a directory for my per-test directories to live in: $ ls -l /sys/devices/virtual/intel_ifs total 0 drwxr-xr-x 3 root root 0 Apr 20 13:36 ifs0 drwxr-xr-x 3 root root 0 Apr 20 13:36 ifs1 Can I do that with a misc device? Or is it ok for them all to sit at the top level of /sys/devices/virtual? -Tony
On Wed, Apr 20, 2022 at 05:57:04PM +0000, Luck, Tony wrote: > >> ifs_class = class_create(THIS_MODULE, "intel_ifs"); > > > > Why do you need a class? Why not just use a misc device? Saves you > > loads of boilerplate code that is sometimes tricky to get correct. > > It didn't feel like a "ton" of boiler plate. Just class_create()/class_destroy() > for the class itself. And > > class_for_each_device(ifs_class, NULL, NULL, ifs_device_unregister); > > to clean up devices on exit (or error cleanup in init()). > > > I thought I needed a class to make a directory for my per-test directories to live in: > > $ ls -l /sys/devices/virtual/intel_ifs > total 0 > drwxr-xr-x 3 root root 0 Apr 20 13:36 ifs0 > drwxr-xr-x 3 root root 0 Apr 20 13:36 ifs1 > > Can I do that with a misc device? > > Or is it ok for them all to sit at the top level of /sys/devices/virtual? How many do you have? And why is a directory needed for just one tiny driver type? thanks, greg k-h
>> Or is it ok for them all to sit at the top level of /sys/devices/virtual? > > How many do you have? > > And why is a directory needed for just one tiny driver type? One today. Two tomorrow (internally ... next gen CPU). Three eventually (That's how many are on the roadmap that I can see). -Tony
On Wed, Apr 20, 2022 at 06:08:20PM +0000, Luck, Tony wrote: > >> Or is it ok for them all to sit at the top level of /sys/devices/virtual? > > > > How many do you have? > > > > And why is a directory needed for just one tiny driver type? > > One today. Two tomorrow (internally ... next gen CPU). > > Three eventually (That's how many are on the roadmap that I can see). Then just use a misc device, it's simple and saves you a ton of code.
diff --git a/MAINTAINERS b/MAINTAINERS index 40fa1955ca3f..9e372a960fa5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9861,6 +9861,13 @@ B: https://bugzilla.kernel.org T: git git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git F: drivers/idle/intel_idle.c +INTEL IN FIELD SCAN (IFS) DRIVER +M: Jithu Joseph <jithu.joseph@intel.com> +R: Ashok Raj <ashok.raj@intel.com> +R: Tony Luck <tony.luck@intel.com> +S: Maintained +F: drivers/platform/x86/intel/ifs + INTEL INTEGRATED SENSOR HUB DRIVER M: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> M: Jiri Kosina <jikos@kernel.org> diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig index 1f01a8a23c57..794968bda115 100644 --- a/drivers/platform/x86/intel/Kconfig +++ b/drivers/platform/x86/intel/Kconfig @@ -4,6 +4,7 @@ # source "drivers/platform/x86/intel/atomisp2/Kconfig" +source "drivers/platform/x86/intel/ifs/Kconfig" source "drivers/platform/x86/intel/int1092/Kconfig" source "drivers/platform/x86/intel/int3472/Kconfig" source "drivers/platform/x86/intel/pmc/Kconfig" diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile index c61bc3e97121..10285d0fd16a 100644 --- a/drivers/platform/x86/intel/Makefile +++ b/drivers/platform/x86/intel/Makefile @@ -5,6 +5,7 @@ # obj-$(CONFIG_INTEL_ATOMISP2_PDX86) += atomisp2/ +obj-y += ifs/ obj-$(CONFIG_INTEL_SAR_INT1092) += int1092/ obj-$(CONFIG_INTEL_SKL_INT3472) += int3472/ obj-$(CONFIG_INTEL_PMC_CORE) += pmc/ diff --git a/drivers/platform/x86/intel/ifs/Kconfig b/drivers/platform/x86/intel/ifs/Kconfig new file mode 100644 index 000000000000..51325b699563 --- /dev/null +++ b/drivers/platform/x86/intel/ifs/Kconfig @@ -0,0 +1,2 @@ +config INTEL_IFS_DEVICE + bool diff --git a/drivers/platform/x86/intel/ifs/Makefile b/drivers/platform/x86/intel/ifs/Makefile new file mode 100644 index 000000000000..12c2f5ce9925 --- /dev/null +++ b/drivers/platform/x86/intel/ifs/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_INTEL_IFS_DEVICE) += intel_ifs_device.o diff --git a/drivers/platform/x86/intel/ifs/intel_ifs_device.c b/drivers/platform/x86/intel/ifs/intel_ifs_device.c new file mode 100644 index 000000000000..64a143871d72 --- /dev/null +++ b/drivers/platform/x86/intel/ifs/intel_ifs_device.c @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright(c) 2022 Intel Corporation. */ + +#include <linux/platform_device.h> +#include <linux/init.h> +#include <asm/cpu_device_id.h> + +#define MSR_IA32_CORE_CAPS_INTEGRITY_BIT 2 +#define MSR_IA32_CORE_CAPS_INTEGRITY BIT(MSR_IA32_CORE_CAPS_INTEGRITY_BIT) + +#define X86_MATCH(model) \ + X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6, \ + INTEL_FAM6_##model, X86_FEATURE_CORE_CAPABILITIES, NULL) + +static const struct x86_cpu_id ifs_cpu_ids[] __initconst = { + X86_MATCH(SAPPHIRERAPIDS_X), + {} +}; + +static __init int register_ifs_device(void) +{ + struct platform_device *pdev; + const struct x86_cpu_id *m; + u64 ia32_core_caps; + + m = x86_match_cpu(ifs_cpu_ids); + if (!m) + return -ENODEV; + + if (rdmsrl_safe(MSR_IA32_CORE_CAPS, &ia32_core_caps)) + return -ENODEV; + + if (ia32_core_caps & MSR_IA32_CORE_CAPS_INTEGRITY) { + pdev = platform_device_alloc("intel_ifs", 0); + if (pdev) { + if (platform_device_add(pdev)) + platform_device_put(pdev); + } + } + + /* + * Failure here will be visible by a missing device + * in sysfs. Returning an error code would not make + * that any easier to diagnose. Would also complicate + * future implementations that may support a subset of + * the types of tests. + */ + return 0; +} +device_initcall(register_ifs_device);