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
