RE: [PATCH v2 1/1] platform/x86/intel/pmt: Notify userspace when crashlogs complete

[Ruhl, Michael J]

>-----Original Message-----
>From: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
>Sent: Thursday, October 22, 2026 10:39 PM
>To: ilpo.jarvinen@xxxxxxxxxxxxxxx; david.e.box@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx; Vivi, Rodrigo
><rodrigo.vivi@xxxxxxxxx>; Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>; Siddiqui,
>Ayaz A <ayaz.siddiqui@xxxxxxxxx>; Muqthyar Ahmed, Syed Abdul
><syed.abdul.muqthyar.ahmed@xxxxxxxxx>; intel-xe@xxxxxxxxxxxxxxxxxxxxx;
>hansg@xxxxxxxxxx
>Subject: [PATCH v2 1/1] platform/x86/intel/pmt: Notify userspace when
>crashlogs complete
>
>A crashlog may already be complete when the driver binds, or it may
>complete asynchronously after userspace requests a manual trigger.
>Userspace otherwise has to poll to discover that data is ready.
>
>Emit a KOBJ_CHANGE uevent on the per-instance crashlog device when a
>completed log is found at probe. After a manual trigger, poll the
>completion bit at 100 ms intervals for up to 5 seconds and emit the same
>event when capture completes, or when the poll window expires without
>completion, so userspace is not left waiting. A clear or rearm while
>polling ends the poll without an event. The poll runs on the freezable
>workqueue so it does not read the device during suspend. The event
>carries INTEL_PMT_CRASHLOG_COMPLETE set to 1 or 0 accordingly.
>
>The class device's attributes are created after its KOBJ_ADD, so the
>probe-time event is also the first point at which userspace can reliably
>read a log that was already captured.
>
>Initialize the work items before the device's sysfs attributes are
>exposed, and reject manual triggers until setup of that crashlog
>completes so a trigger cannot queue work against a device torn down by
>a later probe failure. Disable and drain the work items during removal.
>
>Document the new uevent in the sysfs-class-intel_pmt ABI file.
>
>Assisted-by: LLM
>Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
>---
> .../ABI/testing/sysfs-class-intel_pmt         |  32 ++++
> drivers/platform/x86/intel/pmt/crashlog.c     | 138 +++++++++++++++++-
> 2 files changed, 165 insertions(+), 5 deletions(-)
>
>diff --git a/Documentation/ABI/testing/sysfs-class-intel_pmt
>b/Documentation/ABI/testing/sysfs-class-intel_pmt
>index ed4c886a21b1..cf95d1dcd4d6 100644
>--- a/Documentation/ABI/testing/sysfs-class-intel_pmt
>+++ b/Documentation/ABI/testing/sysfs-class-intel_pmt
>@@ -66,6 +66,38 @@ Description:
> 		can be determined from an XML file of specified GUID for the
> 		parent device.
>
>+What:		/sys/class/intel_pmt/crashlog<x>
>+Date:		October 2026
>+KernelVersion:	7.4
>+Contact:	David Box <david.e.box@xxxxxxxxxxxxxxx>
>+Description:
>+		The driver emits a KOBJ_CHANGE uevent for the crashlog<x>
>+		device with the following environment variable:
>+
>+		INTEL_PMT_CRASHLOG_COMPLETE
>+			The crashlog complete status at the end of a check:
>+			1 if set, 0 if not. It reports the status only, not
>+			what caused the crashlog to be captured.
>+
>+		The event is currently sent when a completed crashlog is
>+		found at probe, and after a write of 1 to trigger. It
>+		returns 1 when the crashlog completes, or 0 for trigger if
>+		it has not completed within the poll time. Writing clear
>+		or rearm while polling ends trigger without an event.
>+
>+		The status may have changed by the time the event is
>+		received, and the same crashlog may be reported more than
>+		once. Consumers should read the status attributes before
>+		acting. Consumers that start after the device was probed
>+		should check the complete status of existing devices, for
>+		example when handling a coldplug add event, since an event
>+		emitted before the uevent listener starts is not replayed.
>+
>+		Other KOBJ_CHANGE events may be added in the future with
>+		different variables. Consumers should match on
>+		INTEL_PMT_CRASHLOG_COMPLETE and ignore variables they
>do not
>+		recognize.
>+
> What:		/sys/class/intel_pmt/crashlog<x>/crashlog
> Date:		October 2020
> KernelVersion:	5.10
>diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
>b/drivers/platform/x86/intel/pmt/crashlog.c
>index d7018b495217..2d006abce5bf 100644
>--- a/drivers/platform/x86/intel/pmt/crashlog.c
>+++ b/drivers/platform/x86/intel/pmt/crashlog.c
>@@ -13,13 +13,16 @@
> #include <linux/intel_vsec.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
>+#include <linux/kobject.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/pci.h>
> #include <linux/printk.h>
> #include <linux/slab.h>
>+#include <linux/sysfs.h>
> #include <linux/uaccess.h>
> #include <linux/overflow.h>
>+#include <linux/workqueue.h>
>
> #include "class.h"
>
>@@ -115,6 +118,12 @@ struct crashlog_entry {
> 	struct intel_pmt_entry		entry;
> 	struct mutex			control_mutex;
> 	const struct crashlog_info	*info;
>+	struct work_struct		uevent_work;
>+	struct delayed_work		uevent_poll_work;
>+	u8				uevent_poll_tries;
>+	bool				uevent_polling;
>+	bool				uevent_complete;
>+	bool				trigger_ready;
> };
>
> struct pmt_crashlog_priv {
>@@ -122,6 +131,9 @@ struct pmt_crashlog_priv {
> 	struct crashlog_entry	entry[];
> };
>
>+#define PMT_CRASHLOG_UEVENT_POLL_MS		100
>+#define PMT_CRASHLOG_UEVENT_POLL_MAX_TRIES	50
>+
> /*
>  * I/O
>  */
>@@ -271,6 +283,80 @@ static int pmt_crashlog_set_rearm(struct
>crashlog_entry *crashlog)
> 	return pmt_crashlog_rmw(crashlog, crashlog->info->control.rearm,
>true);
> }
>
>+static void pmt_crashlog_uevent_fn(struct work_struct *work)
>+{
>+	struct crashlog_entry *crashlog =
>+		container_of(work, struct crashlog_entry, uevent_work);
>+	char *envp[] = {
>+		READ_ONCE(crashlog->uevent_complete) ?
>+			"INTEL_PMT_CRASHLOG_COMPLETE=1" :
>+			"INTEL_PMT_CRASHLOG_COMPLETE=0",
>+		NULL,
>+	};
>+
>+	kobject_uevent_env(crashlog->entry.kobj, KOBJ_CHANGE, envp);
>+}
>+
>+static void pmt_crashlog_uevent_poll_fn(struct work_struct *work)
>+{
>+	struct crashlog_entry *crashlog =
>+		container_of(to_delayed_work(work), struct crashlog_entry,
>+			     uevent_poll_work);
>+	bool complete;
>+	int ret;
>+
>+	guard(mutex)(&crashlog->control_mutex);
>+
>+	/* The cycle was ended by a clear or rearm */
>+	if (!crashlog->uevent_polling)
>+		return;
>+
>+	/* A newer trigger_store() re-armed us; that cycle owns the notification
>*/
>+	if (delayed_work_pending(&crashlog->uevent_poll_work))
>+		return;
>+
>+	ret = pmt_crashlog_complete(crashlog, &complete);
>+	if (!ret && complete) {
>+		crashlog->uevent_polling = false;
>+		WRITE_ONCE(crashlog->uevent_complete, true);
>+		schedule_work(&crashlog->uevent_work);
>+		return;
>+	}
>+
>+	if (++crashlog->uevent_poll_tries <
>PMT_CRASHLOG_UEVENT_POLL_MAX_TRIES) {
>+		queue_delayed_work(system_freezable_wq, &crashlog-
>>uevent_poll_work,
>+
>msecs_to_jiffies(PMT_CRASHLOG_UEVENT_POLL_MS));
>+		return;
>+	}
>+
>+	/* Trigger did not complete within the poll window */
>+	crashlog->uevent_polling = false;
>+	WRITE_ONCE(crashlog->uevent_complete, false);
>+	schedule_work(&crashlog->uevent_work);
>+}
>+
>+static void pmt_crashlog_end_poll(struct crashlog_entry *crashlog)
>+{
>+	lockdep_assert_held(&crashlog->control_mutex);
>+
>+	crashlog->uevent_polling = false;
>+	cancel_delayed_work(&crashlog->uevent_poll_work);
>+}
>+
>+static void pmt_crashlog_notify_pending(struct crashlog_entry *crashlog)
>+{
>+	bool complete;
>+	int ret;
>+
>+	lockdep_assert_held(&crashlog->control_mutex);
>+
>+	ret = pmt_crashlog_complete(crashlog, &complete);

This call will use the callback (if available).

Unfortunately, during probe, the callbacks can't be used for the CRI path.

Doing so reveals an attempt to take the mutex(pmt.lock) twice.

We need to revisit this code path.

M


>+	if (!ret && complete) {
>+		WRITE_ONCE(crashlog->uevent_complete, true);
>+		schedule_work(&crashlog->uevent_work);
>+	}
>+}
>+
> /*
>  * sysfs
>  */
>@@ -309,8 +395,12 @@ clear_store(struct device *dev, struct
>device_attribute *attr,
> 	guard(mutex)(&crashlog->control_mutex);
>
> 	ret = pmt_crashlog_set_clear(crashlog);
>+	if (ret)
>+		return ret;
>
>-	return ret ?: count;
>+	pmt_crashlog_end_poll(crashlog);
>+
>+	return count;
> }
> static DEVICE_ATTR_RW(clear);
>
>@@ -454,8 +544,12 @@ rearm_store(struct device *dev, struct
>device_attribute *attr, const char *buf,
> 	guard(mutex)(&crashlog->control_mutex);
>
> 	ret = pmt_crashlog_set_rearm(crashlog);
>+	if (ret)
>+		return ret;
>
>-	return ret ?: count;
>+	pmt_crashlog_end_poll(crashlog);
>+
>+	return count;
> }
> static DEVICE_ATTR_RW(rearm);
>
>@@ -502,9 +596,17 @@ trigger_store(struct device *dev, struct
>device_attribute *attr,
>
> 	if (!trigger) {
> 		ret = pmt_crashlog_set_clear(crashlog);
>-		return ret ?: count;
>+		if (ret)
>+			return ret;
>+
>+		pmt_crashlog_end_poll(crashlog);
>+		return count;
> 	}
>
>+	/* reject triggers until probe has finished setting up this entry */
>+	if (!crashlog->trigger_ready)
>+		return -EBUSY;
>+
> 	/* we cannot trigger a new crash if one is still pending */
> 	ret = pmt_crashlog_complete(crashlog, &complete);
> 	if (ret)
>@@ -513,8 +615,21 @@ trigger_store(struct device *dev, struct
>device_attribute *attr,
> 		return -EEXIST;
>
> 	ret = pmt_crashlog_set_execute(crashlog);
>+	if (ret)
>+		return ret;
>
>-	return ret ?: count;
>+	/*
>+	 * Use non-sync since uevent_poll_fn() takes this same mutex, so
>+	 * cancel_delayed_work_sync() here could deadlock against it.
>+	 */
>+	cancel_delayed_work(&crashlog->uevent_poll_work);
>+
>+	crashlog->uevent_polling = true;
>+	crashlog->uevent_poll_tries = 0;
>+	queue_delayed_work(system_freezable_wq, &crashlog-
>>uevent_poll_work,
>+
>msecs_to_jiffies(PMT_CRASHLOG_UEVENT_POLL_MS));
>+
>+	return count;
> }
> static DEVICE_ATTR_RW(trigger);
>
>@@ -640,6 +755,8 @@ static void pmt_crashlog_remove(struct
>auxiliary_device *auxdev)
> 	for (i = 0; i < priv->num_entries; i++) {
> 		struct crashlog_entry *crashlog = &priv->entry[i];
>
>+		disable_delayed_work_sync(&crashlog->uevent_poll_work);
>+		disable_work_sync(&crashlog->uevent_work);
> 		intel_pmt_dev_destroy(&crashlog->entry, &pmt_crashlog_ns);
> 		mutex_destroy(&crashlog->control_mutex);
> 	}
>@@ -661,7 +778,13 @@ static int pmt_crashlog_probe(struct auxiliary_device
>*auxdev,
> 	auxiliary_set_drvdata(auxdev, priv);
>
> 	for (i = 0; i < intel_vsec_dev->num_resources; i++) {
>-		struct intel_pmt_entry *entry = &priv->entry[priv-
>>num_entries].entry;
>+		struct crashlog_entry *crashlog = &priv->entry[priv-
>>num_entries];
>+		struct intel_pmt_entry *entry = &crashlog->entry;
>+
>+		/* trigger_store() rejects triggers until trigger_ready is set below
>*/
>+		INIT_WORK(&crashlog->uevent_work,
>pmt_crashlog_uevent_fn);
>+		INIT_DELAYED_WORK(&crashlog->uevent_poll_work,
>+				  pmt_crashlog_uevent_poll_fn);
>
> 		ret = intel_pmt_dev_create(entry, &pmt_crashlog_ns,
>intel_vsec_dev, i);
> 		if (ret < 0)
>@@ -670,6 +793,11 @@ static int pmt_crashlog_probe(struct auxiliary_device
>*auxdev,
> 			continue;
>
> 		priv->num_entries++;
>+
>+		scoped_guard(mutex, &crashlog->control_mutex) {
>+			crashlog->trigger_ready = true;
>+			pmt_crashlog_notify_pending(crashlog);
>+		}
> 	}
>
> 	return 0;
>--
>2.43.0