On Tue, Apr 19, 2022 at 10:21 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Apr 19, 2022 at 09:38:57AM -0700, Tony Luck wrote:
> > From: Jithu Joseph <jithu.joseph@xxxxxxxxx>
> >
> > Implement sysfs interface to trigger ifs test for a specific cpu.
> > Additional interfaces related to checking the status of the
> > scan test and seeing the version of the loaded IFS binary
> > are also added.
> >
> > The basic usage is as below.
> > - To start test, for example on cpu5:
> > echo 5 > /sys/devices/platform/intel_ifs/run_test
> > - To see the status of the last test
> > cat /sys/devices/platform/intel_ifs/status
> > - To see the version of the loaded scan binary
> > cat /sys/devices/platform/intel_ifs/image_version
> >
> > Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > Signed-off-by: Jithu Joseph <jithu.joseph@xxxxxxxxx>
> > Co-developed-by: Tony Luck <tony.luck@xxxxxxxxx>
> > Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> > ---
> > drivers/platform/x86/intel/ifs/Makefile | 2 +-
> > drivers/platform/x86/intel/ifs/core.c | 9 ++
> > drivers/platform/x86/intel/ifs/ifs.h | 3 +
> > drivers/platform/x86/intel/ifs/runtest.c | 7 ++
> > drivers/platform/x86/intel/ifs/sysfs.c | 151 +++++++++++++++++++++++
> > 5 files changed, 171 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/platform/x86/intel/ifs/sysfs.c
> >
> > diff --git a/drivers/platform/x86/intel/ifs/Makefile b/drivers/platform/x86/intel/ifs/Makefile
> > index 7de27361b479..dbbe0bf66987 100644
> > --- a/drivers/platform/x86/intel/ifs/Makefile
> > +++ b/drivers/platform/x86/intel/ifs/Makefile
> > @@ -2,4 +2,4 @@ obj-$(CONFIG_INTEL_IFS_DEVICE) += intel_ifs_device.o
> >
> > obj-$(CONFIG_INTEL_IFS) += intel_ifs.o
> >
> > -intel_ifs-objs := core.o load.o runtest.o
> > +intel_ifs-objs := core.o load.o runtest.o sysfs.o
> > diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> > index 0dc4cdda35ff..f56cde0cdfd6 100644
> > --- a/drivers/platform/x86/intel/ifs/core.c
> > +++ b/drivers/platform/x86/intel/ifs/core.c
> > @@ -3,6 +3,7 @@
> >
> > #include <linux/module.h>
> > #include <linux/platform_device.h>
> > +#include <linux/semaphore.h>
> >
> > #include "ifs.h"
> >
> > @@ -26,10 +27,18 @@ static int ifs_probe(struct platform_device *pdev)
> > return 0;
> > }
> >
> > +/*
> > + * Note there is no need for a ->remove() call back. There isn't an
> > + * "unload" operation to remove the scan binary from the BIOS reserved
> > + * area. Also ".dev_groups" removal order will guarantee that any in
> > + * flight tests have completed.
> > + */
>
> So you are ok with the warning the kernel gives you when you unload the
> driver? That feels wrong :(
>
> > +
> > static struct platform_driver intel_ifs_driver = {
> > .probe = ifs_probe,
> > .driver = {
> > .name = "intel_ifs",
> > + .dev_groups = plat_ifs_groups,
> > },
> > };
> >
> > diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> > index f5e3636d709f..4e6662f2d2f8 100644
> > --- a/drivers/platform/x86/intel/ifs/ifs.h
> > +++ b/drivers/platform/x86/intel/ifs/ifs.h
> > @@ -97,4 +97,7 @@ int ifs_setup_wq(void);
> > void ifs_destroy_wq(void);
> > int do_core_test(int cpu, struct device *dev);
> >
> > +extern const struct attribute_group *plat_ifs_groups[];
> > +extern struct semaphore ifs_sem;
> > +
> > #endif
> > diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
> > index 7793a01f7b94..246eff250563 100644
> > --- a/drivers/platform/x86/intel/ifs/runtest.c
> > +++ b/drivers/platform/x86/intel/ifs/runtest.c
> > @@ -10,6 +10,13 @@
> >
> > #include "ifs.h"
> >
> > +/*
> > + * Note all code and data in this file is protected by
> > + * ifs_sem. On HT systems all threads on a core will
> > + * execute together, but only the first thread on the
> > + * core will update results of the test and indicate
> > + * completion.
> > + */
> > static struct workqueue_struct *ifs_wq;
> > static struct completion test_thread_done;
> > static atomic_t siblings_in;
> > diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
> > new file mode 100644
> > index 000000000000..41db2a12fbc8
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel/ifs/sysfs.c
> > @@ -0,0 +1,151 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright(c) 2022 Intel Corporation. */
> > +
> > +#include <linux/cpu.h>
> > +#include <linux/delay.h>
> > +#include <linux/fs.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/semaphore.h>
> > +#include <linux/slab.h>
> > +
> > +#include "ifs.h"
> > +
> > +/*
> > + * Protects against simultaneous tests on multiple cores, or
> > + * reloading can file while a test is in progress
> > + */
> > +DEFINE_SEMAPHORE(ifs_sem);
> > +
> > +/*
> > + * The sysfs interface to check additional details of last test
> > + * cat /sys/devices/system/platform/ifs/details
> > + */
> > +static ssize_t details_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct ifs_data *ifsd = dev_get_drvdata(dev);
> > +
> > + return sysfs_emit(buf, "%#llx\n", ifsd->scan_details);
> > +}
> > +
> > +static DEVICE_ATTR_RO(details);
> > +
> > +static const char * const status_msg[] = {
> > + [SCAN_NOT_TESTED] = "untested",
> > + [SCAN_TEST_PASS] = "pass",
> > + [SCAN_TEST_FAIL] = "fail"
> > +};
> > +
> > +/*
> > + * The sysfs interface to check the test status:
> > + * To check the status of last test
> > + * cat /sys/devices/platform/ifs/status
> > + */
> > +static ssize_t status_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct ifs_data *ifsd = dev_get_drvdata(dev);
> > +
> > + return sysfs_emit(buf, "%s\n", status_msg[ifsd->status]);
> > +}
> > +
> > +static DEVICE_ATTR_RO(status);
> > +
> > +/*
> > + * The sysfs interface for single core testing
> > + * To start test, for example, cpu5
> > + * echo 5 > /sys/devices/platform/ifs/run_test
> > + * To check the result:
> > + * cat /sys/devices/platform/ifs/result
> > + * The sibling core gets tested at the same time.
> > + */
> > +static ssize_t run_test_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct ifs_data *ifsd = dev_get_drvdata(dev);
> > + unsigned int cpu;
> > + int rc;
> > +
> > + rc = kstrtouint(buf, 0, &cpu);
> > + if (rc < 0 || cpu >= nr_cpu_ids)
> > + return -EINVAL;
> > +
> > + if (down_interruptible(&ifs_sem))
> > + return -EINTR;
> > +
> > + if (!ifsd->loaded)
> > + rc = -EPERM;
> > + else
> > + rc = do_core_test(cpu, dev);
> > +
> > + up(&ifs_sem);
> > +
> > + return rc ? rc : count;
> > +}
> > +
> > +static DEVICE_ATTR_WO(run_test);
> > +
> > +/*
> > + * Reload the IFS image. When user wants to install new IFS image
> > + */
> > +static ssize_t reload_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct ifs_data *ifsd = dev_get_drvdata(dev);
> > + int rc;
> > +
> > + if (!sysfs_streq(buf, "1"))
>
> kstrtobool()?
I had asked them to drop kstrtobool() to save a line or 2, because
this is a write-only attribute where "1" is the only valid value.
Otherwise, no worries from me about supporting the other 'true'
values.
>
> > + return -EINVAL;
> > +
> > + if (down_interruptible(&ifs_sem))
> > + return -EINTR;
> > +
> > + rc = load_ifs_binary(dev);
> > +
> > + ifsd->loaded = (rc == 0);
> > +
> > + up(&ifs_sem);
> > +
> > + return rc ? rc : count;
> > +}
> > +
> > +static DEVICE_ATTR_WO(reload);
> > +
> > +/*
> > + * Display currently loaded IFS image version.
> > + */
> > +static ssize_t image_version_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct ifs_data *ifsd = dev_get_drvdata(dev);
> > +
> > + if (!ifsd->loaded)
> > + return sysfs_emit(buf, "%s\n", "none");
> > + else
> > + return sysfs_emit(buf, "%#x\n", ifsd->loaded_version);
> > +}
> > +
> > +static DEVICE_ATTR_RO(image_version);
> > +
> > +/* global scan sysfs attributes */
> > +static struct attribute *plat_ifs_attrs[] = {
> > + &dev_attr_details.attr,
> > + &dev_attr_status.attr,
> > + &dev_attr_run_test.attr,
> > + &dev_attr_reload.attr,
> > + &dev_attr_image_version.attr,
> > + NULL
> > +};
> > +
> > +static const struct attribute_group plat_ifs_attr_group = {
> > + .attrs = plat_ifs_attrs,
> > +};
> > +
> > +const struct attribute_group *plat_ifs_groups[] = {
> > + &plat_ifs_attr_group,
> > + NULL
> > +};
>
> ATTRIBUTE_GROUPS()?
Yeah, I should have caught that.
