RE: [PATCH 3/3] 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 1, 2026 6:04 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 3/3] 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. The event carries two environment
>variables:
>
>  INTEL_PMT_CRASHLOG_EVENT=PRESENT
>  INTEL_PMT_CRASHLOG_COMPLETE=1
>
>Initialize the work items before the device's sysfs attributes are
>exposed, and disable and drain them 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         | 14 ++++
> drivers/platform/x86/intel/pmt/crashlog.c     | 84 ++++++++++++++++++-
> 2 files changed, 95 insertions(+), 3 deletions(-)
>
>diff --git a/Documentation/ABI/testing/sysfs-class-intel_pmt
>b/Documentation/ABI/testing/sysfs-class-intel_pmt
>index ed4c886a21b1..c0b0d123afdf 100644
>--- a/Documentation/ABI/testing/sysfs-class-intel_pmt
>+++ b/Documentation/ABI/testing/sysfs-class-intel_pmt
>@@ -66,6 +66,20 @@ 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:
>+		When a crashlog device is probed with a completed crashlog
>+		already present, or a manual trigger completes, the driver emits
>+		a KOBJ_CHANGE uevent for that crashlog<x> device.
>+
>+		The following uevent environment variables are added:
>+
>+		INTEL_PMT_CRASHLOG_EVENT=PRESENT
>+		INTEL_PMT_CRASHLOG_COMPLETE=1
>+
> 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 f936daf99e4d..21e8e2199bdc 100644
>--- a/drivers/platform/x86/intel/pmt/crashlog.c
>+++ b/drivers/platform/x86/intel/pmt/crashlog.c
>@@ -12,12 +12,14 @@
> #include <linux/cleanup.h>
> #include <linux/intel_vsec.h>
> #include <linux/kernel.h>
>+#include <linux/kobject.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/pci.h>
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> #include <linux/overflow.h>
>+#include <linux/workqueue.h>
>
> #include "class.h"
>
>@@ -113,6 +115,9 @@ 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;
> };
>
> struct pmt_crashlog_priv {
>@@ -120,6 +125,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
>  */
>@@ -227,6 +235,47 @@ static void pmt_crashlog_set_rearm(struct
>crashlog_entry *crashlog)
> 	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[] = {
>+		"INTEL_PMT_CRASHLOG_EVENT=PRESENT",
>+		"INTEL_PMT_CRASHLOG_COMPLETE=1",
>+		NULL,
>+	};
>+
>+	if (crashlog->entry.kobj)
>+		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);
>+	guard(mutex)(&crashlog->control_mutex);
>+
>+	/* A newer trigger_store() re-armed us; that cycle owns the notification
>*/
>+	if (delayed_work_pending(&crashlog->uevent_poll_work))
>+		return;
>+
>+	if (pmt_crashlog_complete(crashlog)) {
>+		schedule_work(&crashlog->uevent_work);
>+		return;
>+	}
>+
>+	if (++crashlog->uevent_poll_tries <
>PMT_CRASHLOG_UEVENT_POLL_MAX_TRIES)
>+		schedule_delayed_work(&crashlog->uevent_poll_work,
>+
>msecs_to_jiffies(PMT_CRASHLOG_UEVENT_POLL_MS));
>+}
>+
>+static void pmt_crashlog_notify_pending(struct crashlog_entry *crashlog)
>+{
>+	if (pmt_crashlog_complete(crashlog))
>+		schedule_work(&crashlog->uevent_work);

This looks good to me.  The polling loop on the manual trigger is a very nice to have.

Note: my patch set for CRI support changes most of the predicate functions to return 0 on success
and the state in a parameter...

Not sure on how this should be handled?

Otherwise:

Reviewed-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx>

M

>+}
>+
> /*
>  * sysfs
>  */
>@@ -423,7 +472,19 @@ trigger_store(struct device *dev, struct
>device_attribute *attr,
> 	if (pmt_crashlog_complete(crashlog))
> 		return -EEXIST;
>
>+	/*
>+	 * Only now are we actually starting a fresh crash: any stale poll
>+	 * cycle left over from a prior trigger can be dropped safely, since
>+	 * pmt_crashlog_complete() above proved it carried no unresolved
>+	 * completion. Non-sync: uevent_poll_fn() takes this same mutex, so
>+	 * cancel_delayed_work_sync() here could deadlock against it.
>+	 */
>+	cancel_delayed_work(&crashlog->uevent_poll_work);
>+
> 	pmt_crashlog_set_execute(crashlog);
>+	crashlog->uevent_poll_tries = 0;
>+	schedule_delayed_work(&crashlog->uevent_poll_work,
>+
>msecs_to_jiffies(PMT_CRASHLOG_UEVENT_POLL_MS));
>
> 	return count;
> }
>@@ -551,6 +612,9 @@ 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, not cancel: a racing trigger_store() must not re-arm
>*/
>+		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);
> 	}
>@@ -572,15 +636,29 @@ 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;
>+
>+		/* init before dev_create() exposes trigger sysfs to userspace */
>+		INIT_WORK(&crashlog->uevent_work,
>pmt_crashlog_uevent_fn);
>+		INIT_DELAYED_WORK(&crashlog->uevent_poll_work,
>+				  pmt_crashlog_uevent_poll_fn);
>+		crashlog->uevent_poll_tries = 0;
>
> 		ret = intel_pmt_dev_create(entry, &pmt_crashlog_ns,
>intel_vsec_dev, i);
>-		if (ret < 0)
>+		if (ret < 0) {
>+			cancel_delayed_work_sync(&crashlog-
>>uevent_poll_work);
>+			cancel_work_sync(&crashlog->uevent_work);
> 			goto abort_probe;
>-		if (ret)
>+		}
>+		if (ret) {
>+			cancel_delayed_work_sync(&crashlog-
>>uevent_poll_work);
>+			cancel_work_sync(&crashlog->uevent_work);
> 			continue;
>+		}
>
> 		priv->num_entries++;
>+		pmt_crashlog_notify_pending(crashlog);
> 	}
>
> 	return 0;
>--
>2.43.0