On Thu, Apr 07, 2022 at 12:13:45PM -0700, Jithu Joseph wrote:
> 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: Tony Luck <tony.luck@xxxxxxxxx>
> Signed-off-by: Jithu Joseph <jithu.joseph@xxxxxxxxx>
> ---
> drivers/platform/x86/intel/ifs/Makefile | 2 +-
> drivers/platform/x86/intel/ifs/core.c | 1 +
> drivers/platform/x86/intel/ifs/ifs.h | 2 +
> drivers/platform/x86/intel/ifs/sysfs.c | 139 ++++++++++++++++++++++++
> 4 files changed, 143 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 d5905e5d2de8..93745fcdf652 100644
> --- a/drivers/platform/x86/intel/ifs/Makefile
> +++ b/drivers/platform/x86/intel/ifs/Makefile
> @@ -1,3 +1,3 @@
> 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 95847e00038b..85442953d0f5 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -82,6 +82,7 @@ static int __init ifs_init(void)
> goto drv_unreg;
> }
>
> + ifs_sysfs_add();
> ret = platform_device_add(ifs_pdev);
> if (ret) {
> pr_err("intel_ifs: platform device add failed\n");
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index 93cc1af4aea0..3200d9de4436 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -127,6 +127,8 @@ struct ifs_test {
>
> int load_ifs_binary(void);
> int do_core_test(int cpu);
> +void ifs_sysfs_add(void);
> +
> extern struct platform_device *ifs_pdev;
> extern struct ifs_binary ifs_binary;
> extern struct ifs_test ifs_test;
> diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
> new file mode 100644
> index 000000000000..f6decebbeae9
> --- /dev/null
> +++ b/drivers/platform/x86/intel/ifs/sysfs.c
> @@ -0,0 +1,139 @@
> +// 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"
> +
> +static 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)
> +{
> + int ret;
> +
> + if (down_trylock(&ifs_sem))
> + return -EBUSY;
Why do you care about locking here at all?
> +
> + ret = sysfs_emit(buf, "%#llx\n", ifs_test.scan_details);
> + up(&ifs_sem);
What are you protecting? The value can change right after the lock is
released, so who cares?
> +
> + return ret;
> +}
> +
> +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)
> +{
> + int ret;
> +
> + if (down_trylock(&ifs_sem))
> + return -EBUSY;
> +
> + ret = sysfs_emit(buf, "%s\n", status_msg[ifs_test.status]);
> +
> + up(&ifs_sem);
> +
> + return ret;
> +}
> +
> +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)
> +{
> + unsigned int cpu;
> + int ret = count;
> + int rc;
> +
> + if (!ifs_binary.loaded) {
> + dev_info(&ifs_pdev->dev, "Load scan binary using driver bind interface\n");
Do not allow userspace to spam kernel logs for no reason :(
sysfs files are not "help files" in the kernel.
> + return -EPERM;
> + }
> +
> + if (ifs_disabled)
> + return -ENXIO;
> +
> + rc = kstrtouint(buf, 0, &cpu);
> + if (rc < 0 || cpu >= nr_cpu_ids)
> + return -EINVAL;
> +
> + if (down_trylock(&ifs_sem))
> + return -EBUSY;
> +
> + rc = do_core_test(cpu);
> + if (rc)
> + ret = rc;
> +
> + up(&ifs_sem);
> +
> + return ret;
> +}
> +
> +static DEVICE_ATTR_WO(run_test);
> +
> +/*
> + * Currently loaded IFS image version.
> + */
> +static ssize_t image_version_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sysfs_emit(buf, "%#x\n", ifs_binary.loaded_version);
> +}
> +
> +static DEVICE_ATTR_RO(image_version);
> +
> +/* global scan sysfs attributes */
> +static struct attribute *plat_ifs_attrs[] = {
> + &dev_attr_image_version.attr,
> + &dev_attr_run_test.attr,
> + &dev_attr_status.attr,
> + &dev_attr_details.attr,
> + NULL
> +};
> +
> +static const struct attribute_group plat_ifs_attr_group = {
> + .attrs = plat_ifs_attrs,
> +};
> +
> +static const struct attribute_group *plat_ifs_groups[] = {
> + &plat_ifs_attr_group,
> + NULL
> +};
> +
> +void ifs_sysfs_add(void)
> +{
> + ifs_pdev->dev.groups = plat_ifs_groups;
Why do you have a single global structure?
{sigh}
