On Tue, Apr 19, 2022 at 09:38:57AM -0700, Tony Luck wrote:
> From: Jithu Joseph <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
> Co-developed-by: Tony Luck <[email protected]>
> Signed-off-by: Tony Luck <[email protected]>
> ---
> 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()?
> + 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()?
thanks,
greg k-h
On Tue, Apr 19, 2022 at 10:21 AM Greg KH <[email protected]> wrote:
>
> On Tue, Apr 19, 2022 at 09:38:57AM -0700, Tony Luck wrote:
> > From: Jithu Joseph <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Jithu Joseph <[email protected]>
> > Co-developed-by: Tony Luck <[email protected]>
> > Signed-off-by: Tony Luck <[email protected]>
> > ---
> > 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.
On Tue, Apr 19, 2022 at 07:20:54PM +0200, Greg KH wrote:
> > +/*
> > + * 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 :(
What warning?
# dmesg | tail -5
[ 38.084165] virbr0: port 1(virbr0-nic) entered listening state
[ 38.149621] virbr0: port 1(virbr0-nic) entered disabled state
[ 38.582054] broken atomic modeset userspace detected, disabling atomic
[ 43.703321] igc 0000:01:00.0 enp1s0: NIC Link is Up 2500 Mbps Full Duplex, Flow Control: RX
[ 43.703470] IPv6: ADDRCONF(NETDEV_CHANGE): enp1s0: link becomes ready
# modprobe intel_ifs
# ls /sys/devices/platform/intel_ifs.0/
details driver_override modalias reload status uevent
driver image_version power run_test subsystem
# rmmod intel_ifs
# dmesg | tail -5
[ 38.084165] virbr0: port 1(virbr0-nic) entered listening state
[ 38.149621] virbr0: port 1(virbr0-nic) entered disabled state
[ 38.582054] broken atomic modeset userspace detected, disabling atomic
[ 43.703321] igc 0000:01:00.0 enp1s0: NIC Link is Up 2500 Mbps Full Duplex, Flow Control: RX
[ 43.703470] IPv6: ADDRCONF(NETDEV_CHANGE): enp1s0: link becomes ready
#
-Tony