| Message ID | 20220419163859.2228874-10-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:57AM -0700, Tony Luck wrote: > From: Jithu Joseph <jithu.joseph@intel.com> > > 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@intel.com> > Signed-off-by: Jithu Joseph <jithu.joseph@intel.com> > Co-developed-by: Tony Luck <tony.luck@intel.com> > Signed-off-by: Tony Luck <tony.luck@intel.com> > --- > 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 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
On Tue, Apr 19, 2022 at 10:35:17AM -0700, Luck, Tony wrote: > 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 > # If there's no warning when the device goes away, why the crazy comment trying to justify the lack of a remove callback? confused, greg k-h
On Tue, Apr 19, 2022 at 11:01 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Tue, Apr 19, 2022 at 10:35:17AM -0700, Luck, Tony wrote: > > 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 > > # > > If there's no warning when the device goes away, why the crazy comment > trying to justify the lack of a remove callback? The comment clarifies the nuance that driver.dev_groups coordinates flushing access sysfs ops users before ->remove() is called. It can certainly be dropped.
On Tue, Apr 19, 2022 at 10:21 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Tue, Apr 19, 2022 at 09:38:57AM -0700, Tony Luck wrote: > > From: Jithu Joseph <jithu.joseph@intel.com> > > > > 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@intel.com> > > Signed-off-by: Jithu Joseph <jithu.joseph@intel.com> > > Co-developed-by: Tony Luck <tony.luck@intel.com> > > Signed-off-by: Tony Luck <tony.luck@intel.com> > > --- > > 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.
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. + */ + 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")) + 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 +};