[PATCH 0/3] intel_pmt: Add Alder Lake and OOB-MSM support

[PATCH 0/3] intel_pmt: Add Alder Lake and OOB-MSM support

[David E. Box]

Adds Intel Platform Monitoring Technology (PMT) PCI ids for Alder Lake and
the Out Of Band Management Services Module used by Sapphire Rapdis and
other platforms.

This patchset applies on top of the patchset linked below which was
accepted and awaiting merge in mfd next.

Link: https://lore.kernel.org/lkml/20200819180255.11770-1-david.e.box@linux.intel.com/

Alexander Duyck (1):
  platform/x86: Intel PMT Crashlog capability driver

David E. Box (2):
  mfd: intel_pmt: Add OOBMSM device ID
  mfd: intel_pmt: Add Alder Lake (ADL) support

 .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
 drivers/mfd/intel_pmt.c                       |   6 +
 drivers/platform/x86/Kconfig                  |  10 +
 drivers/platform/x86/Makefile                 |   1 +
 drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
 5 files changed, 671 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
 create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c

-- 
2.20.1

[PATCH 1/3] mfd: intel_pmt: Add OOBMSM device ID

[David E. Box]

Add Out of Band Management Services Module device ID to Intel PMT driver.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/mfd/intel_pmt.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mfd/intel_pmt.c b/drivers/mfd/intel_pmt.c
index 0e572b105101..8f9970ab3026 100644
--- a/drivers/mfd/intel_pmt.c
+++ b/drivers/mfd/intel_pmt.c
@@ -55,6 +55,8 @@ struct pmt_platform_info {
 	unsigned long quirks;
 };
 
+static const struct pmt_platform_info pmt_info;
+
 static const struct pmt_platform_info tgl_info = {
 	.quirks = PMT_QUIRK_NO_WATCHER | PMT_QUIRK_NO_CRASHLOG |
 		  PMT_QUIRK_TABLE_SHIFT,
@@ -200,8 +202,10 @@ static void pmt_pci_remove(struct pci_dev *pdev)
 	pm_runtime_get_sync(&pdev->dev);
 }
 
+#define PCI_DEVICE_ID_INTEL_PMT_OOBMSM	0x09a7
 #define PCI_DEVICE_ID_INTEL_PMT_TGL	0x9a0d
 static const struct pci_device_id pmt_pci_ids[] = {
+	{ PCI_DEVICE_DATA(INTEL, PMT_OOBMSM, &pmt_info) },
 	{ PCI_DEVICE_DATA(INTEL, PMT_TGL, &tgl_info) },
 	{ }
 };
-- 
2.20.1

[PATCH 2/3] mfd: intel_pmt: Add Alder Lake (ADL) support

[David E. Box]

Add PMT support for Alder Lake (ADL). Use same quirks as Tiger Lake since
the design is the same, meaning no support for Watcher or Crashlog.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/mfd/intel_pmt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mfd/intel_pmt.c b/drivers/mfd/intel_pmt.c
index 8f9970ab3026..1b57a970a9d7 100644
--- a/drivers/mfd/intel_pmt.c
+++ b/drivers/mfd/intel_pmt.c
@@ -202,9 +202,11 @@ static void pmt_pci_remove(struct pci_dev *pdev)
 	pm_runtime_get_sync(&pdev->dev);
 }
 
+#define PCI_DEVICE_ID_INTEL_PMT_ADL	0x467d
 #define PCI_DEVICE_ID_INTEL_PMT_OOBMSM	0x09a7
 #define PCI_DEVICE_ID_INTEL_PMT_TGL	0x9a0d
 static const struct pci_device_id pmt_pci_ids[] = {
+	{ PCI_DEVICE_DATA(INTEL, PMT_ADL, &tgl_info) },
 	{ PCI_DEVICE_DATA(INTEL, PMT_OOBMSM, &pmt_info) },
 	{ PCI_DEVICE_DATA(INTEL, PMT_TGL, &tgl_info) },
 	{ }
-- 
2.20.1

[PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

[David E. Box]

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Add support for the Intel Platform Monitoring Technology crashlog
interface.  This interface provides a few sysfs values to allow for
controlling the crashlog telemetry interface as well as a character driver
to allow for mapping the crashlog memory region so that it can be accessed
after a crashlog has been recorded.

This driver is meant to only support the server version of the crashlog
which is identified as crash_type 1 with a version of zero. Currently no
other types are supported.

Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
 drivers/platform/x86/Kconfig                  |  10 +
 drivers/platform/x86/Makefile                 |   1 +
 drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
 4 files changed, 665 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
 create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c

diff --git a/Documentation/ABI/testing/sysfs-class-pmt_crashlog b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
new file mode 100644
index 000000000000..40fb4ff437a6
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
@@ -0,0 +1,66 @@
+What:		/sys/class/pmt_crashlog/
+Date:		September 2020
+KernelVersion:	5.10
+Contact:	Alexander Duyck <alexander.h.duyck@linux.intel.com>
+Description:
+		The pmt_crashlog/ class directory contains information
+		for devices that expose crashlog capabilities using the Intel
+		Platform Monitoring Technology (PTM).
+
+What:		/sys/class/pmt_crashlog/crashlogX
+Date:		September 2020
+KernelVersion:	5.10
+Contact:	Alexander Duyck <alexander.h.duyck@linux.intel.com>
+Description:
+		The crashlogX directory contains files for configuring an
+		instance of a PMT crashlog device that can perform crash data
+		recoring. Each crashlogX device has an associated
+		/dev/crashlogX device node. This node can be opened and mapped
+		to access the resulting crashlog data. The register layout for
+		the log can be determined from an XML file of specified guid
+		for the parent device.
+
+What:		/sys/class/pmt_crashlog/crashlogX/guid
+Date:		September 2020
+KernelVersion:	5.10
+Contact:	Alexander Duyck <alexander.h.duyck@linux.intel.com>
+Description:
+		(RO) The guid for this crashlog device. The guid identifies the
+		version of the XML file for the parent device that should be
+		used to determine the register layout.
+
+What:		/sys/class/pmt_crashlog/crashlogX/size
+Date:		September 2020
+KernelVersion:	5.10
+Contact:	Alexander Duyck <alexander.h.duyck@linux.intel.com>
+Description:
+		(RO) The length of the result buffer in bytes that corresponds
+		to the mapping size for the /dev/crashlogX device node.
+
+What:		/sys/class/pmt_crashlog/crashlogX/offset
+Date:		September 2020
+KernelVersion:	5.10
+Contact:	Alexander Duyck <alexander.h.duyck@linux.intel.com>
+Description:
+		(RO) The offset of the buffer in bytes that corresponds
+		to the mapping for the /dev/crashlogX device node.
+
+What:		/sys/class/pmt_crashlog/crashlogX/enable
+Date:		September 2020
+KernelVersion:	5.10
+Contact:	Alexander Duyck <alexander.h.duyck@linux.intel.com>
+Description:
+		(RW) Boolean value controlling if the crashlog functionality
+		is enabled for the /dev/crashlogX device node.
+
+What:		/sys/class/pmt_crashlog/crashlogX/trigger
+Date:		September 2020
+KernelVersion:	5.10
+Contact:	Alexander Duyck <alexander.h.duyck@linux.intel.com>
+Description:
+		(RW) Boolean value controlling  the triggering of the
+		/dev/crashlogX device node. When read it provides data on if
+		the crashlog has been triggered. When written to it can be
+		used to either clear the current trigger by writing false, or
+		to trigger a new event if the trigger is not currently set.
+
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 48335b02014f..50c3234e4f72 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1360,6 +1360,16 @@ config INTEL_PMC_CORE
 		- LTR Ignore
 		- MPHY/PLL gating status (Sunrisepoint PCH only)
 
+config INTEL_PMT_CRASHLOG
+	tristate "Intel Platform Monitoring Technology (PMT) Crashlog driver"
+	help
+	 The Intel Platform Monitoring Technology (PMT) crashlog driver provides
+	 access to hardware crashlog capabilities on devices that support the
+	 feature.
+
+	 For more information, see
+	 <file:Documentation/ABI/testing/sysfs-class-intel_pmt_crashlog>
+
 config INTEL_PMT_TELEMETRY
 	tristate "Intel Platform Monitoring Technology (PMT) Telemetry driver"
 	help
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index ca942e70de8d..1b8b2502d460 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -140,6 +140,7 @@ obj-$(CONFIG_INTEL_MFLD_THERMAL)	+= intel_mid_thermal.o
 obj-$(CONFIG_INTEL_MID_POWER_BUTTON)	+= intel_mid_powerbtn.o
 obj-$(CONFIG_INTEL_MRFLD_PWRBTN)	+= intel_mrfld_pwrbtn.o
 obj-$(CONFIG_INTEL_PMC_CORE)		+= intel_pmc_core.o intel_pmc_core_pltdrv.o
+obj-$(CONFIG_INTEL_PMT_CRASHLOG)	+= intel_pmt_crashlog.o
 obj-$(CONFIG_INTEL_PMT_TELEMETRY)	+= intel_pmt_telemetry.o
 obj-$(CONFIG_INTEL_PUNIT_IPC)		+= intel_punit_ipc.o
 obj-$(CONFIG_INTEL_SCU_IPC)		+= intel_scu_ipc.o
diff --git a/drivers/platform/x86/intel_pmt_crashlog.c b/drivers/platform/x86/intel_pmt_crashlog.c
new file mode 100644
index 000000000000..31d43708055c
--- /dev/null
+++ b/drivers/platform/x86/intel_pmt_crashlog.c
@@ -0,0 +1,588 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Platform Monitoring Technology Crashlog driver
+ *
+ * Copyright (c) 2020, Intel Corporation.
+ * All Rights Reserved.
+ *
+ * Authors: "Alexander Duyck" <alexander.h.duyck@linux.intel.com>
+ */
+
+#include <linux/cdev.h>
+#include <linux/idr.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#define DRV_NAME		"pmt_crashlog"
+
+/* Crashlog access types */
+#define ACCESS_FUTURE		1
+#define ACCESS_BARID		2
+#define ACCESS_LOCAL		3
+
+/* Crashlog discovery header types */
+#define CRASH_TYPE_OOBMSM	1
+
+/* Control Flags */
+#define CRASHLOG_FLAG_DISABLE	BIT(27)
+#define CRASHLOG_FLAG_CLEAR	BIT(28)
+#define CRASHLOG_FLAG_EXECUTE	BIT(29)
+#define CRASHLOG_FLAG_COMPLETE	BIT(31)
+#define CRASHLOG_FLAG_MASK	GENMASK(31, 28)
+
+/* Common Header */
+#define CONTROL_OFFSET		0x0
+#define GUID_OFFSET		0x4
+#define BASE_OFFSET		0x8
+#define SIZE_OFFSET		0xC
+#define GET_ACCESS(v)		((v) & GENMASK(3, 0))
+#define GET_TYPE(v)		(((v) & GENMASK(7, 4)) >> 4)
+#define GET_VERSION(v)		(((v) & GENMASK(19, 16)) >> 16)
+
+#define GET_ADDRESS(v)		((v) & GENMASK(31, 3))
+#define GET_BIR(v)		((v) & GENMASK(2, 0))
+
+static DEFINE_IDA(crashlog_devid_ida);
+
+struct crashlog_header {
+	u32	base_offset;
+	u32	size;
+	u32	guid;
+	u8	bir;
+	u8	access_type;
+	u8	crash_type;
+	u8	version;
+};
+
+struct pmt_crashlog_priv;
+
+struct crashlog_entry {
+	struct pmt_crashlog_priv	*priv;
+	struct crashlog_header		header;
+	struct resource			*header_res;
+	void __iomem			*disc_table;
+	unsigned long			crashlog_data;
+	size_t				crashlog_data_size;
+	struct cdev			cdev;
+	dev_t				devt;
+	int				devid;
+	struct ida			*ida;
+};
+
+struct pmt_crashlog_priv {
+	struct device		*dev;
+	struct pci_dev		*parent;
+	struct crashlog_entry	*entry;
+	int			num_entries;
+};
+
+/*
+ * I/O
+ */
+static bool pmt_crashlog_complete(struct crashlog_entry *entry)
+{
+	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
+
+	/* return current value of the crashlog complete flag */
+	return !!(control & CRASHLOG_FLAG_COMPLETE);
+}
+
+static bool pmt_crashlog_disabled(struct crashlog_entry *entry)
+{
+	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
+
+	/* return current value of the crashlog disabled flag */
+	return !!(control & CRASHLOG_FLAG_DISABLE);
+}
+
+static void pmt_crashlog_set_disable(struct crashlog_entry *entry, bool disable)
+{
+	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
+
+	/* clear control bits */
+	control &= ~(CRASHLOG_FLAG_MASK | CRASHLOG_FLAG_DISABLE);
+	if (disable)
+		control |= CRASHLOG_FLAG_DISABLE;
+
+	writel(control, entry->disc_table + CONTROL_OFFSET);
+}
+
+static void pmt_crashlog_set_clear(struct crashlog_entry *entry)
+{
+	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
+
+	/* clear control bits */
+	control &= ~CRASHLOG_FLAG_MASK;
+	control |= CRASHLOG_FLAG_CLEAR;
+
+	writel(control, entry->disc_table + CONTROL_OFFSET);
+}
+
+static void pmt_crashlog_set_execute(struct crashlog_entry *entry)
+{
+	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
+
+	/* clear control bits */
+	control &= ~CRASHLOG_FLAG_MASK;
+	control |= CRASHLOG_FLAG_EXECUTE;
+
+	writel(control, entry->disc_table + CONTROL_OFFSET);
+}
+
+/*
+ * devfs
+ */
+static int pmt_crashlog_open(struct inode *inode, struct file *filp)
+{
+	struct crashlog_entry *entry;
+	struct pci_driver *pci_drv;
+	struct pmt_crashlog_priv *priv;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	entry = container_of(inode->i_cdev, struct crashlog_entry, cdev);
+	priv = entry->priv;
+	pci_drv = pci_dev_driver(priv->parent);
+
+	if (!pci_drv)
+		return -ENODEV;
+
+	filp->private_data = entry;
+	get_device(&priv->parent->dev);
+
+	if (!try_module_get(pci_drv->driver.owner)) {
+		put_device(&priv->parent->dev);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int pmt_crashlog_release(struct inode *inode, struct file *filp)
+{
+	struct crashlog_entry *entry = filp->private_data;
+	struct pmt_crashlog_priv *priv;
+	struct pci_driver *pci_drv;
+
+	priv = entry->priv;
+	pci_drv = pci_dev_driver(priv->parent);
+
+	put_device(&priv->parent->dev);
+	module_put(pci_drv->driver.owner);
+
+	return 0;
+}
+
+static int
+pmt_crashlog_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+	struct crashlog_entry *entry = filp->private_data;
+	struct pmt_crashlog_priv *priv;
+	unsigned long phys = entry->crashlog_data;
+	unsigned long pfn = PFN_DOWN(phys);
+	unsigned long vsize = vma->vm_end - vma->vm_start;
+	unsigned long psize;
+
+	if ((vma->vm_flags & VM_WRITE) ||
+	    (vma->vm_flags & VM_MAYWRITE))
+		return -EPERM;
+
+	priv = entry->priv;
+
+	if (!entry->crashlog_data_size) {
+		dev_err(priv->dev, "Crashlog data not accessible\n");
+		return -EAGAIN;
+	}
+
+	psize = (PFN_UP(entry->crashlog_data + entry->crashlog_data_size) - pfn) *
+		PAGE_SIZE;
+	if (vsize > psize) {
+		dev_err(priv->dev, "Requested mmap size is too large\n");
+		return -EINVAL;
+	}
+
+	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+	if (io_remap_pfn_range(vma, vma->vm_start, pfn,
+		vsize, vma->vm_page_prot))
+		return -EAGAIN;
+
+	return 0;
+}
+
+static const struct file_operations pmt_crashlog_fops = {
+	.owner =	THIS_MODULE,
+	.open =		pmt_crashlog_open,
+	.mmap =		pmt_crashlog_mmap,
+	.release =	pmt_crashlog_release,
+};
+
+/*
+ * sysfs
+ */
+static ssize_t
+guid_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct crashlog_entry *entry;
+
+	entry = dev_get_drvdata(dev);
+
+	return sprintf(buf, "0x%x\n", entry->header.guid);
+}
+static DEVICE_ATTR_RO(guid);
+
+static ssize_t size_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct crashlog_entry *entry;
+
+	entry = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%lu\n", entry->crashlog_data_size);
+}
+static DEVICE_ATTR_RO(size);
+
+static ssize_t
+offset_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct crashlog_entry *entry;
+
+	entry = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%lu\n", offset_in_page(entry->crashlog_data));
+}
+static DEVICE_ATTR_RO(offset);
+
+static ssize_t
+enable_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct crashlog_entry *entry;
+	int enabled;
+
+	entry = dev_get_drvdata(dev);
+	enabled = !pmt_crashlog_disabled(entry);
+
+	return sprintf(buf, "%d\n", enabled);
+}
+
+static ssize_t
+enable_store(struct device *dev, struct device_attribute *attr,
+	    const char *buf, size_t count)
+{
+	struct crashlog_entry *entry;
+	bool enabled;
+	int result;
+
+	entry = dev_get_drvdata(dev);
+
+	result = kstrtobool(buf, &enabled);
+	if (result)
+		return result;
+
+	pmt_crashlog_set_disable(entry, !enabled);
+
+	return strnlen(buf, count);
+}
+static DEVICE_ATTR_RW(enable);
+
+static ssize_t
+trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct crashlog_entry *entry;
+	int trigger;
+
+	entry = dev_get_drvdata(dev);
+	trigger = pmt_crashlog_complete(entry);
+
+	return sprintf(buf, "%d\n", trigger);
+}
+
+static ssize_t
+trigger_store(struct device *dev, struct device_attribute *attr,
+	    const char *buf, size_t count)
+{
+	struct crashlog_entry *entry;
+	bool trigger;
+	int result;
+
+	entry = dev_get_drvdata(dev);
+
+	result = kstrtobool(buf, &trigger);
+	if (result)
+		return result;
+
+	if (trigger) {
+		/* we cannot trigger a new crash if one is still pending */
+		if (pmt_crashlog_complete(entry))
+			return -EEXIST;
+
+		/* if device is currently disabled, return busy */
+		if (pmt_crashlog_disabled(entry))
+			return -EBUSY;
+
+		pmt_crashlog_set_execute(entry);
+	} else {
+		pmt_crashlog_set_clear(entry);
+	}
+
+	return strnlen(buf, count);
+}
+static DEVICE_ATTR_RW(trigger);
+
+static struct attribute *pmt_crashlog_attrs[] = {
+	&dev_attr_guid.attr,
+	&dev_attr_size.attr,
+	&dev_attr_offset.attr,
+	&dev_attr_enable.attr,
+	&dev_attr_trigger.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(pmt_crashlog);
+
+static struct class pmt_crashlog_class = {
+	.name = "pmt_crashlog",
+	.owner = THIS_MODULE,
+	.dev_groups = pmt_crashlog_groups,
+};
+
+/*
+ * initialization
+ */
+static int pmt_crashlog_make_dev(struct pmt_crashlog_priv *priv,
+				 struct crashlog_entry *entry)
+{
+	struct device *dev;
+	int ret;
+
+	ret = alloc_chrdev_region(&entry->devt, 0, 1, DRV_NAME);
+	if (ret < 0) {
+		dev_err(priv->dev, "alloc_chrdev_region err: %d\n", ret);
+		return ret;
+	}
+
+	/* Create a character device for Samplers */
+	cdev_init(&entry->cdev, &pmt_crashlog_fops);
+
+	ret = cdev_add(&entry->cdev, entry->devt, 1);
+	if (ret) {
+		dev_err(priv->dev, "Could not add char dev\n");
+		return ret;
+	}
+
+	dev = device_create(&pmt_crashlog_class, &priv->parent->dev, entry->devt,
+			    entry, "%s%d", "crashlog", entry->devid);
+
+	if (IS_ERR(dev)) {
+		dev_err(priv->dev, "Could not create device node\n");
+		cdev_del(&entry->cdev);
+	}
+
+	return PTR_ERR_OR_ZERO(dev);
+}
+
+static void
+pmt_crashlog_populate_header(void __iomem *disc_offset,
+			     struct crashlog_header *header)
+{
+	u32 discovery_header = readl(disc_offset);
+
+	header->access_type = GET_ACCESS(discovery_header);
+	header->crash_type = GET_TYPE(discovery_header);
+	header->version = GET_VERSION(discovery_header);
+	header->guid = readl(disc_offset + GUID_OFFSET);
+	header->base_offset = readl(disc_offset + BASE_OFFSET);
+
+	/*
+	 * For non-local access types the lower 3 bits of base offset
+	 * contains the index of the base address register where the
+	 * crashlogetry can be found.
+	 */
+	header->bir = GET_BIR(header->base_offset);
+	header->base_offset ^= header->bir;
+
+	/* Size is measured in DWORDs */
+	header->size = readl(disc_offset + SIZE_OFFSET);
+}
+
+static int pmt_crashlog_add_entry(struct pmt_crashlog_priv *priv,
+				  struct crashlog_entry *entry)
+{
+	struct resource *res = entry->header_res;
+	int ret;
+
+	pmt_crashlog_populate_header(entry->disc_table, &entry->header);
+
+	/* Local access and BARID only for now */
+	switch (entry->header.access_type) {
+	case ACCESS_LOCAL:
+		dev_info(priv->dev, "access_type: LOCAL\n");
+		if (entry->header.bir) {
+			dev_err(priv->dev,
+				"Unsupported BAR index %d for access type %d\n",
+				entry->header.bir, entry->header.access_type);
+			return -EINVAL;
+		}
+
+		entry->crashlog_data = res->start + resource_size(res) +
+				       entry->header.base_offset;
+		break;
+
+	case ACCESS_BARID:
+		dev_info(priv->dev, "access_type: BARID\n");
+		entry->crashlog_data =
+			priv->parent->resource[entry->header.bir].start +
+			entry->header.base_offset;
+		break;
+
+	default:
+		dev_err(priv->dev, "Unsupported access type %d\n",
+			entry->header.access_type);
+		return -EINVAL;
+	}
+
+	dev_info(priv->dev, "crashlod_data address: 0x%lx\n", entry->crashlog_data);
+
+	entry->crashlog_data_size = entry->header.size * 4;
+
+	if (entry->header.crash_type != CRASH_TYPE_OOBMSM) {
+		dev_err(priv->dev, "Unsupported crashlog header type %d\n",
+			entry->header.crash_type);
+		return -EINVAL;
+	}
+
+	if (entry->header.version != 0) {
+		dev_err(priv->dev, "Unsupported version value %d\n",
+			entry->header.version);
+		return -EINVAL;
+	}
+
+	entry->ida = &crashlog_devid_ida;
+
+	entry->devid = ida_simple_get(entry->ida, 0, 0, GFP_KERNEL);
+	if (entry->devid < 0)
+		return entry->devid;
+
+	ret = pmt_crashlog_make_dev(priv, entry);
+	if (ret) {
+		ida_simple_remove(entry->ida, entry->devid);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void pmt_crashlog_remove_entries(struct pmt_crashlog_priv *priv)
+{
+	int i;
+
+	for (i = 0; i < priv->num_entries; i++) {
+		device_destroy(&pmt_crashlog_class, priv->entry[i].devt);
+		cdev_del(&priv->entry[i].cdev);
+
+		unregister_chrdev_region(priv->entry[i].devt, 1);
+		ida_simple_remove(priv->entry[i].ida, priv->entry[i].devid);
+	}
+}
+static int pmt_crashlog_probe(struct platform_device *pdev)
+{
+	struct pmt_crashlog_priv *priv;
+	struct crashlog_entry *entry;
+	int i;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+	priv->dev = &pdev->dev;
+	priv->parent  = to_pci_dev(priv->dev->parent);
+
+	priv->entry = devm_kcalloc(&pdev->dev, pdev->num_resources,
+				   sizeof(*(priv->entry)), GFP_KERNEL);
+
+	for (i = 0, entry = priv->entry; i < pdev->num_resources;
+	     i++, entry++) {
+		int ret;
+
+		entry->header_res = platform_get_resource(pdev, IORESOURCE_MEM,
+							  i);
+		if (!entry->header_res) {
+			pmt_crashlog_remove_entries(priv);
+			return -ENODEV;
+		}
+
+		dev_info(&pdev->dev, "%d res start: 0x%llx, end 0x%llx\n", i,
+			 entry->header_res->start, entry->header_res->end);
+
+		entry->disc_table = devm_platform_ioremap_resource(pdev, i);
+		if (IS_ERR(entry->disc_table)) {
+			pmt_crashlog_remove_entries(priv);
+			return PTR_ERR(entry->disc_table);
+		}
+
+		ret = pmt_crashlog_add_entry(priv, entry);
+		if (ret) {
+			pmt_crashlog_remove_entries(priv);
+			return ret;
+		}
+
+		entry->priv = priv;
+		priv->num_entries++;
+	}
+
+	return 0;
+}
+
+static int pmt_crashlog_remove(struct platform_device *pdev)
+{
+	struct pmt_crashlog_priv *priv;
+
+	priv = (struct pmt_crashlog_priv *)platform_get_drvdata(pdev);
+
+	pmt_crashlog_remove_entries(priv);
+
+	return 0;
+}
+
+static struct platform_driver pmt_crashlog_driver = {
+	.driver = {
+		.name   = DRV_NAME,
+	},
+	.probe  = pmt_crashlog_probe,
+	.remove = pmt_crashlog_remove,
+};
+
+static int __init pmt_crashlog_init(void)
+{
+	int ret = class_register(&pmt_crashlog_class);
+
+	if (ret)
+		return ret;
+
+	ret = platform_driver_register(&pmt_crashlog_driver);
+	if (ret) {
+		class_unregister(&pmt_crashlog_class);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void __exit pmt_crashlog_exit(void)
+{
+	platform_driver_unregister(&pmt_crashlog_driver);
+	class_unregister(&pmt_crashlog_class);
+	ida_destroy(&crashlog_devid_ida);
+}
+
+module_init(pmt_crashlog_init);
+module_exit(pmt_crashlog_exit);
+
+MODULE_AUTHOR("Alexander Duyck <alexander.h.duyck@linux.intel.com>");
+MODULE_DESCRIPTION("Intel PMT Crashlog driver");
+MODULE_ALIAS("platform:" DRV_NAME);
+MODULE_LICENSE("GPL v2");
-- 
2.20.1

Re: [PATCH 1/3] mfd: intel_pmt: Add OOBMSM device ID

[Hans de Goede]

Hi,

On 9/11/20 9:45 PM, David E. Box wrote:
> Add Out of Band Management Services Module device ID to Intel PMT driver.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Looks good to me:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Lee, I expect you will pick this-one up (and the next also) ?

Regards,

Hans

> ---
>   drivers/mfd/intel_pmt.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mfd/intel_pmt.c b/drivers/mfd/intel_pmt.c
> index 0e572b105101..8f9970ab3026 100644
> --- a/drivers/mfd/intel_pmt.c
> +++ b/drivers/mfd/intel_pmt.c
> @@ -55,6 +55,8 @@ struct pmt_platform_info {
>   	unsigned long quirks;
>   };
>   
> +static const struct pmt_platform_info pmt_info;
> +
>   static const struct pmt_platform_info tgl_info = {
>   	.quirks = PMT_QUIRK_NO_WATCHER | PMT_QUIRK_NO_CRASHLOG |
>   		  PMT_QUIRK_TABLE_SHIFT,
> @@ -200,8 +202,10 @@ static void pmt_pci_remove(struct pci_dev *pdev)
>   	pm_runtime_get_sync(&pdev->dev);
>   }
>   
> +#define PCI_DEVICE_ID_INTEL_PMT_OOBMSM	0x09a7
>   #define PCI_DEVICE_ID_INTEL_PMT_TGL	0x9a0d
>   static const struct pci_device_id pmt_pci_ids[] = {
> +	{ PCI_DEVICE_DATA(INTEL, PMT_OOBMSM, &pmt_info) },
>   	{ PCI_DEVICE_DATA(INTEL, PMT_TGL, &tgl_info) },
>   	{ }
>   };
> 

Re: [PATCH 2/3] mfd: intel_pmt: Add Alder Lake (ADL) support

[Hans de Goede]

Hi,

On 9/11/20 9:45 PM, David E. Box wrote:
> Add PMT support for Alder Lake (ADL). Use same quirks as Tiger Lake since
> the design is the same, meaning no support for Watcher or Crashlog.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Looks good to me:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Lee, I expect you will pick this-one up (and the next also) ?

Regards,

Hans



> ---
>   drivers/mfd/intel_pmt.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mfd/intel_pmt.c b/drivers/mfd/intel_pmt.c
> index 8f9970ab3026..1b57a970a9d7 100644
> --- a/drivers/mfd/intel_pmt.c
> +++ b/drivers/mfd/intel_pmt.c
> @@ -202,9 +202,11 @@ static void pmt_pci_remove(struct pci_dev *pdev)
>   	pm_runtime_get_sync(&pdev->dev);
>   }
>   
> +#define PCI_DEVICE_ID_INTEL_PMT_ADL	0x467d
>   #define PCI_DEVICE_ID_INTEL_PMT_OOBMSM	0x09a7
>   #define PCI_DEVICE_ID_INTEL_PMT_TGL	0x9a0d
>   static const struct pci_device_id pmt_pci_ids[] = {
> +	{ PCI_DEVICE_DATA(INTEL, PMT_ADL, &tgl_info) },
>   	{ PCI_DEVICE_DATA(INTEL, PMT_OOBMSM, &pmt_info) },
>   	{ PCI_DEVICE_DATA(INTEL, PMT_TGL, &tgl_info) },
>   	{ }
> 

Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

[Hans de Goede]

Hi,

On 9/11/20 9:45 PM, David E. Box wrote:
> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 
> Add support for the Intel Platform Monitoring Technology crashlog
> interface.  This interface provides a few sysfs values to allow for
> controlling the crashlog telemetry interface as well as a character driver
> to allow for mapping the crashlog memory region so that it can be accessed
> after a crashlog has been recorded.
> 
> This driver is meant to only support the server version of the crashlog
> which is identified as crash_type 1 with a version of zero. Currently no
> other types are supported.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>   .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
>   drivers/platform/x86/Kconfig                  |  10 +
>   drivers/platform/x86/Makefile                 |   1 +
>   drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
>   4 files changed, 665 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
>   create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-pmt_crashlog b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
> new file mode 100644
> index 000000000000..40fb4ff437a6
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
> @@ -0,0 +1,66 @@
> +What:		/sys/class/pmt_crashlog/
> +Date:		September 2020
> +KernelVersion:	5.10
> +Contact:	Alexander Duyck <alexander.h.duyck@linux.intel.com>
> +Description:
> +		The pmt_crashlog/ class directory contains information
> +		for devices that expose crashlog capabilities using the Intel
> +		Platform Monitoring Technology (PTM).
> +
> +What:		/sys/class/pmt_crashlog/crashlogX
> +Date:		September 2020
> +KernelVersion:	5.10
> +Contact:	Alexander Duyck <alexander.h.duyck@linux.intel.com>
> +Description:
> +		The crashlogX directory contains files for configuring an
> +		instance of a PMT crashlog device that can perform crash data
> +		recoring. Each crashlogX device has an associated
> +		/dev/crashlogX device node. This node can be opened and mapped
> +		to access the resulting crashlog data. The register layout for
> +		the log can be determined from an XML file of specified guid
> +		for the parent device.
> +
> +What:		/sys/class/pmt_crashlog/crashlogX/guid
> +Date:		September 2020
> +KernelVersion:	5.10
> +Contact:	Alexander Duyck <alexander.h.duyck@linux.intel.com>
> +Description:
> +		(RO) The guid for this crashlog device. The guid identifies the
> +		version of the XML file for the parent device that should be
> +		used to determine the register layout.
> +
> +What:		/sys/class/pmt_crashlog/crashlogX/size
> +Date:		September 2020
> +KernelVersion:	5.10
> +Contact:	Alexander Duyck <alexander.h.duyck@linux.intel.com>
> +Description:
> +		(RO) The length of the result buffer in bytes that corresponds
> +		to the mapping size for the /dev/crashlogX device node.
> +
> +What:		/sys/class/pmt_crashlog/crashlogX/offset
> +Date:		September 2020
> +KernelVersion:	5.10
> +Contact:	Alexander Duyck <alexander.h.duyck@linux.intel.com>
> +Description:
> +		(RO) The offset of the buffer in bytes that corresponds
> +		to the mapping for the /dev/crashlogX device node.
> +
> +What:		/sys/class/pmt_crashlog/crashlogX/enable
> +Date:		September 2020
> +KernelVersion:	5.10
> +Contact:	Alexander Duyck <alexander.h.duyck@linux.intel.com>
> +Description:
> +		(RW) Boolean value controlling if the crashlog functionality
> +		is enabled for the /dev/crashlogX device node.
> +
> +What:		/sys/class/pmt_crashlog/crashlogX/trigger
> +Date:		September 2020
> +KernelVersion:	5.10
> +Contact:	Alexander Duyck <alexander.h.duyck@linux.intel.com>
> +Description:
> +		(RW) Boolean value controlling  the triggering of the
> +		/dev/crashlogX device node. When read it provides data on if
> +		the crashlog has been triggered. When written to it can be
> +		used to either clear the current trigger by writing false, or
> +		to trigger a new event if the trigger is not currently set.
> +

Both the pmt_crashlog and the attributes suggest that this is highly
Intel PMT specific. /sys/class/foo interfaces are generally speaking
meant to be generic interfaces.

If this was defining a generic, vendor and implementation agnostic interface for
configuring / accessing crashlogs, then using a class would be fine, but that
is not the case, so I believe that this should not implement / register a class.

Since the devices are instantiated through MFD there already is a
static sysfs-path which can be used to find the device in sysfs:
/sys/bus/platform/device/pmt_crashlog

So you can register the sysfs attributes directly under the platform_device
and then userspace can easily find them, so there really is no need to
use a class here.

> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 48335b02014f..50c3234e4f72 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1360,6 +1360,16 @@ config INTEL_PMC_CORE
>   		- LTR Ignore
>   		- MPHY/PLL gating status (Sunrisepoint PCH only)
>   
> +config INTEL_PMT_CRASHLOG
> +	tristate "Intel Platform Monitoring Technology (PMT) Crashlog driver"
> +	help
> +	 The Intel Platform Monitoring Technology (PMT) crashlog driver provides
> +	 access to hardware crashlog capabilities on devices that support the
> +	 feature.
> +
> +	 For more information, see
> +	 <file:Documentation/ABI/testing/sysfs-class-intel_pmt_crashlog>
> +
>   config INTEL_PMT_TELEMETRY
>   	tristate "Intel Platform Monitoring Technology (PMT) Telemetry driver"
>   	help
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index ca942e70de8d..1b8b2502d460 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -140,6 +140,7 @@ obj-$(CONFIG_INTEL_MFLD_THERMAL)	+= intel_mid_thermal.o
>   obj-$(CONFIG_INTEL_MID_POWER_BUTTON)	+= intel_mid_powerbtn.o
>   obj-$(CONFIG_INTEL_MRFLD_PWRBTN)	+= intel_mrfld_pwrbtn.o
>   obj-$(CONFIG_INTEL_PMC_CORE)		+= intel_pmc_core.o intel_pmc_core_pltdrv.o
> +obj-$(CONFIG_INTEL_PMT_CRASHLOG)	+= intel_pmt_crashlog.o
>   obj-$(CONFIG_INTEL_PMT_TELEMETRY)	+= intel_pmt_telemetry.o
>   obj-$(CONFIG_INTEL_PUNIT_IPC)		+= intel_punit_ipc.o
>   obj-$(CONFIG_INTEL_SCU_IPC)		+= intel_scu_ipc.o
> diff --git a/drivers/platform/x86/intel_pmt_crashlog.c b/drivers/platform/x86/intel_pmt_crashlog.c
> new file mode 100644
> index 000000000000..31d43708055c
> --- /dev/null
> +++ b/drivers/platform/x86/intel_pmt_crashlog.c
> @@ -0,0 +1,588 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Platform Monitoring Technology Crashlog driver
> + *
> + * Copyright (c) 2020, Intel Corporation.
> + * All Rights Reserved.
> + *
> + * Authors: "Alexander Duyck" <alexander.h.duyck@linux.intel.com>
> + */
> +
> +#include <linux/cdev.h>
> +#include <linux/idr.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +
> +#define DRV_NAME		"pmt_crashlog"
> +
> +/* Crashlog access types */
> +#define ACCESS_FUTURE		1
> +#define ACCESS_BARID		2
> +#define ACCESS_LOCAL		3
> +
> +/* Crashlog discovery header types */
> +#define CRASH_TYPE_OOBMSM	1
> +
> +/* Control Flags */
> +#define CRASHLOG_FLAG_DISABLE	BIT(27)
> +#define CRASHLOG_FLAG_CLEAR	BIT(28)
> +#define CRASHLOG_FLAG_EXECUTE	BIT(29)
> +#define CRASHLOG_FLAG_COMPLETE	BIT(31)
> +#define CRASHLOG_FLAG_MASK	GENMASK(31, 28)
> +
> +/* Common Header */
> +#define CONTROL_OFFSET		0x0
> +#define GUID_OFFSET		0x4
> +#define BASE_OFFSET		0x8
> +#define SIZE_OFFSET		0xC
> +#define GET_ACCESS(v)		((v) & GENMASK(3, 0))
> +#define GET_TYPE(v)		(((v) & GENMASK(7, 4)) >> 4)
> +#define GET_VERSION(v)		(((v) & GENMASK(19, 16)) >> 16)
> +
> +#define GET_ADDRESS(v)		((v) & GENMASK(31, 3))
> +#define GET_BIR(v)		((v) & GENMASK(2, 0))
> +
> +static DEFINE_IDA(crashlog_devid_ida);
> +
> +struct crashlog_header {
> +	u32	base_offset;
> +	u32	size;
> +	u32	guid;
> +	u8	bir;
> +	u8	access_type;
> +	u8	crash_type;
> +	u8	version;
> +};
> +
> +struct pmt_crashlog_priv;
> +
> +struct crashlog_entry {
> +	struct pmt_crashlog_priv	*priv;
> +	struct crashlog_header		header;
> +	struct resource			*header_res;
> +	void __iomem			*disc_table;
> +	unsigned long			crashlog_data;
> +	size_t				crashlog_data_size;
> +	struct cdev			cdev;
> +	dev_t				devt;
> +	int				devid;
> +	struct ida			*ida;
> +};
> +
> +struct pmt_crashlog_priv {
> +	struct device		*dev;
> +	struct pci_dev		*parent;
> +	struct crashlog_entry	*entry;
> +	int			num_entries;
> +};
> +
> +/*
> + * I/O
> + */
> +static bool pmt_crashlog_complete(struct crashlog_entry *entry)
> +{
> +	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +
> +	/* return current value of the crashlog complete flag */
> +	return !!(control & CRASHLOG_FLAG_COMPLETE);
> +}
> +
> +static bool pmt_crashlog_disabled(struct crashlog_entry *entry)
> +{
> +	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +
> +	/* return current value of the crashlog disabled flag */
> +	return !!(control & CRASHLOG_FLAG_DISABLE);
> +}
> +
> +static void pmt_crashlog_set_disable(struct crashlog_entry *entry, bool disable)
> +{
> +	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +
> +	/* clear control bits */
> +	control &= ~(CRASHLOG_FLAG_MASK | CRASHLOG_FLAG_DISABLE);
> +	if (disable)
> +		control |= CRASHLOG_FLAG_DISABLE;
> +
> +	writel(control, entry->disc_table + CONTROL_OFFSET);
> +}
> +
> +static void pmt_crashlog_set_clear(struct crashlog_entry *entry)
> +{
> +	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +
> +	/* clear control bits */
> +	control &= ~CRASHLOG_FLAG_MASK;
> +	control |= CRASHLOG_FLAG_CLEAR;
> +
> +	writel(control, entry->disc_table + CONTROL_OFFSET);
> +}
> +
> +static void pmt_crashlog_set_execute(struct crashlog_entry *entry)
> +{
> +	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +
> +	/* clear control bits */
> +	control &= ~CRASHLOG_FLAG_MASK;
> +	control |= CRASHLOG_FLAG_EXECUTE;
> +
> +	writel(control, entry->disc_table + CONTROL_OFFSET);
> +}

These 3 pmt_crashlog_set_* functions are all triggered through
sysfs writes and they all do read-modify-write of the control-register,
so this is racy. You need to add a mutex to protect the r-m-w sequences.


> +
> +/*
> + * devfs
> + */
> +static int pmt_crashlog_open(struct inode *inode, struct file *filp)
> +{
> +	struct crashlog_entry *entry;
> +	struct pci_driver *pci_drv;
> +	struct pmt_crashlog_priv *priv;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	entry = container_of(inode->i_cdev, struct crashlog_entry, cdev);
> +	priv = entry->priv;
> +	pci_drv = pci_dev_driver(priv->parent);
> +
> +	if (!pci_drv)
> +		return -ENODEV;
> +
> +	filp->private_data = entry;
> +	get_device(&priv->parent->dev);
> +
> +	if (!try_module_get(pci_drv->driver.owner)) {
> +		put_device(&priv->parent->dev);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static int pmt_crashlog_release(struct inode *inode, struct file *filp)
> +{
> +	struct crashlog_entry *entry = filp->private_data;
> +	struct pmt_crashlog_priv *priv;
> +	struct pci_driver *pci_drv;
> +
> +	priv = entry->priv;
> +	pci_drv = pci_dev_driver(priv->parent);
> +
> +	put_device(&priv->parent->dev);
> +	module_put(pci_drv->driver.owner);
> +
> +	return 0;
> +}
> +
> +static int
> +pmt_crashlog_mmap(struct file *filp, struct vm_area_struct *vma)
> +{
> +	struct crashlog_entry *entry = filp->private_data;
> +	struct pmt_crashlog_priv *priv;
> +	unsigned long phys = entry->crashlog_data;
> +	unsigned long pfn = PFN_DOWN(phys);
> +	unsigned long vsize = vma->vm_end - vma->vm_start;
> +	unsigned long psize;
> +
> +	if ((vma->vm_flags & VM_WRITE) ||
> +	    (vma->vm_flags & VM_MAYWRITE))
> +		return -EPERM;
> +
> +	priv = entry->priv;
> +
> +	if (!entry->crashlog_data_size) {
> +		dev_err(priv->dev, "Crashlog data not accessible\n");
> +		return -EAGAIN;
> +	}
> +
> +	psize = (PFN_UP(entry->crashlog_data + entry->crashlog_data_size) - pfn) *
> +		PAGE_SIZE;
> +	if (vsize > psize) {
> +		dev_err(priv->dev, "Requested mmap size is too large\n");
> +		return -EINVAL;
> +	}
> +
> +	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> +	if (io_remap_pfn_range(vma, vma->vm_start, pfn,
> +		vsize, vma->vm_page_prot))
> +		return -EAGAIN;
> +
> +	return 0;
> +}
> +
> +static const struct file_operations pmt_crashlog_fops = {
> +	.owner =	THIS_MODULE,
> +	.open =		pmt_crashlog_open,
> +	.mmap =		pmt_crashlog_mmap,

mmap but no read, I guess read may be emulated through mmap,
is that the case ?

I can see sysadmins wanting to be able to do a simple cat
on this file to get the logs (including headers), so if
the kernel-core does not emulate read in this case, you
should really add read support I guess.

Also how big are these files ?  sysfs also supports binary
files, so unless these files are huge / this is really
performance critical it may make more sense to just add
a binary sysfs attr for this and get rid of the whole chardev
all together.


> +	.release =	pmt_crashlog_release,
> +};
> +
> +/*
> + * sysfs
> + */
> +static ssize_t
> +guid_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct crashlog_entry *entry;
> +
> +	entry = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "0x%x\n", entry->header.guid);
> +}
> +static DEVICE_ATTR_RO(guid);
> +
> +static ssize_t size_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct crashlog_entry *entry;
> +
> +	entry = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%lu\n", entry->crashlog_data_size);
> +}
> +static DEVICE_ATTR_RO(size);
> +
> +static ssize_t
> +offset_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct crashlog_entry *entry;
> +
> +	entry = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%lu\n", offset_in_page(entry->crashlog_data));
> +}
> +static DEVICE_ATTR_RO(offset);
> +
> +static ssize_t
> +enable_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct crashlog_entry *entry;
> +	int enabled;
> +
> +	entry = dev_get_drvdata(dev);
> +	enabled = !pmt_crashlog_disabled(entry);
> +
> +	return sprintf(buf, "%d\n", enabled);
> +}
> +
> +static ssize_t
> +enable_store(struct device *dev, struct device_attribute *attr,
> +	    const char *buf, size_t count)
> +{
> +	struct crashlog_entry *entry;
> +	bool enabled;
> +	int result;
> +
> +	entry = dev_get_drvdata(dev);
> +
> +	result = kstrtobool(buf, &enabled);
> +	if (result)
> +		return result;
> +
> +	pmt_crashlog_set_disable(entry, !enabled);
> +
> +	return strnlen(buf, count);
> +}
> +static DEVICE_ATTR_RW(enable);
> +
> +static ssize_t
> +trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct crashlog_entry *entry;
> +	int trigger;
> +
> +	entry = dev_get_drvdata(dev);
> +	trigger = pmt_crashlog_complete(entry);
> +
> +	return sprintf(buf, "%d\n", trigger);
> +}
> +
> +static ssize_t
> +trigger_store(struct device *dev, struct device_attribute *attr,
> +	    const char *buf, size_t count)
> +{
> +	struct crashlog_entry *entry;
> +	bool trigger;
> +	int result;
> +
> +	entry = dev_get_drvdata(dev);
> +
> +	result = kstrtobool(buf, &trigger);
> +	if (result)
> +		return result;
> +
> +	if (trigger) {
> +		/* we cannot trigger a new crash if one is still pending */
> +		if (pmt_crashlog_complete(entry))
> +			return -EEXIST;
> +
> +		/* if device is currently disabled, return busy */
> +		if (pmt_crashlog_disabled(entry))
> +			return -EBUSY;
> +
> +		pmt_crashlog_set_execute(entry);
> +	} else {
> +		pmt_crashlog_set_clear(entry);
> +	}
> +
> +	return strnlen(buf, count);
> +}
> +static DEVICE_ATTR_RW(trigger);
> +
> +static struct attribute *pmt_crashlog_attrs[] = {
> +	&dev_attr_guid.attr,
> +	&dev_attr_size.attr,
> +	&dev_attr_offset.attr,
> +	&dev_attr_enable.attr,
> +	&dev_attr_trigger.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(pmt_crashlog);
> +
> +static struct class pmt_crashlog_class = {
> +	.name = "pmt_crashlog",
> +	.owner = THIS_MODULE,
> +	.dev_groups = pmt_crashlog_groups,
> +};
> +
> +/*
> + * initialization
> + */
> +static int pmt_crashlog_make_dev(struct pmt_crashlog_priv *priv,
> +				 struct crashlog_entry *entry)
> +{
> +	struct device *dev;
> +	int ret;
> +
> +	ret = alloc_chrdev_region(&entry->devt, 0, 1, DRV_NAME);
> +	if (ret < 0) {
> +		dev_err(priv->dev, "alloc_chrdev_region err: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Create a character device for Samplers */
> +	cdev_init(&entry->cdev, &pmt_crashlog_fops);
> +
> +	ret = cdev_add(&entry->cdev, entry->devt, 1);
> +	if (ret) {
> +		dev_err(priv->dev, "Could not add char dev\n");
> +		return ret;
> +	}
> +
> +	dev = device_create(&pmt_crashlog_class, &priv->parent->dev, entry->devt,
> +			    entry, "%s%d", "crashlog", entry->devid);
> +
> +	if (IS_ERR(dev)) {
> +		dev_err(priv->dev, "Could not create device node\n");
> +		cdev_del(&entry->cdev);
> +	}
> +
> +	return PTR_ERR_OR_ZERO(dev);
> +}
> +
> +static void
> +pmt_crashlog_populate_header(void __iomem *disc_offset,
> +			     struct crashlog_header *header)
> +{
> +	u32 discovery_header = readl(disc_offset);
> +
> +	header->access_type = GET_ACCESS(discovery_header);
> +	header->crash_type = GET_TYPE(discovery_header);
> +	header->version = GET_VERSION(discovery_header);
> +	header->guid = readl(disc_offset + GUID_OFFSET);
> +	header->base_offset = readl(disc_offset + BASE_OFFSET);
> +
> +	/*
> +	 * For non-local access types the lower 3 bits of base offset
> +	 * contains the index of the base address register where the
> +	 * crashlogetry can be found.
> +	 */
> +	header->bir = GET_BIR(header->base_offset);
> +	header->base_offset ^= header->bir;
> +
> +	/* Size is measured in DWORDs */
> +	header->size = readl(disc_offset + SIZE_OFFSET);
> +}
> +
> +static int pmt_crashlog_add_entry(struct pmt_crashlog_priv *priv,
> +				  struct crashlog_entry *entry)
> +{
> +	struct resource *res = entry->header_res;
> +	int ret;
> +
> +	pmt_crashlog_populate_header(entry->disc_table, &entry->header);
> +
> +	/* Local access and BARID only for now */
> +	switch (entry->header.access_type) {
> +	case ACCESS_LOCAL:
> +		dev_info(priv->dev, "access_type: LOCAL\n");
> +		if (entry->header.bir) {
> +			dev_err(priv->dev,
> +				"Unsupported BAR index %d for access type %d\n",
> +				entry->header.bir, entry->header.access_type);
> +			return -EINVAL;
> +		}
> +
> +		entry->crashlog_data = res->start + resource_size(res) +
> +				       entry->header.base_offset;
> +		break;
> +
> +	case ACCESS_BARID:
> +		dev_info(priv->dev, "access_type: BARID\n");
> +		entry->crashlog_data =
> +			priv->parent->resource[entry->header.bir].start +
> +			entry->header.base_offset;
> +		break;
> +
> +	default:
> +		dev_err(priv->dev, "Unsupported access type %d\n",
> +			entry->header.access_type);
> +		return -EINVAL;
> +	}
> +
> +	dev_info(priv->dev, "crashlod_data address: 0x%lx\n", entry->crashlog_data);
> +
> +	entry->crashlog_data_size = entry->header.size * 4;
> +
> +	if (entry->header.crash_type != CRASH_TYPE_OOBMSM) {
> +		dev_err(priv->dev, "Unsupported crashlog header type %d\n",
> +			entry->header.crash_type);
> +		return -EINVAL;
> +	}
> +
> +	if (entry->header.version != 0) {
> +		dev_err(priv->dev, "Unsupported version value %d\n",
> +			entry->header.version);
> +		return -EINVAL;
> +	}
> +
> +	entry->ida = &crashlog_devid_ida;
> +
> +	entry->devid = ida_simple_get(entry->ida, 0, 0, GFP_KERNEL);
> +	if (entry->devid < 0)
> +		return entry->devid;
> +
> +	ret = pmt_crashlog_make_dev(priv, entry);
> +	if (ret) {
> +		ida_simple_remove(entry->ida, entry->devid);
> +		return ret;
> +	}

Hmm wait, you are making one chardev per log entry ? Then just using
binary sysfs attributes seems to make even more sense to me.

> +
> +	return 0;
> +}
> +
> +static void pmt_crashlog_remove_entries(struct pmt_crashlog_priv *priv)
> +{
> +	int i;
> +
> +	for (i = 0; i < priv->num_entries; i++) {
> +		device_destroy(&pmt_crashlog_class, priv->entry[i].devt);
> +		cdev_del(&priv->entry[i].cdev);
> +
> +		unregister_chrdev_region(priv->entry[i].devt, 1);
> +		ida_simple_remove(priv->entry[i].ida, priv->entry[i].devid);
> +	}
> +}
> +static int pmt_crashlog_probe(struct platform_device *pdev)
> +{
> +	struct pmt_crashlog_priv *priv;
> +	struct crashlog_entry *entry;
> +	int i;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +	priv->dev = &pdev->dev;
> +	priv->parent  = to_pci_dev(priv->dev->parent);
> +
> +	priv->entry = devm_kcalloc(&pdev->dev, pdev->num_resources,
> +				   sizeof(*(priv->entry)), GFP_KERNEL);
> +
> +	for (i = 0, entry = priv->entry; i < pdev->num_resources;
> +	     i++, entry++) {
> +		int ret;
> +
> +		entry->header_res = platform_get_resource(pdev, IORESOURCE_MEM,
> +							  i);
> +		if (!entry->header_res) {
> +			pmt_crashlog_remove_entries(priv);
> +			return -ENODEV;
> +		}
> +
> +		dev_info(&pdev->dev, "%d res start: 0x%llx, end 0x%llx\n", i,
> +			 entry->header_res->start, entry->header_res->end);
> +
> +		entry->disc_table = devm_platform_ioremap_resource(pdev, i);
> +		if (IS_ERR(entry->disc_table)) {
> +			pmt_crashlog_remove_entries(priv);
> +			return PTR_ERR(entry->disc_table);
> +		}
> +
> +		ret = pmt_crashlog_add_entry(priv, entry);
> +		if (ret) {
> +			pmt_crashlog_remove_entries(priv);
> +			return ret;
> +		}
> +
> +		entry->priv = priv;
> +		priv->num_entries++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int pmt_crashlog_remove(struct platform_device *pdev)
> +{
> +	struct pmt_crashlog_priv *priv;
> +
> +	priv = (struct pmt_crashlog_priv *)platform_get_drvdata(pdev);
> +
> +	pmt_crashlog_remove_entries(priv);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver pmt_crashlog_driver = {
> +	.driver = {
> +		.name   = DRV_NAME,
> +	},
> +	.probe  = pmt_crashlog_probe,
> +	.remove = pmt_crashlog_remove,
> +};
> +
> +static int __init pmt_crashlog_init(void)
> +{
> +	int ret = class_register(&pmt_crashlog_class);
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = platform_driver_register(&pmt_crashlog_driver);
> +	if (ret) {
> +		class_unregister(&pmt_crashlog_class);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit pmt_crashlog_exit(void)
> +{
> +	platform_driver_unregister(&pmt_crashlog_driver);
> +	class_unregister(&pmt_crashlog_class);
> +	ida_destroy(&crashlog_devid_ida);
> +}
> +
> +module_init(pmt_crashlog_init);
> +module_exit(pmt_crashlog_exit);
> +
> +MODULE_AUTHOR("Alexander Duyck <alexander.h.duyck@linux.intel.com>");
> +MODULE_DESCRIPTION("Intel PMT Crashlog driver");
> +MODULE_ALIAS("platform:" DRV_NAME);
> +MODULE_LICENSE("GPL v2");
> 

Regards,

Hans

Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

[Alexander Duyck]

On Mon, Sep 14, 2020 at 6:42 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 9/11/20 9:45 PM, David E. Box wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > Add support for the Intel Platform Monitoring Technology crashlog
> > interface.  This interface provides a few sysfs values to allow for
> > controlling the crashlog telemetry interface as well as a character driver
> > to allow for mapping the crashlog memory region so that it can be accessed
> > after a crashlog has been recorded.
> >
> > This driver is meant to only support the server version of the crashlog
> > which is identified as crash_type 1 with a version of zero. Currently no
> > other types are supported.
> >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> >   .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
> >   drivers/platform/x86/Kconfig                  |  10 +
> >   drivers/platform/x86/Makefile                 |   1 +
> >   drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
> >   4 files changed, 665 insertions(+)
> >   create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
> >   create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-pmt_crashlog b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
> > new file mode 100644
> > index 000000000000..40fb4ff437a6
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
> > @@ -0,0 +1,66 @@
> > +What:                /sys/class/pmt_crashlog/
> > +Date:                September 2020
> > +KernelVersion:       5.10
> > +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > +Description:
> > +             The pmt_crashlog/ class directory contains information
> > +             for devices that expose crashlog capabilities using the Intel
> > +             Platform Monitoring Technology (PTM).
> > +
> > +What:                /sys/class/pmt_crashlog/crashlogX
> > +Date:                September 2020
> > +KernelVersion:       5.10
> > +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > +Description:
> > +             The crashlogX directory contains files for configuring an
> > +             instance of a PMT crashlog device that can perform crash data
> > +             recoring. Each crashlogX device has an associated
> > +             /dev/crashlogX device node. This node can be opened and mapped
> > +             to access the resulting crashlog data. The register layout for
> > +             the log can be determined from an XML file of specified guid
> > +             for the parent device.
> > +
> > +What:                /sys/class/pmt_crashlog/crashlogX/guid
> > +Date:                September 2020
> > +KernelVersion:       5.10
> > +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > +Description:
> > +             (RO) The guid for this crashlog device. The guid identifies the
> > +             version of the XML file for the parent device that should be
> > +             used to determine the register layout.
> > +
> > +What:                /sys/class/pmt_crashlog/crashlogX/size
> > +Date:                September 2020
> > +KernelVersion:       5.10
> > +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > +Description:
> > +             (RO) The length of the result buffer in bytes that corresponds
> > +             to the mapping size for the /dev/crashlogX device node.
> > +
> > +What:                /sys/class/pmt_crashlog/crashlogX/offset
> > +Date:                September 2020
> > +KernelVersion:       5.10
> > +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > +Description:
> > +             (RO) The offset of the buffer in bytes that corresponds
> > +             to the mapping for the /dev/crashlogX device node.
> > +
> > +What:                /sys/class/pmt_crashlog/crashlogX/enable
> > +Date:                September 2020
> > +KernelVersion:       5.10
> > +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > +Description:
> > +             (RW) Boolean value controlling if the crashlog functionality
> > +             is enabled for the /dev/crashlogX device node.
> > +
> > +What:                /sys/class/pmt_crashlog/crashlogX/trigger
> > +Date:                September 2020
> > +KernelVersion:       5.10
> > +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > +Description:
> > +             (RW) Boolean value controlling  the triggering of the
> > +             /dev/crashlogX device node. When read it provides data on if
> > +             the crashlog has been triggered. When written to it can be
> > +             used to either clear the current trigger by writing false, or
> > +             to trigger a new event if the trigger is not currently set.
> > +
>
> Both the pmt_crashlog and the attributes suggest that this is highly
> Intel PMT specific. /sys/class/foo interfaces are generally speaking
> meant to be generic interfaces.
>
> If this was defining a generic, vendor and implementation agnostic interface for
> configuring / accessing crashlogs, then using a class would be fine, but that
> is not the case, so I believe that this should not implement / register a class.
>
> Since the devices are instantiated through MFD there already is a
> static sysfs-path which can be used to find the device in sysfs:
> /sys/bus/platform/device/pmt_crashlog
>
> So you can register the sysfs attributes directly under the platform_device
> and then userspace can easily find them, so there really is no need to
> use a class here.

I see. So we change the root directory from "/sys/class/pmt_crashlog/"
to "/sys/bus/platform/device/pmt_crashlog" while retaining the same
functionality. That should be workable.

> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 48335b02014f..50c3234e4f72 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -1360,6 +1360,16 @@ config INTEL_PMC_CORE
> >               - LTR Ignore
> >               - MPHY/PLL gating status (Sunrisepoint PCH only)
> >
> > +config INTEL_PMT_CRASHLOG
> > +     tristate "Intel Platform Monitoring Technology (PMT) Crashlog driver"
> > +     help
> > +      The Intel Platform Monitoring Technology (PMT) crashlog driver provides
> > +      access to hardware crashlog capabilities on devices that support the
> > +      feature.
> > +
> > +      For more information, see
> > +      <file:Documentation/ABI/testing/sysfs-class-intel_pmt_crashlog>
> > +
> >   config INTEL_PMT_TELEMETRY
> >       tristate "Intel Platform Monitoring Technology (PMT) Telemetry driver"
> >       help
> > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> > index ca942e70de8d..1b8b2502d460 100644
> > --- a/drivers/platform/x86/Makefile
> > +++ b/drivers/platform/x86/Makefile
> > @@ -140,6 +140,7 @@ obj-$(CONFIG_INTEL_MFLD_THERMAL)  += intel_mid_thermal.o
> >   obj-$(CONFIG_INTEL_MID_POWER_BUTTON)        += intel_mid_powerbtn.o
> >   obj-$(CONFIG_INTEL_MRFLD_PWRBTN)    += intel_mrfld_pwrbtn.o
> >   obj-$(CONFIG_INTEL_PMC_CORE)                += intel_pmc_core.o intel_pmc_core_pltdrv.o
> > +obj-$(CONFIG_INTEL_PMT_CRASHLOG)     += intel_pmt_crashlog.o
> >   obj-$(CONFIG_INTEL_PMT_TELEMETRY)   += intel_pmt_telemetry.o
> >   obj-$(CONFIG_INTEL_PUNIT_IPC)               += intel_punit_ipc.o
> >   obj-$(CONFIG_INTEL_SCU_IPC)         += intel_scu_ipc.o
> > diff --git a/drivers/platform/x86/intel_pmt_crashlog.c b/drivers/platform/x86/intel_pmt_crashlog.c
> > new file mode 100644
> > index 000000000000..31d43708055c
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel_pmt_crashlog.c
> > @@ -0,0 +1,588 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Intel Platform Monitoring Technology Crashlog driver
> > + *
> > + * Copyright (c) 2020, Intel Corporation.
> > + * All Rights Reserved.
> > + *
> > + * Authors: "Alexander Duyck" <alexander.h.duyck@linux.intel.com>
> > + */
> > +
> > +#include <linux/cdev.h>
> > +#include <linux/idr.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +
> > +#define DRV_NAME             "pmt_crashlog"
> > +
> > +/* Crashlog access types */
> > +#define ACCESS_FUTURE                1
> > +#define ACCESS_BARID         2
> > +#define ACCESS_LOCAL         3
> > +
> > +/* Crashlog discovery header types */
> > +#define CRASH_TYPE_OOBMSM    1
> > +
> > +/* Control Flags */
> > +#define CRASHLOG_FLAG_DISABLE        BIT(27)
> > +#define CRASHLOG_FLAG_CLEAR  BIT(28)
> > +#define CRASHLOG_FLAG_EXECUTE        BIT(29)
> > +#define CRASHLOG_FLAG_COMPLETE       BIT(31)
> > +#define CRASHLOG_FLAG_MASK   GENMASK(31, 28)
> > +
> > +/* Common Header */
> > +#define CONTROL_OFFSET               0x0
> > +#define GUID_OFFSET          0x4
> > +#define BASE_OFFSET          0x8
> > +#define SIZE_OFFSET          0xC
> > +#define GET_ACCESS(v)                ((v) & GENMASK(3, 0))
> > +#define GET_TYPE(v)          (((v) & GENMASK(7, 4)) >> 4)
> > +#define GET_VERSION(v)               (((v) & GENMASK(19, 16)) >> 16)
> > +
> > +#define GET_ADDRESS(v)               ((v) & GENMASK(31, 3))
> > +#define GET_BIR(v)           ((v) & GENMASK(2, 0))
> > +
> > +static DEFINE_IDA(crashlog_devid_ida);
> > +
> > +struct crashlog_header {
> > +     u32     base_offset;
> > +     u32     size;
> > +     u32     guid;
> > +     u8      bir;
> > +     u8      access_type;
> > +     u8      crash_type;
> > +     u8      version;
> > +};
> > +
> > +struct pmt_crashlog_priv;
> > +
> > +struct crashlog_entry {
> > +     struct pmt_crashlog_priv        *priv;
> > +     struct crashlog_header          header;
> > +     struct resource                 *header_res;
> > +     void __iomem                    *disc_table;
> > +     unsigned long                   crashlog_data;
> > +     size_t                          crashlog_data_size;
> > +     struct cdev                     cdev;
> > +     dev_t                           devt;
> > +     int                             devid;
> > +     struct ida                      *ida;
> > +};
> > +
> > +struct pmt_crashlog_priv {
> > +     struct device           *dev;
> > +     struct pci_dev          *parent;
> > +     struct crashlog_entry   *entry;
> > +     int                     num_entries;
> > +};
> > +
> > +/*
> > + * I/O
> > + */
> > +static bool pmt_crashlog_complete(struct crashlog_entry *entry)
> > +{
> > +     u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> > +
> > +     /* return current value of the crashlog complete flag */
> > +     return !!(control & CRASHLOG_FLAG_COMPLETE);
> > +}
> > +
> > +static bool pmt_crashlog_disabled(struct crashlog_entry *entry)
> > +{
> > +     u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> > +
> > +     /* return current value of the crashlog disabled flag */
> > +     return !!(control & CRASHLOG_FLAG_DISABLE);
> > +}
> > +
> > +static void pmt_crashlog_set_disable(struct crashlog_entry *entry, bool disable)
> > +{
> > +     u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> > +
> > +     /* clear control bits */
> > +     control &= ~(CRASHLOG_FLAG_MASK | CRASHLOG_FLAG_DISABLE);
> > +     if (disable)
> > +             control |= CRASHLOG_FLAG_DISABLE;
> > +
> > +     writel(control, entry->disc_table + CONTROL_OFFSET);
> > +}
> > +
> > +static void pmt_crashlog_set_clear(struct crashlog_entry *entry)
> > +{
> > +     u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> > +
> > +     /* clear control bits */
> > +     control &= ~CRASHLOG_FLAG_MASK;
> > +     control |= CRASHLOG_FLAG_CLEAR;
> > +
> > +     writel(control, entry->disc_table + CONTROL_OFFSET);
> > +}
> > +
> > +static void pmt_crashlog_set_execute(struct crashlog_entry *entry)
> > +{
> > +     u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> > +
> > +     /* clear control bits */
> > +     control &= ~CRASHLOG_FLAG_MASK;
> > +     control |= CRASHLOG_FLAG_EXECUTE;
> > +
> > +     writel(control, entry->disc_table + CONTROL_OFFSET);
> > +}
>
> These 3 pmt_crashlog_set_* functions are all triggered through
> sysfs writes and they all do read-modify-write of the control-register,
> so this is racy. You need to add a mutex to protect the r-m-w sequences.

I'll update things to add the mutex.

> > +
> > +/*
> > + * devfs
> > + */
> > +static int pmt_crashlog_open(struct inode *inode, struct file *filp)
> > +{
> > +     struct crashlog_entry *entry;
> > +     struct pci_driver *pci_drv;
> > +     struct pmt_crashlog_priv *priv;
> > +
> > +     if (!capable(CAP_SYS_ADMIN))
> > +             return -EPERM;
> > +
> > +     entry = container_of(inode->i_cdev, struct crashlog_entry, cdev);
> > +     priv = entry->priv;
> > +     pci_drv = pci_dev_driver(priv->parent);
> > +
> > +     if (!pci_drv)
> > +             return -ENODEV;
> > +
> > +     filp->private_data = entry;
> > +     get_device(&priv->parent->dev);
> > +
> > +     if (!try_module_get(pci_drv->driver.owner)) {
> > +             put_device(&priv->parent->dev);
> > +             return -ENODEV;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int pmt_crashlog_release(struct inode *inode, struct file *filp)
> > +{
> > +     struct crashlog_entry *entry = filp->private_data;
> > +     struct pmt_crashlog_priv *priv;
> > +     struct pci_driver *pci_drv;
> > +
> > +     priv = entry->priv;
> > +     pci_drv = pci_dev_driver(priv->parent);
> > +
> > +     put_device(&priv->parent->dev);
> > +     module_put(pci_drv->driver.owner);
> > +
> > +     return 0;
> > +}
> > +
> > +static int
> > +pmt_crashlog_mmap(struct file *filp, struct vm_area_struct *vma)
> > +{
> > +     struct crashlog_entry *entry = filp->private_data;
> > +     struct pmt_crashlog_priv *priv;
> > +     unsigned long phys = entry->crashlog_data;
> > +     unsigned long pfn = PFN_DOWN(phys);
> > +     unsigned long vsize = vma->vm_end - vma->vm_start;
> > +     unsigned long psize;
> > +
> > +     if ((vma->vm_flags & VM_WRITE) ||
> > +         (vma->vm_flags & VM_MAYWRITE))
> > +             return -EPERM;
> > +
> > +     priv = entry->priv;
> > +
> > +     if (!entry->crashlog_data_size) {
> > +             dev_err(priv->dev, "Crashlog data not accessible\n");
> > +             return -EAGAIN;
> > +     }
> > +
> > +     psize = (PFN_UP(entry->crashlog_data + entry->crashlog_data_size) - pfn) *
> > +             PAGE_SIZE;
> > +     if (vsize > psize) {
> > +             dev_err(priv->dev, "Requested mmap size is too large\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> > +     if (io_remap_pfn_range(vma, vma->vm_start, pfn,
> > +             vsize, vma->vm_page_prot))
> > +             return -EAGAIN;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct file_operations pmt_crashlog_fops = {
> > +     .owner =        THIS_MODULE,
> > +     .open =         pmt_crashlog_open,
> > +     .mmap =         pmt_crashlog_mmap,
>
> mmap but no read, I guess read may be emulated through mmap,
> is that the case ?
>
> I can see sysadmins wanting to be able to do a simple cat
> on this file to get the logs (including headers), so if
> the kernel-core does not emulate read in this case, you
> should really add read support I guess.

So first the contents of the crashlog are not really human readable,
so it is not likely that they would "cat" the contents. Also I don't
believe it is a very common thing to provide read access if we don't
know the memory layout of the region. If you take a look at the
handling for resourceN in
pci_create_attr(https://elixir.bootlin.com/linux/v5.9-rc4/source/drivers/pci/pci-sysfs.c#L1127)
it looks like it does something similar where it only provides mmap
for MMIO access.

> Also how big are these files ?  sysfs also supports binary
> files, so unless these files are huge / this is really
> performance critical it may make more sense to just add
> a binary sysfs attr for this and get rid of the whole chardev
> all together.

So for the file we are looking at the minimum of a page up to multiple
pages of data. It largely depends on how much information is collected
by the crashlog agent. I can take a look and see if we can do it. Odds
are it shouldn't be too different from how resourceN is done for the
PCI devices.

> > +     .release =      pmt_crashlog_release,
> > +};
> > +
> > +/*
> > + * sysfs
> > + */
> > +static ssize_t
> > +guid_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +     struct crashlog_entry *entry;
> > +
> > +     entry = dev_get_drvdata(dev);
> > +
> > +     return sprintf(buf, "0x%x\n", entry->header.guid);
> > +}
> > +static DEVICE_ATTR_RO(guid);
> > +
> > +static ssize_t size_show(struct device *dev, struct device_attribute *attr,
> > +                      char *buf)
> > +{
> > +     struct crashlog_entry *entry;
> > +
> > +     entry = dev_get_drvdata(dev);
> > +
> > +     return sprintf(buf, "%lu\n", entry->crashlog_data_size);
> > +}
> > +static DEVICE_ATTR_RO(size);
> > +
> > +static ssize_t
> > +offset_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +     struct crashlog_entry *entry;
> > +
> > +     entry = dev_get_drvdata(dev);
> > +
> > +     return sprintf(buf, "%lu\n", offset_in_page(entry->crashlog_data));
> > +}
> > +static DEVICE_ATTR_RO(offset);
> > +
> > +static ssize_t
> > +enable_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +     struct crashlog_entry *entry;
> > +     int enabled;
> > +
> > +     entry = dev_get_drvdata(dev);
> > +     enabled = !pmt_crashlog_disabled(entry);
> > +
> > +     return sprintf(buf, "%d\n", enabled);
> > +}
> > +
> > +static ssize_t
> > +enable_store(struct device *dev, struct device_attribute *attr,
> > +         const char *buf, size_t count)
> > +{
> > +     struct crashlog_entry *entry;
> > +     bool enabled;
> > +     int result;
> > +
> > +     entry = dev_get_drvdata(dev);
> > +
> > +     result = kstrtobool(buf, &enabled);
> > +     if (result)
> > +             return result;
> > +
> > +     pmt_crashlog_set_disable(entry, !enabled);
> > +
> > +     return strnlen(buf, count);
> > +}
> > +static DEVICE_ATTR_RW(enable);
> > +
> > +static ssize_t
> > +trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +     struct crashlog_entry *entry;
> > +     int trigger;
> > +
> > +     entry = dev_get_drvdata(dev);
> > +     trigger = pmt_crashlog_complete(entry);
> > +
> > +     return sprintf(buf, "%d\n", trigger);
> > +}
> > +
> > +static ssize_t
> > +trigger_store(struct device *dev, struct device_attribute *attr,
> > +         const char *buf, size_t count)
> > +{
> > +     struct crashlog_entry *entry;
> > +     bool trigger;
> > +     int result;
> > +
> > +     entry = dev_get_drvdata(dev);
> > +
> > +     result = kstrtobool(buf, &trigger);
> > +     if (result)
> > +             return result;
> > +
> > +     if (trigger) {
> > +             /* we cannot trigger a new crash if one is still pending */
> > +             if (pmt_crashlog_complete(entry))
> > +                     return -EEXIST;
> > +
> > +             /* if device is currently disabled, return busy */
> > +             if (pmt_crashlog_disabled(entry))
> > +                     return -EBUSY;
> > +
> > +             pmt_crashlog_set_execute(entry);
> > +     } else {
> > +             pmt_crashlog_set_clear(entry);
> > +     }
> > +
> > +     return strnlen(buf, count);
> > +}
> > +static DEVICE_ATTR_RW(trigger);
> > +
> > +static struct attribute *pmt_crashlog_attrs[] = {
> > +     &dev_attr_guid.attr,
> > +     &dev_attr_size.attr,
> > +     &dev_attr_offset.attr,
> > +     &dev_attr_enable.attr,
> > +     &dev_attr_trigger.attr,
> > +     NULL
> > +};
> > +ATTRIBUTE_GROUPS(pmt_crashlog);
> > +
> > +static struct class pmt_crashlog_class = {
> > +     .name = "pmt_crashlog",
> > +     .owner = THIS_MODULE,
> > +     .dev_groups = pmt_crashlog_groups,
> > +};
> > +
> > +/*
> > + * initialization
> > + */
> > +static int pmt_crashlog_make_dev(struct pmt_crashlog_priv *priv,
> > +                              struct crashlog_entry *entry)
> > +{
> > +     struct device *dev;
> > +     int ret;
> > +
> > +     ret = alloc_chrdev_region(&entry->devt, 0, 1, DRV_NAME);
> > +     if (ret < 0) {
> > +             dev_err(priv->dev, "alloc_chrdev_region err: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     /* Create a character device for Samplers */
> > +     cdev_init(&entry->cdev, &pmt_crashlog_fops);
> > +
> > +     ret = cdev_add(&entry->cdev, entry->devt, 1);
> > +     if (ret) {
> > +             dev_err(priv->dev, "Could not add char dev\n");
> > +             return ret;
> > +     }
> > +
> > +     dev = device_create(&pmt_crashlog_class, &priv->parent->dev, entry->devt,
> > +                         entry, "%s%d", "crashlog", entry->devid);
> > +
> > +     if (IS_ERR(dev)) {
> > +             dev_err(priv->dev, "Could not create device node\n");
> > +             cdev_del(&entry->cdev);
> > +     }
> > +
> > +     return PTR_ERR_OR_ZERO(dev);
> > +}
> > +
> > +static void
> > +pmt_crashlog_populate_header(void __iomem *disc_offset,
> > +                          struct crashlog_header *header)
> > +{
> > +     u32 discovery_header = readl(disc_offset);
> > +
> > +     header->access_type = GET_ACCESS(discovery_header);
> > +     header->crash_type = GET_TYPE(discovery_header);
> > +     header->version = GET_VERSION(discovery_header);
> > +     header->guid = readl(disc_offset + GUID_OFFSET);
> > +     header->base_offset = readl(disc_offset + BASE_OFFSET);
> > +
> > +     /*
> > +      * For non-local access types the lower 3 bits of base offset
> > +      * contains the index of the base address register where the
> > +      * crashlogetry can be found.
> > +      */
> > +     header->bir = GET_BIR(header->base_offset);
> > +     header->base_offset ^= header->bir;
> > +
> > +     /* Size is measured in DWORDs */
> > +     header->size = readl(disc_offset + SIZE_OFFSET);
> > +}
> > +
> > +static int pmt_crashlog_add_entry(struct pmt_crashlog_priv *priv,
> > +                               struct crashlog_entry *entry)
> > +{
> > +     struct resource *res = entry->header_res;
> > +     int ret;
> > +
> > +     pmt_crashlog_populate_header(entry->disc_table, &entry->header);
> > +
> > +     /* Local access and BARID only for now */
> > +     switch (entry->header.access_type) {
> > +     case ACCESS_LOCAL:
> > +             dev_info(priv->dev, "access_type: LOCAL\n");
> > +             if (entry->header.bir) {
> > +                     dev_err(priv->dev,
> > +                             "Unsupported BAR index %d for access type %d\n",
> > +                             entry->header.bir, entry->header.access_type);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             entry->crashlog_data = res->start + resource_size(res) +
> > +                                    entry->header.base_offset;
> > +             break;
> > +
> > +     case ACCESS_BARID:
> > +             dev_info(priv->dev, "access_type: BARID\n");
> > +             entry->crashlog_data =
> > +                     priv->parent->resource[entry->header.bir].start +
> > +                     entry->header.base_offset;
> > +             break;
> > +
> > +     default:
> > +             dev_err(priv->dev, "Unsupported access type %d\n",
> > +                     entry->header.access_type);
> > +             return -EINVAL;
> > +     }
> > +
> > +     dev_info(priv->dev, "crashlod_data address: 0x%lx\n", entry->crashlog_data);
> > +
> > +     entry->crashlog_data_size = entry->header.size * 4;
> > +
> > +     if (entry->header.crash_type != CRASH_TYPE_OOBMSM) {
> > +             dev_err(priv->dev, "Unsupported crashlog header type %d\n",
> > +                     entry->header.crash_type);
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (entry->header.version != 0) {
> > +             dev_err(priv->dev, "Unsupported version value %d\n",
> > +                     entry->header.version);
> > +             return -EINVAL;
> > +     }
> > +
> > +     entry->ida = &crashlog_devid_ida;
> > +
> > +     entry->devid = ida_simple_get(entry->ida, 0, 0, GFP_KERNEL);
> > +     if (entry->devid < 0)
> > +             return entry->devid;
> > +
> > +     ret = pmt_crashlog_make_dev(priv, entry);
> > +     if (ret) {
> > +             ida_simple_remove(entry->ida, entry->devid);
> > +             return ret;
> > +     }
>
> Hmm wait, you are making one chardev per log entry ? Then just using
> binary sysfs attributes seems to make even more sense to me.

Yes we are required to create one per log entry as each one can be
accessed independently.

Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

[Alexander Duyck]

On Mon, Sep 14, 2020 at 11:07 AM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> On Mon, Sep 14, 2020 at 6:42 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi,
> >
> > On 9/11/20 9:45 PM, David E. Box wrote:
> > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > >
> > > Add support for the Intel Platform Monitoring Technology crashlog
> > > interface.  This interface provides a few sysfs values to allow for
> > > controlling the crashlog telemetry interface as well as a character driver
> > > to allow for mapping the crashlog memory region so that it can be accessed
> > > after a crashlog has been recorded.
> > >
> > > This driver is meant to only support the server version of the crashlog
> > > which is identified as crash_type 1 with a version of zero. Currently no
> > > other types are supported.
> > >
> > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > ---
> > >   .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
> > >   drivers/platform/x86/Kconfig                  |  10 +
> > >   drivers/platform/x86/Makefile                 |   1 +
> > >   drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
> > >   4 files changed, 665 insertions(+)
> > >   create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
> > >   create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-class-pmt_crashlog b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
> > > new file mode 100644
> > > index 000000000000..40fb4ff437a6
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
> > > @@ -0,0 +1,66 @@
> > > +What:                /sys/class/pmt_crashlog/
> > > +Date:                September 2020
> > > +KernelVersion:       5.10
> > > +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > +Description:
> > > +             The pmt_crashlog/ class directory contains information
> > > +             for devices that expose crashlog capabilities using the Intel
> > > +             Platform Monitoring Technology (PTM).
> > > +
> > > +What:                /sys/class/pmt_crashlog/crashlogX
> > > +Date:                September 2020
> > > +KernelVersion:       5.10
> > > +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > +Description:
> > > +             The crashlogX directory contains files for configuring an
> > > +             instance of a PMT crashlog device that can perform crash data
> > > +             recoring. Each crashlogX device has an associated
> > > +             /dev/crashlogX device node. This node can be opened and mapped
> > > +             to access the resulting crashlog data. The register layout for
> > > +             the log can be determined from an XML file of specified guid
> > > +             for the parent device.
> > > +
> > > +What:                /sys/class/pmt_crashlog/crashlogX/guid
> > > +Date:                September 2020
> > > +KernelVersion:       5.10
> > > +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > +Description:
> > > +             (RO) The guid for this crashlog device. The guid identifies the
> > > +             version of the XML file for the parent device that should be
> > > +             used to determine the register layout.
> > > +
> > > +What:                /sys/class/pmt_crashlog/crashlogX/size
> > > +Date:                September 2020
> > > +KernelVersion:       5.10
> > > +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > +Description:
> > > +             (RO) The length of the result buffer in bytes that corresponds
> > > +             to the mapping size for the /dev/crashlogX device node.
> > > +
> > > +What:                /sys/class/pmt_crashlog/crashlogX/offset
> > > +Date:                September 2020
> > > +KernelVersion:       5.10
> > > +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > +Description:
> > > +             (RO) The offset of the buffer in bytes that corresponds
> > > +             to the mapping for the /dev/crashlogX device node.
> > > +
> > > +What:                /sys/class/pmt_crashlog/crashlogX/enable
> > > +Date:                September 2020
> > > +KernelVersion:       5.10
> > > +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > +Description:
> > > +             (RW) Boolean value controlling if the crashlog functionality
> > > +             is enabled for the /dev/crashlogX device node.
> > > +
> > > +What:                /sys/class/pmt_crashlog/crashlogX/trigger
> > > +Date:                September 2020
> > > +KernelVersion:       5.10
> > > +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > +Description:
> > > +             (RW) Boolean value controlling  the triggering of the
> > > +             /dev/crashlogX device node. When read it provides data on if
> > > +             the crashlog has been triggered. When written to it can be
> > > +             used to either clear the current trigger by writing false, or
> > > +             to trigger a new event if the trigger is not currently set.
> > > +
> >
> > Both the pmt_crashlog and the attributes suggest that this is highly
> > Intel PMT specific. /sys/class/foo interfaces are generally speaking
> > meant to be generic interfaces.
> >
> > If this was defining a generic, vendor and implementation agnostic interface for
> > configuring / accessing crashlogs, then using a class would be fine, but that
> > is not the case, so I believe that this should not implement / register a class.
> >
> > Since the devices are instantiated through MFD there already is a
> > static sysfs-path which can be used to find the device in sysfs:
> > /sys/bus/platform/device/pmt_crashlog
> >
> > So you can register the sysfs attributes directly under the platform_device
> > and then userspace can easily find them, so there really is no need to
> > use a class here.
>
> I see. So we change the root directory from "/sys/class/pmt_crashlog/"
> to "/sys/bus/platform/device/pmt_crashlog" while retaining the same
> functionality. That should be workable.

So one issue as I see it is that if we were to change this then we
probably need to to change the telemetry functionality that was
recently accepted
(https://lore.kernel.org/lkml/20200819180255.11770-1-david.e.box@linux.intel.com/)
as well. The general idea with using the /sys/class/pmt_crashlog/
approach was to keep things consistent with how the pmt_telemetry was
being accessed. So if we change this then we end up with very
different interfaces for the two very similar pieces of functionality.
So ideally we would want to change both telemetry and crashlog to
function the same way.

Do you have any good examples of anything that has done something
similar? From what I can tell it looks like we need to clean up the
naming to drop the ".%d.auto" for the bus directory names and then
look at adding a folder to handle all of the instances of either
telemetry or crashlog, assuming we follow the reg-dummy or serial8250
model.

Similarly the crashlog and telemetry both rely on similar mechanisms
to display the MMIO region containing the data. I still need to spend
some more time looking into what is involved in switching from a char
device to a binary sysfs, but I think with the example I found earlier
of the resourceN bit from the PCI sysfs I can probably make that work
for both cases.

Thanks.

- Alex

Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

[Hans de Goede]

Hi,

On 9/14/20 8:07 PM, Alexander Duyck wrote:
> On Mon, Sep 14, 2020 at 6:42 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 9/11/20 9:45 PM, David E. Box wrote:
>>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>
>>> Add support for the Intel Platform Monitoring Technology crashlog
>>> interface.  This interface provides a few sysfs values to allow for
>>> controlling the crashlog telemetry interface as well as a character driver
>>> to allow for mapping the crashlog memory region so that it can be accessed
>>> after a crashlog has been recorded.
>>>
>>> This driver is meant to only support the server version of the crashlog
>>> which is identified as crash_type 1 with a version of zero. Currently no
>>> other types are supported.
>>>
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
>>> ---
>>>    .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
>>>    drivers/platform/x86/Kconfig                  |  10 +
>>>    drivers/platform/x86/Makefile                 |   1 +
>>>    drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
>>>    4 files changed, 665 insertions(+)
>>>    create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
>>>    create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-pmt_crashlog b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
>>> new file mode 100644
>>> index 000000000000..40fb4ff437a6
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
>>> @@ -0,0 +1,66 @@
>>> +What:                /sys/class/pmt_crashlog/
>>> +Date:                September 2020
>>> +KernelVersion:       5.10
>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>> +Description:
>>> +             The pmt_crashlog/ class directory contains information
>>> +             for devices that expose crashlog capabilities using the Intel
>>> +             Platform Monitoring Technology (PTM).
>>> +
>>> +What:                /sys/class/pmt_crashlog/crashlogX
>>> +Date:                September 2020
>>> +KernelVersion:       5.10
>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>> +Description:
>>> +             The crashlogX directory contains files for configuring an
>>> +             instance of a PMT crashlog device that can perform crash data
>>> +             recoring. Each crashlogX device has an associated
>>> +             /dev/crashlogX device node. This node can be opened and mapped
>>> +             to access the resulting crashlog data. The register layout for
>>> +             the log can be determined from an XML file of specified guid
>>> +             for the parent device.
>>> +
>>> +What:                /sys/class/pmt_crashlog/crashlogX/guid
>>> +Date:                September 2020
>>> +KernelVersion:       5.10
>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>> +Description:
>>> +             (RO) The guid for this crashlog device. The guid identifies the
>>> +             version of the XML file for the parent device that should be
>>> +             used to determine the register layout.
>>> +
>>> +What:                /sys/class/pmt_crashlog/crashlogX/size
>>> +Date:                September 2020
>>> +KernelVersion:       5.10
>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>> +Description:
>>> +             (RO) The length of the result buffer in bytes that corresponds
>>> +             to the mapping size for the /dev/crashlogX device node.
>>> +
>>> +What:                /sys/class/pmt_crashlog/crashlogX/offset
>>> +Date:                September 2020
>>> +KernelVersion:       5.10
>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>> +Description:
>>> +             (RO) The offset of the buffer in bytes that corresponds
>>> +             to the mapping for the /dev/crashlogX device node.
>>> +
>>> +What:                /sys/class/pmt_crashlog/crashlogX/enable
>>> +Date:                September 2020
>>> +KernelVersion:       5.10
>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>> +Description:
>>> +             (RW) Boolean value controlling if the crashlog functionality
>>> +             is enabled for the /dev/crashlogX device node.
>>> +
>>> +What:                /sys/class/pmt_crashlog/crashlogX/trigger
>>> +Date:                September 2020
>>> +KernelVersion:       5.10
>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>> +Description:
>>> +             (RW) Boolean value controlling  the triggering of the
>>> +             /dev/crashlogX device node. When read it provides data on if
>>> +             the crashlog has been triggered. When written to it can be
>>> +             used to either clear the current trigger by writing false, or
>>> +             to trigger a new event if the trigger is not currently set.
>>> +
>>
>> Both the pmt_crashlog and the attributes suggest that this is highly
>> Intel PMT specific. /sys/class/foo interfaces are generally speaking
>> meant to be generic interfaces.
>>
>> If this was defining a generic, vendor and implementation agnostic interface for
>> configuring / accessing crashlogs, then using a class would be fine, but that
>> is not the case, so I believe that this should not implement / register a class.
>>
>> Since the devices are instantiated through MFD there already is a
>> static sysfs-path which can be used to find the device in sysfs:
>> /sys/bus/platform/device/pmt_crashlog
>>
>> So you can register the sysfs attributes directly under the platform_device
>> and then userspace can easily find them, so there really is no need to
>> use a class here.
> 
> I see. So we change the root directory from "/sys/class/pmt_crashlog/"
> to "/sys/bus/platform/device/pmt_crashlog" while retaining the same
> functionality. That should be workable.

Ack.

<snip>

>>> +static const struct file_operations pmt_crashlog_fops = {
>>> +     .owner =        THIS_MODULE,
>>> +     .open =         pmt_crashlog_open,
>>> +     .mmap =         pmt_crashlog_mmap,
>>
>> mmap but no read, I guess read may be emulated through mmap,
>> is that the case ?
>>
>> I can see sysadmins wanting to be able to do a simple cat
>> on this file to get the logs (including headers), so if
>> the kernel-core does not emulate read in this case, you
>> should really add read support I guess.
> 
> So first the contents of the crashlog are not really human readable,
> so it is not likely that they would "cat" the contents.

Sorry, I was not really clear there, what I meant is a sysadmin doing
something like this:

cat /sys/.../crashlog-file > /mnt/external-usb-disk/server-foo-bar-crashlog20200917

So that they can easily save the crashlog for later reference without
needing to install special tools.

> Also I don't
> believe it is a very common thing to provide read access if we don't
> know the memory layout of the region. If you take a look at the
> handling for resourceN in
> pci_create_attr(https://elixir.bootlin.com/linux/v5.9-rc4/source/drivers/pci/pci-sysfs.c#L1127)
> it looks like it does something similar where it only provides mmap
> for MMIO access.

That was meant as a way to allow doing hardware-drivers in userspace
(think old userspace modesetting Xorg/xfree86) without needing to
call iopl and on non-x86 platforms which don't have iopl.


> 
>> Also how big are these files ?  sysfs also supports binary
>> files, so unless these files are huge / this is really
>> performance critical it may make more sense to just add
>> a binary sysfs attr for this and get rid of the whole chardev
>> all together.
> 
> So for the file we are looking at the minimum of a page up to multiple
> pages of data. It largely depends on how much information is collected
> by the crashlog agent. I can take a look and see if we can do it. Odds
> are it shouldn't be too different from how resourceN is done for the
> PCI devices.

Ok, a few pages of data should not be an issue for a binary sysfs
file at all.

<snip>

>>> +     entry->devid = ida_simple_get(entry->ida, 0, 0, GFP_KERNEL);
>>> +     if (entry->devid < 0)
>>> +             return entry->devid;
>>> +
>>> +     ret = pmt_crashlog_make_dev(priv, entry);
>>> +     if (ret) {
>>> +             ida_simple_remove(entry->ida, entry->devid);
>>> +             return ret;
>>> +     }
>>
>> Hmm wait, you are making one chardev per log entry ? Then just using
>> binary sysfs attributes seems to make even more sense to me.
> 
> Yes we are required to create one per log entry as each one can be
> accessed independently.

That is fine, but then at least to me, using sysfs binary files, seems to make
a lot more sense then creating multiple char devices for this.

Regards,

Hans

Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

[Hans de Goede]

Hi,

On 9/15/20 12:35 AM, Alexander Duyck wrote:
> On Mon, Sep 14, 2020 at 11:07 AM Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>>
>> On Mon, Sep 14, 2020 at 6:42 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>> On 9/11/20 9:45 PM, David E. Box wrote:
>>>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>
>>>> Add support for the Intel Platform Monitoring Technology crashlog
>>>> interface.  This interface provides a few sysfs values to allow for
>>>> controlling the crashlog telemetry interface as well as a character driver
>>>> to allow for mapping the crashlog memory region so that it can be accessed
>>>> after a crashlog has been recorded.
>>>>
>>>> This driver is meant to only support the server version of the crashlog
>>>> which is identified as crash_type 1 with a version of zero. Currently no
>>>> other types are supported.
>>>>
>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
>>>> ---
>>>>    .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
>>>>    drivers/platform/x86/Kconfig                  |  10 +
>>>>    drivers/platform/x86/Makefile                 |   1 +
>>>>    drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
>>>>    4 files changed, 665 insertions(+)
>>>>    create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
>>>>    create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-class-pmt_crashlog b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
>>>> new file mode 100644
>>>> index 000000000000..40fb4ff437a6
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
>>>> @@ -0,0 +1,66 @@
>>>> +What:                /sys/class/pmt_crashlog/
>>>> +Date:                September 2020
>>>> +KernelVersion:       5.10
>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>> +Description:
>>>> +             The pmt_crashlog/ class directory contains information
>>>> +             for devices that expose crashlog capabilities using the Intel
>>>> +             Platform Monitoring Technology (PTM).
>>>> +
>>>> +What:                /sys/class/pmt_crashlog/crashlogX
>>>> +Date:                September 2020
>>>> +KernelVersion:       5.10
>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>> +Description:
>>>> +             The crashlogX directory contains files for configuring an
>>>> +             instance of a PMT crashlog device that can perform crash data
>>>> +             recoring. Each crashlogX device has an associated
>>>> +             /dev/crashlogX device node. This node can be opened and mapped
>>>> +             to access the resulting crashlog data. The register layout for
>>>> +             the log can be determined from an XML file of specified guid
>>>> +             for the parent device.
>>>> +
>>>> +What:                /sys/class/pmt_crashlog/crashlogX/guid
>>>> +Date:                September 2020
>>>> +KernelVersion:       5.10
>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>> +Description:
>>>> +             (RO) The guid for this crashlog device. The guid identifies the
>>>> +             version of the XML file for the parent device that should be
>>>> +             used to determine the register layout.
>>>> +
>>>> +What:                /sys/class/pmt_crashlog/crashlogX/size
>>>> +Date:                September 2020
>>>> +KernelVersion:       5.10
>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>> +Description:
>>>> +             (RO) The length of the result buffer in bytes that corresponds
>>>> +             to the mapping size for the /dev/crashlogX device node.
>>>> +
>>>> +What:                /sys/class/pmt_crashlog/crashlogX/offset
>>>> +Date:                September 2020
>>>> +KernelVersion:       5.10
>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>> +Description:
>>>> +             (RO) The offset of the buffer in bytes that corresponds
>>>> +             to the mapping for the /dev/crashlogX device node.
>>>> +
>>>> +What:                /sys/class/pmt_crashlog/crashlogX/enable
>>>> +Date:                September 2020
>>>> +KernelVersion:       5.10
>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>> +Description:
>>>> +             (RW) Boolean value controlling if the crashlog functionality
>>>> +             is enabled for the /dev/crashlogX device node.
>>>> +
>>>> +What:                /sys/class/pmt_crashlog/crashlogX/trigger
>>>> +Date:                September 2020
>>>> +KernelVersion:       5.10
>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>> +Description:
>>>> +             (RW) Boolean value controlling  the triggering of the
>>>> +             /dev/crashlogX device node. When read it provides data on if
>>>> +             the crashlog has been triggered. When written to it can be
>>>> +             used to either clear the current trigger by writing false, or
>>>> +             to trigger a new event if the trigger is not currently set.
>>>> +
>>>
>>> Both the pmt_crashlog and the attributes suggest that this is highly
>>> Intel PMT specific. /sys/class/foo interfaces are generally speaking
>>> meant to be generic interfaces.
>>>
>>> If this was defining a generic, vendor and implementation agnostic interface for
>>> configuring / accessing crashlogs, then using a class would be fine, but that
>>> is not the case, so I believe that this should not implement / register a class.
>>>
>>> Since the devices are instantiated through MFD there already is a
>>> static sysfs-path which can be used to find the device in sysfs:
>>> /sys/bus/platform/device/pmt_crashlog
>>>
>>> So you can register the sysfs attributes directly under the platform_device
>>> and then userspace can easily find them, so there really is no need to
>>> use a class here.
>>
>> I see. So we change the root directory from "/sys/class/pmt_crashlog/"
>> to "/sys/bus/platform/device/pmt_crashlog" while retaining the same
>> functionality. That should be workable.
> 
> So one issue as I see it is that if we were to change this then we
> probably need to to change the telemetry functionality that was
> recently accepted
> (https://lore.kernel.org/lkml/20200819180255.11770-1-david.e.box@linux.intel.com/)
> as well. The general idea with using the /sys/class/pmt_crashlog/
> approach was to keep things consistent with how the pmt_telemetry was
> being accessed. So if we change this then we end up with very
> different interfaces for the two very similar pieces of functionality.
> So ideally we would want to change both telemetry and crashlog to
> function the same way.

I agree that the telemetry interface should be changed in a similar way.

Luckily it seems that this is not in Linus' tree yet and I'm also not
seeing it in next yet, e.g. :
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/platform/x86/intel_pmt_telemetry.c
does not exist.

So we seem to still have time to also get the telemetry driver userspace API
fixed too.

I see that Andy gave his Reviewed-by for the intel_pmt_telemetry.c code.

Andy, I have some concerns about the userspace API choices made here,
see my earlier review of this patch. Do you agree with my suggestions,
or do you think it would be ok to move forward with the telemetry and
now also the crashlog API each registering their own private class
under /sys/class ?

AFAIK classes are supposed to be generic and not driver-private, so
that seems wrong to me.  Also PMC is Intel specific and vendor specific
stuff really does not belong under /sys/class AFAIK ?

> Do you have any good examples of anything that has done something
> similar? From what I can tell it looks like we need to clean up the
> naming to drop the ".%d.auto" for the bus directory names

Assuming there will only be one of each platform-device, then you
can just replace the PLATFORM_DEVID_AUTO param to devm_mfd_add_devices()
with PLATFORM_DEVID_NONE and the .%d.auto will go away.

> and then
> look at adding a folder to handle all of the instances of either
> telemetry or crashlog, assuming we follow the reg-dummy or serial8250
> model.

So there can be multiple instances, you mean like the multiple chardevs
you add now, or can there be multiple platform-devices of the same
time instantiated through the MFD code ?

If you mean like the multiple chardevs, then yes you could add a folder
for the binary sysfs attributes replacing those, or register them
with a dynamic name with a number appended to the name.

> Similarly the crashlog and telemetry both rely on similar mechanisms
> to display the MMIO region containing the data. I still need to spend
> some more time looking into what is involved in switching from a char
> device to a binary sysfs, but I think with the example I found earlier
> of the resourceN bit from the PCI sysfs I can probably make that work
> for both cases.

I'm not sure that the PCI sysfs io resources are the best example,
as mentioned those mmap to actual memory-mapped io, which is somewhat
special.

For a simpler example see drivers/platform/x86/wmi-bmof.c.

The way normal sysfs binary attributes work is that they
have a read method much like the read method on a block device
where an offset into the file gets passed. So you just copy_to_user
the requested amount of data starting at offset from the in-kernel
mapped buffer to the user buffer:

static ssize_t
read_bmof(struct file *filp, struct kobject *kobj,
          struct bin_attribute *attr,
          char *buf, loff_t off, size_t count)
{
         struct bmof_priv *priv =
                 container_of(attr, struct bmof_priv, bmof_bin_attr);

         if (off < 0)
                 return -EINVAL;

         if (off >= priv->bmofdata->buffer.length)
                 return 0;

         if (count > priv->bmofdata->buffer.length - off)
                 count = priv->bmofdata->buffer.length - off;

         memcpy(buf, priv->bmofdata->buffer.pointer + off, count);
         return count;
}

The wmi_bmof code also shows how you can dynamically create and
add binary sysfs attr which allows you to add a %d postfix to the
name. Note you should always dynamically create binary sysfs attr.

There are some old static initializers for these, but AFAIK those
lead to lockdep issues.

Regards,

Hans

Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

[Alexander Duyck]

On Thu, Sep 17, 2020 at 5:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 9/15/20 12:35 AM, Alexander Duyck wrote:
> > On Mon, Sep 14, 2020 at 11:07 AM Alexander Duyck
> > <alexander.duyck@gmail.com> wrote:
> >>
> >> On Mon, Sep 14, 2020 at 6:42 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On 9/11/20 9:45 PM, David E. Box wrote:
> >>>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>>
> >>>> Add support for the Intel Platform Monitoring Technology crashlog
> >>>> interface.  This interface provides a few sysfs values to allow for
> >>>> controlling the crashlog telemetry interface as well as a character driver
> >>>> to allow for mapping the crashlog memory region so that it can be accessed
> >>>> after a crashlog has been recorded.
> >>>>
> >>>> This driver is meant to only support the server version of the crashlog
> >>>> which is identified as crash_type 1 with a version of zero. Currently no
> >>>> other types are supported.
> >>>>
> >>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> >>>> ---
> >>>>    .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
> >>>>    drivers/platform/x86/Kconfig                  |  10 +
> >>>>    drivers/platform/x86/Makefile                 |   1 +
> >>>>    drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
> >>>>    4 files changed, 665 insertions(+)
> >>>>    create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
> >>>>    create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c
> >>>>
> >>>> diff --git a/Documentation/ABI/testing/sysfs-class-pmt_crashlog b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
> >>>> new file mode 100644
> >>>> index 000000000000..40fb4ff437a6
> >>>> --- /dev/null
> >>>> +++ b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
> >>>> @@ -0,0 +1,66 @@
> >>>> +What:                /sys/class/pmt_crashlog/
> >>>> +Date:                September 2020
> >>>> +KernelVersion:       5.10
> >>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>> +Description:
> >>>> +             The pmt_crashlog/ class directory contains information
> >>>> +             for devices that expose crashlog capabilities using the Intel
> >>>> +             Platform Monitoring Technology (PTM).
> >>>> +
> >>>> +What:                /sys/class/pmt_crashlog/crashlogX
> >>>> +Date:                September 2020
> >>>> +KernelVersion:       5.10
> >>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>> +Description:
> >>>> +             The crashlogX directory contains files for configuring an
> >>>> +             instance of a PMT crashlog device that can perform crash data
> >>>> +             recoring. Each crashlogX device has an associated
> >>>> +             /dev/crashlogX device node. This node can be opened and mapped
> >>>> +             to access the resulting crashlog data. The register layout for
> >>>> +             the log can be determined from an XML file of specified guid
> >>>> +             for the parent device.
> >>>> +
> >>>> +What:                /sys/class/pmt_crashlog/crashlogX/guid
> >>>> +Date:                September 2020
> >>>> +KernelVersion:       5.10
> >>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>> +Description:
> >>>> +             (RO) The guid for this crashlog device. The guid identifies the
> >>>> +             version of the XML file for the parent device that should be
> >>>> +             used to determine the register layout.
> >>>> +
> >>>> +What:                /sys/class/pmt_crashlog/crashlogX/size
> >>>> +Date:                September 2020
> >>>> +KernelVersion:       5.10
> >>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>> +Description:
> >>>> +             (RO) The length of the result buffer in bytes that corresponds
> >>>> +             to the mapping size for the /dev/crashlogX device node.
> >>>> +
> >>>> +What:                /sys/class/pmt_crashlog/crashlogX/offset
> >>>> +Date:                September 2020
> >>>> +KernelVersion:       5.10
> >>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>> +Description:
> >>>> +             (RO) The offset of the buffer in bytes that corresponds
> >>>> +             to the mapping for the /dev/crashlogX device node.
> >>>> +
> >>>> +What:                /sys/class/pmt_crashlog/crashlogX/enable
> >>>> +Date:                September 2020
> >>>> +KernelVersion:       5.10
> >>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>> +Description:
> >>>> +             (RW) Boolean value controlling if the crashlog functionality
> >>>> +             is enabled for the /dev/crashlogX device node.
> >>>> +
> >>>> +What:                /sys/class/pmt_crashlog/crashlogX/trigger
> >>>> +Date:                September 2020
> >>>> +KernelVersion:       5.10
> >>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>> +Description:
> >>>> +             (RW) Boolean value controlling  the triggering of the
> >>>> +             /dev/crashlogX device node. When read it provides data on if
> >>>> +             the crashlog has been triggered. When written to it can be
> >>>> +             used to either clear the current trigger by writing false, or
> >>>> +             to trigger a new event if the trigger is not currently set.
> >>>> +
> >>>
> >>> Both the pmt_crashlog and the attributes suggest that this is highly
> >>> Intel PMT specific. /sys/class/foo interfaces are generally speaking
> >>> meant to be generic interfaces.
> >>>
> >>> If this was defining a generic, vendor and implementation agnostic interface for
> >>> configuring / accessing crashlogs, then using a class would be fine, but that
> >>> is not the case, so I believe that this should not implement / register a class.
> >>>
> >>> Since the devices are instantiated through MFD there already is a
> >>> static sysfs-path which can be used to find the device in sysfs:
> >>> /sys/bus/platform/device/pmt_crashlog
> >>>
> >>> So you can register the sysfs attributes directly under the platform_device
> >>> and then userspace can easily find them, so there really is no need to
> >>> use a class here.
> >>
> >> I see. So we change the root directory from "/sys/class/pmt_crashlog/"
> >> to "/sys/bus/platform/device/pmt_crashlog" while retaining the same
> >> functionality. That should be workable.
> >
> > So one issue as I see it is that if we were to change this then we
> > probably need to to change the telemetry functionality that was
> > recently accepted
> > (https://lore.kernel.org/lkml/20200819180255.11770-1-david.e.box@linux.intel.com/)
> > as well. The general idea with using the /sys/class/pmt_crashlog/
> > approach was to keep things consistent with how the pmt_telemetry was
> > being accessed. So if we change this then we end up with very
> > different interfaces for the two very similar pieces of functionality.
> > So ideally we would want to change both telemetry and crashlog to
> > function the same way.
>
> I agree that the telemetry interface should be changed in a similar way.
>
> Luckily it seems that this is not in Linus' tree yet and I'm also not
> seeing it in next yet, e.g. :
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/platform/x86/intel_pmt_telemetry.c
> does not exist.
>
> So we seem to still have time to also get the telemetry driver userspace API
> fixed too.
>
> I see that Andy gave his Reviewed-by for the intel_pmt_telemetry.c code.
>
> Andy, I have some concerns about the userspace API choices made here,
> see my earlier review of this patch. Do you agree with my suggestions,
> or do you think it would be ok to move forward with the telemetry and
> now also the crashlog API each registering their own private class
> under /sys/class ?
>
> AFAIK classes are supposed to be generic and not driver-private, so
> that seems wrong to me.  Also PMC is Intel specific and vendor specific
> stuff really does not belong under /sys/class AFAIK ?
>
> > Do you have any good examples of anything that has done something
> > similar? From what I can tell it looks like we need to clean up the
> > naming to drop the ".%d.auto" for the bus directory names
>
> Assuming there will only be one of each platform-device, then you
> can just replace the PLATFORM_DEVID_AUTO param to devm_mfd_add_devices()
> with PLATFORM_DEVID_NONE and the .%d.auto will go away.

We will have multiples of each platform device. So for example we can
have multiple OOBMSM in each system and each OOBMSM may have multiple
telemetry regions and maybe one crashlog.

> > and then
> > look at adding a folder to handle all of the instances of either
> > telemetry or crashlog, assuming we follow the reg-dummy or serial8250
> > model.
>
> So there can be multiple instances, you mean like the multiple chardevs
> you add now, or can there be multiple platform-devices of the same
> time instantiated through the MFD code ?
>
> If you mean like the multiple chardevs, then yes you could add a folder
> for the binary sysfs attributes replacing those, or register them
> with a dynamic name with a number appended to the name.

In addition to just the binary sysfs we need to expose several other
fields including the GUID, the size, and controls for enabling,
disabling, and either triggering or checking to see if the crashlog
has already been triggered. As such we would end up with a folder per
device and the binary sysfs would probably be living in the folder.

> > Similarly the crashlog and telemetry both rely on similar mechanisms
> > to display the MMIO region containing the data. I still need to spend
> > some more time looking into what is involved in switching from a char
> > device to a binary sysfs, but I think with the example I found earlier
> > of the resourceN bit from the PCI sysfs I can probably make that work
> > for both cases.
>
> I'm not sure that the PCI sysfs io resources are the best example,
> as mentioned those mmap to actual memory-mapped io, which is somewhat
> special.

The reason why I bring them up is because there are cases for
telemetry where the applications will likely want to be able to just
memory map the region and poll on certain statistics. So if nothing
else we may end up supporting both a mmap and a read option.

> For a simpler example see drivers/platform/x86/wmi-bmof.c.
>
> The way normal sysfs binary attributes work is that they
> have a read method much like the read method on a block device
> where an offset into the file gets passed. So you just copy_to_user
> the requested amount of data starting at offset from the in-kernel
> mapped buffer to the user buffer:
>
> static ssize_t
> read_bmof(struct file *filp, struct kobject *kobj,
>           struct bin_attribute *attr,
>           char *buf, loff_t off, size_t count)
> {
>          struct bmof_priv *priv =
>                  container_of(attr, struct bmof_priv, bmof_bin_attr);
>
>          if (off < 0)
>                  return -EINVAL;
>
>          if (off >= priv->bmofdata->buffer.length)
>                  return 0;
>
>          if (count > priv->bmofdata->buffer.length - off)
>                  count = priv->bmofdata->buffer.length - off;
>
>          memcpy(buf, priv->bmofdata->buffer.pointer + off, count);
>          return count;
> }
>
> The wmi_bmof code also shows how you can dynamically create and
> add binary sysfs attr which allows you to add a %d postfix to the
> name. Note you should always dynamically create binary sysfs attr.
>
> There are some old static initializers for these, but AFAIK those
> lead to lockdep issues.
>
> Regards,
>
> Hans

The binary sysfs isn't that much of a concern for me other than the
fact that it eliminates the character devices. The
cdev_add/device_create path is fairly simple to put together and
worked for the setup. However in switching to a binary sysfs I need to
create something similar to the folder that was created before and
then insert the binary sysfs file there. I have only done a bit of
sysfs work so I am not familiar with what is involved in setting that
up and need to spend some time trying to grok the code.

Thanks.

- Alex

Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

[Alexey Budankov]

Hi,

Thanks for the patches.

On 11.09.2020 22:45, David E. Box wrote:
> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 
> Add support for the Intel Platform Monitoring Technology crashlog
> interface.  This interface provides a few sysfs values to allow for
> controlling the crashlog telemetry interface as well as a character driver
> to allow for mapping the crashlog memory region so that it can be accessed
> after a crashlog has been recorded.
> 
> This driver is meant to only support the server version of the crashlog
> which is identified as crash_type 1 with a version of zero. Currently no
> other types are supported.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
>  drivers/platform/x86/Kconfig                  |  10 +
>  drivers/platform/x86/Makefile                 |   1 +
>  drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
>  4 files changed, 665 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
>  create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c

<SNIP>

> +
> +/*
> + * devfs
> + */
> +static int pmt_crashlog_open(struct inode *inode, struct file *filp)
> +{
> +	struct crashlog_entry *entry;
> +	struct pci_driver *pci_drv;
> +	struct pmt_crashlog_priv *priv;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;

Will not this above still block access to /dev/crashlogX for admin_group users
in case root configured access e.g. similar to this:

ls -alh /dev/
crw-rw----.  1 root admin_group      1,   9 Sep 15 18:28 crashlogX

If yes then that capable() check is probably superfluous and
should be avoided in order not to block access to PMT data.

Could you please clarify or comment?

Thanks,
Alexei  

Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

[Hans de Goede]

Hi,

On 9/17/20 11:35 PM, Alexander Duyck wrote:
> On Thu, Sep 17, 2020 at 5:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 9/15/20 12:35 AM, Alexander Duyck wrote:
>>> On Mon, Sep 14, 2020 at 11:07 AM Alexander Duyck
>>> <alexander.duyck@gmail.com> wrote:
>>>>
>>>> On Mon, Sep 14, 2020 at 6:42 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 9/11/20 9:45 PM, David E. Box wrote:
>>>>>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>>
>>>>>> Add support for the Intel Platform Monitoring Technology crashlog
>>>>>> interface.  This interface provides a few sysfs values to allow for
>>>>>> controlling the crashlog telemetry interface as well as a character driver
>>>>>> to allow for mapping the crashlog memory region so that it can be accessed
>>>>>> after a crashlog has been recorded.
>>>>>>
>>>>>> This driver is meant to only support the server version of the crashlog
>>>>>> which is identified as crash_type 1 with a version of zero. Currently no
>>>>>> other types are supported.
>>>>>>
>>>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
>>>>>> ---
>>>>>>     .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
>>>>>>     drivers/platform/x86/Kconfig                  |  10 +
>>>>>>     drivers/platform/x86/Makefile                 |   1 +
>>>>>>     drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
>>>>>>     4 files changed, 665 insertions(+)
>>>>>>     create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
>>>>>>     create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c
>>>>>>
>>>>>> diff --git a/Documentation/ABI/testing/sysfs-class-pmt_crashlog b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
>>>>>> new file mode 100644
>>>>>> index 000000000000..40fb4ff437a6
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
>>>>>> @@ -0,0 +1,66 @@
>>>>>> +What:                /sys/class/pmt_crashlog/
>>>>>> +Date:                September 2020
>>>>>> +KernelVersion:       5.10
>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> +Description:
>>>>>> +             The pmt_crashlog/ class directory contains information
>>>>>> +             for devices that expose crashlog capabilities using the Intel
>>>>>> +             Platform Monitoring Technology (PTM).
>>>>>> +
>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX
>>>>>> +Date:                September 2020
>>>>>> +KernelVersion:       5.10
>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> +Description:
>>>>>> +             The crashlogX directory contains files for configuring an
>>>>>> +             instance of a PMT crashlog device that can perform crash data
>>>>>> +             recoring. Each crashlogX device has an associated
>>>>>> +             /dev/crashlogX device node. This node can be opened and mapped
>>>>>> +             to access the resulting crashlog data. The register layout for
>>>>>> +             the log can be determined from an XML file of specified guid
>>>>>> +             for the parent device.
>>>>>> +
>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/guid
>>>>>> +Date:                September 2020
>>>>>> +KernelVersion:       5.10
>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> +Description:
>>>>>> +             (RO) The guid for this crashlog device. The guid identifies the
>>>>>> +             version of the XML file for the parent device that should be
>>>>>> +             used to determine the register layout.
>>>>>> +
>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/size
>>>>>> +Date:                September 2020
>>>>>> +KernelVersion:       5.10
>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> +Description:
>>>>>> +             (RO) The length of the result buffer in bytes that corresponds
>>>>>> +             to the mapping size for the /dev/crashlogX device node.
>>>>>> +
>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/offset
>>>>>> +Date:                September 2020
>>>>>> +KernelVersion:       5.10
>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> +Description:
>>>>>> +             (RO) The offset of the buffer in bytes that corresponds
>>>>>> +             to the mapping for the /dev/crashlogX device node.
>>>>>> +
>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/enable
>>>>>> +Date:                September 2020
>>>>>> +KernelVersion:       5.10
>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> +Description:
>>>>>> +             (RW) Boolean value controlling if the crashlog functionality
>>>>>> +             is enabled for the /dev/crashlogX device node.
>>>>>> +
>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/trigger
>>>>>> +Date:                September 2020
>>>>>> +KernelVersion:       5.10
>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> +Description:
>>>>>> +             (RW) Boolean value controlling  the triggering of the
>>>>>> +             /dev/crashlogX device node. When read it provides data on if
>>>>>> +             the crashlog has been triggered. When written to it can be
>>>>>> +             used to either clear the current trigger by writing false, or
>>>>>> +             to trigger a new event if the trigger is not currently set.
>>>>>> +
>>>>>
>>>>> Both the pmt_crashlog and the attributes suggest that this is highly
>>>>> Intel PMT specific. /sys/class/foo interfaces are generally speaking
>>>>> meant to be generic interfaces.
>>>>>
>>>>> If this was defining a generic, vendor and implementation agnostic interface for
>>>>> configuring / accessing crashlogs, then using a class would be fine, but that
>>>>> is not the case, so I believe that this should not implement / register a class.
>>>>>
>>>>> Since the devices are instantiated through MFD there already is a
>>>>> static sysfs-path which can be used to find the device in sysfs:
>>>>> /sys/bus/platform/device/pmt_crashlog
>>>>>
>>>>> So you can register the sysfs attributes directly under the platform_device
>>>>> and then userspace can easily find them, so there really is no need to
>>>>> use a class here.
>>>>
>>>> I see. So we change the root directory from "/sys/class/pmt_crashlog/"
>>>> to "/sys/bus/platform/device/pmt_crashlog" while retaining the same
>>>> functionality. That should be workable.
>>>
>>> So one issue as I see it is that if we were to change this then we
>>> probably need to to change the telemetry functionality that was
>>> recently accepted
>>> (https://lore.kernel.org/lkml/20200819180255.11770-1-david.e.box@linux.intel.com/)

You say that this has been accepted, by I don't see any intel_pmt.c
file here yet: ?

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/mfd/

>>> as well. The general idea with using the /sys/class/pmt_crashlog/
>>> approach was to keep things consistent with how the pmt_telemetry was
>>> being accessed. So if we change this then we end up with very
>>> different interfaces for the two very similar pieces of functionality.
>>> So ideally we would want to change both telemetry and crashlog to
>>> function the same way.
>>
>> I agree that the telemetry interface should be changed in a similar way.
>>
>> Luckily it seems that this is not in Linus' tree yet and I'm also not
>> seeing it in next yet, e.g. :
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/platform/x86/intel_pmt_telemetry.c
>> does not exist.
>>
>> So we seem to still have time to also get the telemetry driver userspace API
>> fixed too.
>>
>> I see that Andy gave his Reviewed-by for the intel_pmt_telemetry.c code.
>>
>> Andy, I have some concerns about the userspace API choices made here,
>> see my earlier review of this patch. Do you agree with my suggestions,
>> or do you think it would be ok to move forward with the telemetry and
>> now also the crashlog API each registering their own private class
>> under /sys/class ?
>>
>> AFAIK classes are supposed to be generic and not driver-private, so
>> that seems wrong to me.  Also PMC is Intel specific and vendor specific
>> stuff really does not belong under /sys/class AFAIK ?
>>
>>> Do you have any good examples of anything that has done something
>>> similar? From what I can tell it looks like we need to clean up the
>>> naming to drop the ".%d.auto" for the bus directory names
>>
>> Assuming there will only be one of each platform-device, then you
>> can just replace the PLATFORM_DEVID_AUTO param to devm_mfd_add_devices()
>> with PLATFORM_DEVID_NONE and the .%d.auto will go away.
> 
> We will have multiples of each platform device. So for example we can
> have multiple OOBMSM in each system and each OOBMSM may have multiple
> telemetry regions and maybe one crashlog.

What is a OOBMSM ? Please don't make the person reviewing your patches
do detective work. Only use acronyms if they are something of which
you could reasonably expect any mailinglist reader to know what
they are.

So looking at:
https://lore.kernel.org/lkml/20200819180255.11770-3-david.e.box@linux.intel.com/

What you are saying (I guess) is that both the pmt_pci_probe()
function may run multiple times; and that for a single pmt_pci_probe()
call, pmt_add_dev() may hit the DVSEC_INTEL_ID_TELEMETRY case more then
once?

If I understand either one correct, then indeed we need PLATFORM_DEVID_AUTO.

Which I guess makes using a class for enumeration somewhat sensible.

But I really do not think we need 2 separate classes, one for
pmt_telemetry and one for pmt_crashlog. Also since this is rather
Intel specific lets at least make that clear in the name.

So how about intel_pmt as class and then register both the telemetry
and the crashlog devs there? (the type can easily be deferred from
the name part before the .%d.auto suffix) ?


>>> and then
>>> look at adding a folder to handle all of the instances of either
>>> telemetry or crashlog, assuming we follow the reg-dummy or serial8250
>>> model.
>>
>> So there can be multiple instances, you mean like the multiple chardevs
>> you add now, or can there be multiple platform-devices of the same
>> time instantiated through the MFD code ?
>>
>> If you mean like the multiple chardevs, then yes you could add a folder
>> for the binary sysfs attributes replacing those, or register them
>> with a dynamic name with a number appended to the name.
> 
> In addition to just the binary sysfs we need to expose several other
> fields including the GUID, the size, and controls for enabling,
> disabling, and either triggering or checking to see if the crashlog
> has already been triggered. As such we would end up with a folder per
> device and the binary sysfs would probably be living in the folder.

Erm, in the Documentation/ABI/testing/sysfs-class-pmt_crashlog
file from above you already put all that in sysfs ?

So just add the binary sysfs file replacing the char-device next
to (in the same dir as) the existing sysfs attributes for these.

>>> Similarly the crashlog and telemetry both rely on similar mechanisms
>>> to display the MMIO region containing the data. I still need to spend
>>> some more time looking into what is involved in switching from a char
>>> device to a binary sysfs, but I think with the example I found earlier
>>> of the resourceN bit from the PCI sysfs I can probably make that work
>>> for both cases.
>>
>> I'm not sure that the PCI sysfs io resources are the best example,
>> as mentioned those mmap to actual memory-mapped io, which is somewhat
>> special.
> 
> The reason why I bring them up is because there are cases for
> telemetry where the applications will likely want to be able to just
> memory map the region and poll on certain statistics. So if nothing
> else we may end up supporting both a mmap and a read option.

Ok.

Regards,

Hans

Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

[Hans de Goede]

Hi,

On 9/17/20 11:35 PM, Alexander Duyck wrote:
> On Thu, Sep 17, 2020 at 5:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 9/15/20 12:35 AM, Alexander Duyck wrote:
>>> On Mon, Sep 14, 2020 at 11:07 AM Alexander Duyck
>>> <alexander.duyck@gmail.com> wrote:
>>>>
>>>> On Mon, Sep 14, 2020 at 6:42 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 9/11/20 9:45 PM, David E. Box wrote:
>>>>>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>>
>>>>>> Add support for the Intel Platform Monitoring Technology crashlog
>>>>>> interface.  This interface provides a few sysfs values to allow for
>>>>>> controlling the crashlog telemetry interface as well as a character driver
>>>>>> to allow for mapping the crashlog memory region so that it can be accessed
>>>>>> after a crashlog has been recorded.
>>>>>>
>>>>>> This driver is meant to only support the server version of the crashlog
>>>>>> which is identified as crash_type 1 with a version of zero. Currently no
>>>>>> other types are supported.
>>>>>>
>>>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
>>>>>> ---
>>>>>>     .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
>>>>>>     drivers/platform/x86/Kconfig                  |  10 +
>>>>>>     drivers/platform/x86/Makefile                 |   1 +
>>>>>>     drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
>>>>>>     4 files changed, 665 insertions(+)
>>>>>>     create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
>>>>>>     create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c
>>>>>>
>>>>>> diff --git a/Documentation/ABI/testing/sysfs-class-pmt_crashlog b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
>>>>>> new file mode 100644
>>>>>> index 000000000000..40fb4ff437a6
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
>>>>>> @@ -0,0 +1,66 @@
>>>>>> +What:                /sys/class/pmt_crashlog/
>>>>>> +Date:                September 2020
>>>>>> +KernelVersion:       5.10
>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> +Description:
>>>>>> +             The pmt_crashlog/ class directory contains information
>>>>>> +             for devices that expose crashlog capabilities using the Intel
>>>>>> +             Platform Monitoring Technology (PTM).
>>>>>> +
>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX
>>>>>> +Date:                September 2020
>>>>>> +KernelVersion:       5.10
>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> +Description:
>>>>>> +             The crashlogX directory contains files for configuring an
>>>>>> +             instance of a PMT crashlog device that can perform crash data
>>>>>> +             recoring. Each crashlogX device has an associated
>>>>>> +             /dev/crashlogX device node. This node can be opened and mapped
>>>>>> +             to access the resulting crashlog data. The register layout for
>>>>>> +             the log can be determined from an XML file of specified guid
>>>>>> +             for the parent device.
>>>>>> +
>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/guid
>>>>>> +Date:                September 2020
>>>>>> +KernelVersion:       5.10
>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> +Description:
>>>>>> +             (RO) The guid for this crashlog device. The guid identifies the
>>>>>> +             version of the XML file for the parent device that should be
>>>>>> +             used to determine the register layout.
>>>>>> +
>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/size
>>>>>> +Date:                September 2020
>>>>>> +KernelVersion:       5.10
>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> +Description:
>>>>>> +             (RO) The length of the result buffer in bytes that corresponds
>>>>>> +             to the mapping size for the /dev/crashlogX device node.
>>>>>> +
>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/offset
>>>>>> +Date:                September 2020
>>>>>> +KernelVersion:       5.10
>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> +Description:
>>>>>> +             (RO) The offset of the buffer in bytes that corresponds
>>>>>> +             to the mapping for the /dev/crashlogX device node.
>>>>>> +
>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/enable
>>>>>> +Date:                September 2020
>>>>>> +KernelVersion:       5.10
>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> +Description:
>>>>>> +             (RW) Boolean value controlling if the crashlog functionality
>>>>>> +             is enabled for the /dev/crashlogX device node.
>>>>>> +
>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/trigger
>>>>>> +Date:                September 2020
>>>>>> +KernelVersion:       5.10
>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>> +Description:
>>>>>> +             (RW) Boolean value controlling  the triggering of the
>>>>>> +             /dev/crashlogX device node. When read it provides data on if
>>>>>> +             the crashlog has been triggered. When written to it can be
>>>>>> +             used to either clear the current trigger by writing false, or
>>>>>> +             to trigger a new event if the trigger is not currently set.
>>>>>> +
>>>>>
>>>>> Both the pmt_crashlog and the attributes suggest that this is highly
>>>>> Intel PMT specific. /sys/class/foo interfaces are generally speaking
>>>>> meant to be generic interfaces.
>>>>>
>>>>> If this was defining a generic, vendor and implementation agnostic interface for
>>>>> configuring / accessing crashlogs, then using a class would be fine, but that
>>>>> is not the case, so I believe that this should not implement / register a class.
>>>>>
>>>>> Since the devices are instantiated through MFD there already is a
>>>>> static sysfs-path which can be used to find the device in sysfs:
>>>>> /sys/bus/platform/device/pmt_crashlog
>>>>>
>>>>> So you can register the sysfs attributes directly under the platform_device
>>>>> and then userspace can easily find them, so there really is no need to
>>>>> use a class here.
>>>>
>>>> I see. So we change the root directory from "/sys/class/pmt_crashlog/"
>>>> to "/sys/bus/platform/device/pmt_crashlog" while retaining the same
>>>> functionality. That should be workable.
>>>
>>> So one issue as I see it is that if we were to change this then we
>>> probably need to to change the telemetry functionality that was
>>> recently accepted
>>> (https://lore.kernel.org/lkml/20200819180255.11770-1-david.e.box@linux.intel.com/)

You say that this has been accepted, by I don't see any intel_pmt.c
file here yet: ?

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/mfd/

>>> as well. The general idea with using the /sys/class/pmt_crashlog/
>>> approach was to keep things consistent with how the pmt_telemetry was
>>> being accessed. So if we change this then we end up with very
>>> different interfaces for the two very similar pieces of functionality.
>>> So ideally we would want to change both telemetry and crashlog to
>>> function the same way.
>>
>> I agree that the telemetry interface should be changed in a similar way.
>>
>> Luckily it seems that this is not in Linus' tree yet and I'm also not
>> seeing it in next yet, e.g. :
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/platform/x86/intel_pmt_telemetry.c
>> does not exist.
>>
>> So we seem to still have time to also get the telemetry driver userspace API
>> fixed too.
>>
>> I see that Andy gave his Reviewed-by for the intel_pmt_telemetry.c code.
>>
>> Andy, I have some concerns about the userspace API choices made here,
>> see my earlier review of this patch. Do you agree with my suggestions,
>> or do you think it would be ok to move forward with the telemetry and
>> now also the crashlog API each registering their own private class
>> under /sys/class ?
>>
>> AFAIK classes are supposed to be generic and not driver-private, so
>> that seems wrong to me.  Also PMC is Intel specific and vendor specific
>> stuff really does not belong under /sys/class AFAIK ?
>>
>>> Do you have any good examples of anything that has done something
>>> similar? From what I can tell it looks like we need to clean up the
>>> naming to drop the ".%d.auto" for the bus directory names
>>
>> Assuming there will only be one of each platform-device, then you
>> can just replace the PLATFORM_DEVID_AUTO param to devm_mfd_add_devices()
>> with PLATFORM_DEVID_NONE and the .%d.auto will go away.
> 
> We will have multiples of each platform device. So for example we can
> have multiple OOBMSM in each system and each OOBMSM may have multiple
> telemetry regions and maybe one crashlog.

What is a OOBMSM ? Please don't make the person reviewing your patches
do detective work. Only use acronyms if they are something of which
you could reasonably expect any mailinglist reader to know what
they are.

So looking at:
https://lore.kernel.org/lkml/20200819180255.11770-3-david.e.box@linux.intel.com/

What you are saying (I guess) is that both the pmt_pci_probe()
function may run multiple times; and that for a single pmt_pci_probe()
call, pmt_add_dev() may hit the DVSEC_INTEL_ID_TELEMETRY case more then
once?

If I understand either one correct, then indeed we need PLATFORM_DEVID_AUTO.

Which I guess makes using a class for enumeration somewhat sensible.

But I really do not think we need 2 separate classes, one for
pmt_telemetry and one for pmt_crashlog. Also since this is rather
Intel specific lets at least make that clear in the name.

So how about intel_pmt as class and then register both the telemetry
and the crashlog devs there? (the type can easily be deferred from
the name part before the .%d.auto suffix) ?


>>> and then
>>> look at adding a folder to handle all of the instances of either
>>> telemetry or crashlog, assuming we follow the reg-dummy or serial8250
>>> model.
>>
>> So there can be multiple instances, you mean like the multiple chardevs
>> you add now, or can there be multiple platform-devices of the same
>> time instantiated through the MFD code ?
>>
>> If you mean like the multiple chardevs, then yes you could add a folder
>> for the binary sysfs attributes replacing those, or register them
>> with a dynamic name with a number appended to the name.
> 
> In addition to just the binary sysfs we need to expose several other
> fields including the GUID, the size, and controls for enabling,
> disabling, and either triggering or checking to see if the crashlog
> has already been triggered. As such we would end up with a folder per
> device and the binary sysfs would probably be living in the folder.

Erm, in the Documentation/ABI/testing/sysfs-class-pmt_crashlog
file from above you already put all that in sysfs ?

So just add the binary sysfs file replacing the char-device next
to (in the same dir as) the existing sysfs attributes for these.

>>> Similarly the crashlog and telemetry both rely on similar mechanisms
>>> to display the MMIO region containing the data. I still need to spend
>>> some more time looking into what is involved in switching from a char
>>> device to a binary sysfs, but I think with the example I found earlier
>>> of the resourceN bit from the PCI sysfs I can probably make that work
>>> for both cases.
>>
>> I'm not sure that the PCI sysfs io resources are the best example,
>> as mentioned those mmap to actual memory-mapped io, which is somewhat
>> special.
> 
> The reason why I bring them up is because there are cases for
> telemetry where the applications will likely want to be able to just
> memory map the region and poll on certain statistics. So if nothing
> else we may end up supporting both a mmap and a read option.

Ok.

Regards,

Hans

Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

[Alexander Duyck]

On Sat, Sep 19, 2020 at 1:01 AM Alexey Budankov
<alexey.budankov@linux.intel.com> wrote:
>
> Hi,
>
> Thanks for the patches.
>
> On 11.09.2020 22:45, David E. Box wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > Add support for the Intel Platform Monitoring Technology crashlog
> > interface.  This interface provides a few sysfs values to allow for
> > controlling the crashlog telemetry interface as well as a character driver
> > to allow for mapping the crashlog memory region so that it can be accessed
> > after a crashlog has been recorded.
> >
> > This driver is meant to only support the server version of the crashlog
> > which is identified as crash_type 1 with a version of zero. Currently no
> > other types are supported.
> >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> >  .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
> >  drivers/platform/x86/Kconfig                  |  10 +
> >  drivers/platform/x86/Makefile                 |   1 +
> >  drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
> >  4 files changed, 665 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
> >  create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c
>
> <SNIP>
>
> > +
> > +/*
> > + * devfs
> > + */
> > +static int pmt_crashlog_open(struct inode *inode, struct file *filp)
> > +{
> > +     struct crashlog_entry *entry;
> > +     struct pci_driver *pci_drv;
> > +     struct pmt_crashlog_priv *priv;
> > +
> > +     if (!capable(CAP_SYS_ADMIN))
> > +             return -EPERM;
>
> Will not this above still block access to /dev/crashlogX for admin_group users
> in case root configured access e.g. similar to this:
>
> ls -alh /dev/
> crw-rw----.  1 root admin_group      1,   9 Sep 15 18:28 crashlogX
>
> If yes then that capable() check is probably superfluous and
> should be avoided in order not to block access to PMT data.
>
> Could you please clarify or comment?
>
> Thanks,
> Alexei

Actually this should probably be updated to "if (!perfmon_capable())"
instead. The telemetry driver code originally had the CAP_SYS_ADMIN
check and it probably makes more sense to limit this user-wise to the
same users who have access to performon.

Thanks.

- Alex

Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

[Alexander Duyck]

On Mon, Sep 21, 2020 at 6:16 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 9/17/20 11:35 PM, Alexander Duyck wrote:
> > On Thu, Sep 17, 2020 at 5:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 9/15/20 12:35 AM, Alexander Duyck wrote:
> >>> On Mon, Sep 14, 2020 at 11:07 AM Alexander Duyck
> >>> <alexander.duyck@gmail.com> wrote:
> >>>>
> >>>> On Mon, Sep 14, 2020 at 6:42 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> On 9/11/20 9:45 PM, David E. Box wrote:
> >>>>>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>>>>
> >>>>>> Add support for the Intel Platform Monitoring Technology crashlog
> >>>>>> interface.  This interface provides a few sysfs values to allow for
> >>>>>> controlling the crashlog telemetry interface as well as a character driver
> >>>>>> to allow for mapping the crashlog memory region so that it can be accessed
> >>>>>> after a crashlog has been recorded.
> >>>>>>
> >>>>>> This driver is meant to only support the server version of the crashlog
> >>>>>> which is identified as crash_type 1 with a version of zero. Currently no
> >>>>>> other types are supported.
> >>>>>>
> >>>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>>>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> >>>>>> ---
> >>>>>>     .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
> >>>>>>     drivers/platform/x86/Kconfig                  |  10 +
> >>>>>>     drivers/platform/x86/Makefile                 |   1 +
> >>>>>>     drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
> >>>>>>     4 files changed, 665 insertions(+)
> >>>>>>     create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
> >>>>>>     create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c
> >>>>>>
> >>>>>> diff --git a/Documentation/ABI/testing/sysfs-class-pmt_crashlog b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..40fb4ff437a6
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
> >>>>>> @@ -0,0 +1,66 @@
> >>>>>> +What:                /sys/class/pmt_crashlog/
> >>>>>> +Date:                September 2020
> >>>>>> +KernelVersion:       5.10
> >>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>>>> +Description:
> >>>>>> +             The pmt_crashlog/ class directory contains information
> >>>>>> +             for devices that expose crashlog capabilities using the Intel
> >>>>>> +             Platform Monitoring Technology (PTM).
> >>>>>> +
> >>>>>> +What:                /sys/class/pmt_crashlog/crashlogX
> >>>>>> +Date:                September 2020
> >>>>>> +KernelVersion:       5.10
> >>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>>>> +Description:
> >>>>>> +             The crashlogX directory contains files for configuring an
> >>>>>> +             instance of a PMT crashlog device that can perform crash data
> >>>>>> +             recoring. Each crashlogX device has an associated
> >>>>>> +             /dev/crashlogX device node. This node can be opened and mapped
> >>>>>> +             to access the resulting crashlog data. The register layout for
> >>>>>> +             the log can be determined from an XML file of specified guid
> >>>>>> +             for the parent device.
> >>>>>> +
> >>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/guid
> >>>>>> +Date:                September 2020
> >>>>>> +KernelVersion:       5.10
> >>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>>>> +Description:
> >>>>>> +             (RO) The guid for this crashlog device. The guid identifies the
> >>>>>> +             version of the XML file for the parent device that should be
> >>>>>> +             used to determine the register layout.
> >>>>>> +
> >>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/size
> >>>>>> +Date:                September 2020
> >>>>>> +KernelVersion:       5.10
> >>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>>>> +Description:
> >>>>>> +             (RO) The length of the result buffer in bytes that corresponds
> >>>>>> +             to the mapping size for the /dev/crashlogX device node.
> >>>>>> +
> >>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/offset
> >>>>>> +Date:                September 2020
> >>>>>> +KernelVersion:       5.10
> >>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>>>> +Description:
> >>>>>> +             (RO) The offset of the buffer in bytes that corresponds
> >>>>>> +             to the mapping for the /dev/crashlogX device node.
> >>>>>> +
> >>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/enable
> >>>>>> +Date:                September 2020
> >>>>>> +KernelVersion:       5.10
> >>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>>>> +Description:
> >>>>>> +             (RW) Boolean value controlling if the crashlog functionality
> >>>>>> +             is enabled for the /dev/crashlogX device node.
> >>>>>> +
> >>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/trigger
> >>>>>> +Date:                September 2020
> >>>>>> +KernelVersion:       5.10
> >>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>>>> +Description:
> >>>>>> +             (RW) Boolean value controlling  the triggering of the
> >>>>>> +             /dev/crashlogX device node. When read it provides data on if
> >>>>>> +             the crashlog has been triggered. When written to it can be
> >>>>>> +             used to either clear the current trigger by writing false, or
> >>>>>> +             to trigger a new event if the trigger is not currently set.
> >>>>>> +
> >>>>>
> >>>>> Both the pmt_crashlog and the attributes suggest that this is highly
> >>>>> Intel PMT specific. /sys/class/foo interfaces are generally speaking
> >>>>> meant to be generic interfaces.
> >>>>>
> >>>>> If this was defining a generic, vendor and implementation agnostic interface for
> >>>>> configuring / accessing crashlogs, then using a class would be fine, but that
> >>>>> is not the case, so I believe that this should not implement / register a class.
> >>>>>
> >>>>> Since the devices are instantiated through MFD there already is a
> >>>>> static sysfs-path which can be used to find the device in sysfs:
> >>>>> /sys/bus/platform/device/pmt_crashlog
> >>>>>
> >>>>> So you can register the sysfs attributes directly under the platform_device
> >>>>> and then userspace can easily find them, so there really is no need to
> >>>>> use a class here.
> >>>>
> >>>> I see. So we change the root directory from "/sys/class/pmt_crashlog/"
> >>>> to "/sys/bus/platform/device/pmt_crashlog" while retaining the same
> >>>> functionality. That should be workable.
> >>>
> >>> So one issue as I see it is that if we were to change this then we
> >>> probably need to to change the telemetry functionality that was
> >>> recently accepted
> >>> (https://lore.kernel.org/lkml/20200819180255.11770-1-david.e.box@linux.intel.com/)
>
> You say that this has been accepted, by I don't see any intel_pmt.c
> file here yet: ?
>
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/mfd/

Here is a link to the thread on the first patch set. The last notes I
saw were that it was going to be applied but it looks like that never
happened.
https://lore.kernel.org/lkml/20200819180255.11770-1-david.e.box@linux.intel.com/

> >>> as well. The general idea with using the /sys/class/pmt_crashlog/
> >>> approach was to keep things consistent with how the pmt_telemetry was
> >>> being accessed. So if we change this then we end up with very
> >>> different interfaces for the two very similar pieces of functionality.
> >>> So ideally we would want to change both telemetry and crashlog to
> >>> function the same way.
> >>
> >> I agree that the telemetry interface should be changed in a similar way.
> >>
> >> Luckily it seems that this is not in Linus' tree yet and I'm also not
> >> seeing it in next yet, e.g. :
> >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/platform/x86/intel_pmt_telemetry.c
> >> does not exist.
> >>
> >> So we seem to still have time to also get the telemetry driver userspace API
> >> fixed too.
> >>
> >> I see that Andy gave his Reviewed-by for the intel_pmt_telemetry.c code.
> >>
> >> Andy, I have some concerns about the userspace API choices made here,
> >> see my earlier review of this patch. Do you agree with my suggestions,
> >> or do you think it would be ok to move forward with the telemetry and
> >> now also the crashlog API each registering their own private class
> >> under /sys/class ?
> >>
> >> AFAIK classes are supposed to be generic and not driver-private, so
> >> that seems wrong to me.  Also PMC is Intel specific and vendor specific
> >> stuff really does not belong under /sys/class AFAIK ?
> >>
> >>> Do you have any good examples of anything that has done something
> >>> similar? From what I can tell it looks like we need to clean up the
> >>> naming to drop the ".%d.auto" for the bus directory names
> >>
> >> Assuming there will only be one of each platform-device, then you
> >> can just replace the PLATFORM_DEVID_AUTO param to devm_mfd_add_devices()
> >> with PLATFORM_DEVID_NONE and the .%d.auto will go away.
> >
> > We will have multiples of each platform device. So for example we can
> > have multiple OOBMSM in each system and each OOBMSM may have multiple
> > telemetry regions and maybe one crashlog.
>
> What is a OOBMSM ? Please don't make the person reviewing your patches
> do detective work. Only use acronyms if they are something of which
> you could reasonably expect any mailinglist reader to know what
> they are.

OOBMSM is an acronym for the Out-of-Band Management Services Module.
It is a PCIe function exposed by a device or CPU to provide in-band
telemetry.

> So looking at:
> https://lore.kernel.org/lkml/20200819180255.11770-3-david.e.box@linux.intel.com/
>
> What you are saying (I guess) is that both the pmt_pci_probe()
> function may run multiple times; and that for a single pmt_pci_probe()
> call, pmt_add_dev() may hit the DVSEC_INTEL_ID_TELEMETRY case more then
> once?
>
> If I understand either one correct, then indeed we need PLATFORM_DEVID_AUTO.
>
> Which I guess makes using a class for enumeration somewhat sensible.

Correct. In our case there will be multiple instances of each device
being potentially allocated.

> But I really do not think we need 2 separate classes, one for
> pmt_telemetry and one for pmt_crashlog. Also since this is rather
> Intel specific lets at least make that clear in the name.
>
> So how about intel_pmt as class and then register both the telemetry
> and the crashlog devs there? (the type can easily be deferred from
> the name part before the .%d.auto suffix) ?

Agreed. So we would set it up as an intel_pmt and then in the case of
crashlog we would be adding the binary sysfs for the memory access, a
trigger control, and the enable control. For the telemetry we would
just be adding the binary sysfs for the telemetry access. Do I have
all of that correct?

> >>> and then
> >>> look at adding a folder to handle all of the instances of either
> >>> telemetry or crashlog, assuming we follow the reg-dummy or serial8250
> >>> model.
> >>
> >> So there can be multiple instances, you mean like the multiple chardevs
> >> you add now, or can there be multiple platform-devices of the same
> >> time instantiated through the MFD code ?
> >>
> >> If you mean like the multiple chardevs, then yes you could add a folder
> >> for the binary sysfs attributes replacing those, or register them
> >> with a dynamic name with a number appended to the name.
> >
> > In addition to just the binary sysfs we need to expose several other
> > fields including the GUID, the size, and controls for enabling,
> > disabling, and either triggering or checking to see if the crashlog
> > has already been triggered. As such we would end up with a folder per
> > device and the binary sysfs would probably be living in the folder.
>
> Erm, in the Documentation/ABI/testing/sysfs-class-pmt_crashlog
> file from above you already put all that in sysfs ?
>
> So just add the binary sysfs file replacing the char-device next
> to (in the same dir as) the existing sysfs attributes for these.

Okay.

Thanks.

- Alex

Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

[Hans de Goede]

Hi,

On 9/21/20 3:57 PM, Alexander Duyck wrote:
> On Mon, Sep 21, 2020 at 6:16 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 9/17/20 11:35 PM, Alexander Duyck wrote:
>>> On Thu, Sep 17, 2020 at 5:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 9/15/20 12:35 AM, Alexander Duyck wrote:
>>>>> On Mon, Sep 14, 2020 at 11:07 AM Alexander Duyck
>>>>> <alexander.duyck@gmail.com> wrote:
>>>>>>
>>>>>> On Mon, Sep 14, 2020 at 6:42 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 9/11/20 9:45 PM, David E. Box wrote:
>>>>>>>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>>>>
>>>>>>>> Add support for the Intel Platform Monitoring Technology crashlog
>>>>>>>> interface.  This interface provides a few sysfs values to allow for
>>>>>>>> controlling the crashlog telemetry interface as well as a character driver
>>>>>>>> to allow for mapping the crashlog memory region so that it can be accessed
>>>>>>>> after a crashlog has been recorded.
>>>>>>>>
>>>>>>>> This driver is meant to only support the server version of the crashlog
>>>>>>>> which is identified as crash_type 1 with a version of zero. Currently no
>>>>>>>> other types are supported.
>>>>>>>>
>>>>>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>>>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
>>>>>>>> ---
>>>>>>>>      .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
>>>>>>>>      drivers/platform/x86/Kconfig                  |  10 +
>>>>>>>>      drivers/platform/x86/Makefile                 |   1 +
>>>>>>>>      drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
>>>>>>>>      4 files changed, 665 insertions(+)
>>>>>>>>      create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
>>>>>>>>      create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c
>>>>>>>>
>>>>>>>> diff --git a/Documentation/ABI/testing/sysfs-class-pmt_crashlog b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..40fb4ff437a6
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/ABI/testing/sysfs-class-pmt_crashlog
>>>>>>>> @@ -0,0 +1,66 @@
>>>>>>>> +What:                /sys/class/pmt_crashlog/
>>>>>>>> +Date:                September 2020
>>>>>>>> +KernelVersion:       5.10
>>>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>>>> +Description:
>>>>>>>> +             The pmt_crashlog/ class directory contains information
>>>>>>>> +             for devices that expose crashlog capabilities using the Intel
>>>>>>>> +             Platform Monitoring Technology (PTM).
>>>>>>>> +
>>>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX
>>>>>>>> +Date:                September 2020
>>>>>>>> +KernelVersion:       5.10
>>>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>>>> +Description:
>>>>>>>> +             The crashlogX directory contains files for configuring an
>>>>>>>> +             instance of a PMT crashlog device that can perform crash data
>>>>>>>> +             recoring. Each crashlogX device has an associated
>>>>>>>> +             /dev/crashlogX device node. This node can be opened and mapped
>>>>>>>> +             to access the resulting crashlog data. The register layout for
>>>>>>>> +             the log can be determined from an XML file of specified guid
>>>>>>>> +             for the parent device.
>>>>>>>> +
>>>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/guid
>>>>>>>> +Date:                September 2020
>>>>>>>> +KernelVersion:       5.10
>>>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>>>> +Description:
>>>>>>>> +             (RO) The guid for this crashlog device. The guid identifies the
>>>>>>>> +             version of the XML file for the parent device that should be
>>>>>>>> +             used to determine the register layout.
>>>>>>>> +
>>>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/size
>>>>>>>> +Date:                September 2020
>>>>>>>> +KernelVersion:       5.10
>>>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>>>> +Description:
>>>>>>>> +             (RO) The length of the result buffer in bytes that corresponds
>>>>>>>> +             to the mapping size for the /dev/crashlogX device node.
>>>>>>>> +
>>>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/offset
>>>>>>>> +Date:                September 2020
>>>>>>>> +KernelVersion:       5.10
>>>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>>>> +Description:
>>>>>>>> +             (RO) The offset of the buffer in bytes that corresponds
>>>>>>>> +             to the mapping for the /dev/crashlogX device node.
>>>>>>>> +
>>>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/enable
>>>>>>>> +Date:                September 2020
>>>>>>>> +KernelVersion:       5.10
>>>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>>>> +Description:
>>>>>>>> +             (RW) Boolean value controlling if the crashlog functionality
>>>>>>>> +             is enabled for the /dev/crashlogX device node.
>>>>>>>> +
>>>>>>>> +What:                /sys/class/pmt_crashlog/crashlogX/trigger
>>>>>>>> +Date:                September 2020
>>>>>>>> +KernelVersion:       5.10
>>>>>>>> +Contact:     Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>>>> +Description:
>>>>>>>> +             (RW) Boolean value controlling  the triggering of the
>>>>>>>> +             /dev/crashlogX device node. When read it provides data on if
>>>>>>>> +             the crashlog has been triggered. When written to it can be
>>>>>>>> +             used to either clear the current trigger by writing false, or
>>>>>>>> +             to trigger a new event if the trigger is not currently set.
>>>>>>>> +
>>>>>>>
>>>>>>> Both the pmt_crashlog and the attributes suggest that this is highly
>>>>>>> Intel PMT specific. /sys/class/foo interfaces are generally speaking
>>>>>>> meant to be generic interfaces.
>>>>>>>
>>>>>>> If this was defining a generic, vendor and implementation agnostic interface for
>>>>>>> configuring / accessing crashlogs, then using a class would be fine, but that
>>>>>>> is not the case, so I believe that this should not implement / register a class.
>>>>>>>
>>>>>>> Since the devices are instantiated through MFD there already is a
>>>>>>> static sysfs-path which can be used to find the device in sysfs:
>>>>>>> /sys/bus/platform/device/pmt_crashlog
>>>>>>>
>>>>>>> So you can register the sysfs attributes directly under the platform_device
>>>>>>> and then userspace can easily find them, so there really is no need to
>>>>>>> use a class here.
>>>>>>
>>>>>> I see. So we change the root directory from "/sys/class/pmt_crashlog/"
>>>>>> to "/sys/bus/platform/device/pmt_crashlog" while retaining the same
>>>>>> functionality. That should be workable.
>>>>>
>>>>> So one issue as I see it is that if we were to change this then we
>>>>> probably need to to change the telemetry functionality that was
>>>>> recently accepted
>>>>> (https://lore.kernel.org/lkml/20200819180255.11770-1-david.e.box@linux.intel.com/)
>>
>> You say that this has been accepted, by I don't see any intel_pmt.c
>> file here yet: ?
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/mfd/
> 
> Here is a link to the thread on the first patch set. The last notes I
> saw were that it was going to be applied but it looks like that never
> happened.
> https://lore.kernel.org/lkml/20200819180255.11770-1-david.e.box@linux.intel.com/
> 
>>>>> as well. The general idea with using the /sys/class/pmt_crashlog/
>>>>> approach was to keep things consistent with how the pmt_telemetry was
>>>>> being accessed. So if we change this then we end up with very
>>>>> different interfaces for the two very similar pieces of functionality.
>>>>> So ideally we would want to change both telemetry and crashlog to
>>>>> function the same way.
>>>>
>>>> I agree that the telemetry interface should be changed in a similar way.
>>>>
>>>> Luckily it seems that this is not in Linus' tree yet and I'm also not
>>>> seeing it in next yet, e.g. :
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/platform/x86/intel_pmt_telemetry.c
>>>> does not exist.
>>>>
>>>> So we seem to still have time to also get the telemetry driver userspace API
>>>> fixed too.
>>>>
>>>> I see that Andy gave his Reviewed-by for the intel_pmt_telemetry.c code.
>>>>
>>>> Andy, I have some concerns about the userspace API choices made here,
>>>> see my earlier review of this patch. Do you agree with my suggestions,
>>>> or do you think it would be ok to move forward with the telemetry and
>>>> now also the crashlog API each registering their own private class
>>>> under /sys/class ?
>>>>
>>>> AFAIK classes are supposed to be generic and not driver-private, so
>>>> that seems wrong to me.  Also PMC is Intel specific and vendor specific
>>>> stuff really does not belong under /sys/class AFAIK ?
>>>>
>>>>> Do you have any good examples of anything that has done something
>>>>> similar? From what I can tell it looks like we need to clean up the
>>>>> naming to drop the ".%d.auto" for the bus directory names
>>>>
>>>> Assuming there will only be one of each platform-device, then you
>>>> can just replace the PLATFORM_DEVID_AUTO param to devm_mfd_add_devices()
>>>> with PLATFORM_DEVID_NONE and the .%d.auto will go away.
>>>
>>> We will have multiples of each platform device. So for example we can
>>> have multiple OOBMSM in each system and each OOBMSM may have multiple
>>> telemetry regions and maybe one crashlog.
>>
>> What is a OOBMSM ? Please don't make the person reviewing your patches
>> do detective work. Only use acronyms if they are something of which
>> you could reasonably expect any mailinglist reader to know what
>> they are.
> 
> OOBMSM is an acronym for the Out-of-Band Management Services Module.
> It is a PCIe function exposed by a device or CPU to provide in-band
> telemetry.
> 
>> So looking at:
>> https://lore.kernel.org/lkml/20200819180255.11770-3-david.e.box@linux.intel.com/
>>
>> What you are saying (I guess) is that both the pmt_pci_probe()
>> function may run multiple times; and that for a single pmt_pci_probe()
>> call, pmt_add_dev() may hit the DVSEC_INTEL_ID_TELEMETRY case more then
>> once?
>>
>> If I understand either one correct, then indeed we need PLATFORM_DEVID_AUTO.
>>
>> Which I guess makes using a class for enumeration somewhat sensible.
> 
> Correct. In our case there will be multiple instances of each device
> being potentially allocated.
> 
>> But I really do not think we need 2 separate classes, one for
>> pmt_telemetry and one for pmt_crashlog. Also since this is rather
>> Intel specific lets at least make that clear in the name.
>>
>> So how about intel_pmt as class and then register both the telemetry
>> and the crashlog devs there? (the type can easily be deferred from
>> the name part before the .%d.auto suffix) ?
> 
> Agreed. So we would set it up as an intel_pmt and then in the case of
> crashlog we would be adding the binary sysfs for the memory access, a
> trigger control, and the enable control. For the telemetry we would
> just be adding the binary sysfs for the telemetry access. Do I have
> all of that correct?

Yes sounds good.

Regards,

Hans

Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

[Alexander Duyck]

On Mon, Sep 21, 2020 at 9:07 AM Alexey Budankov
<alexey.budankov@linux.intel.com> wrote:
>
>
> On 21.09.2020 16:36, Alexander Duyck wrote:
> > On Sat, Sep 19, 2020 at 1:01 AM Alexey Budankov
> > <alexey.budankov@linux.intel.com> wrote:
> >>
> >> Hi,
> >>
> >> Thanks for the patches.
> >>
> >> On 11.09.2020 22:45, David E. Box wrote:
> >>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>>
> >>> Add support for the Intel Platform Monitoring Technology crashlog
> >>> interface.  This interface provides a few sysfs values to allow for
> >>> controlling the crashlog telemetry interface as well as a character driver
> >>> to allow for mapping the crashlog memory region so that it can be accessed
> >>> after a crashlog has been recorded.
> >>>
> >>> This driver is meant to only support the server version of the crashlog
> >>> which is identified as crash_type 1 with a version of zero. Currently no
> >>> other types are supported.
> >>>
> >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> >>> ---
> >>>  .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
> >>>  drivers/platform/x86/Kconfig                  |  10 +
> >>>  drivers/platform/x86/Makefile                 |   1 +
> >>>  drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
> >>>  4 files changed, 665 insertions(+)
> >>>  create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
> >>>  create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c
> >>
> >> <SNIP>
> >>
> >>> +
> >>> +/*
> >>> + * devfs
> >>> + */
> >>> +static int pmt_crashlog_open(struct inode *inode, struct file *filp)
> >>> +{
> >>> +     struct crashlog_entry *entry;
> >>> +     struct pci_driver *pci_drv;
> >>> +     struct pmt_crashlog_priv *priv;
> >>> +
> >>> +     if (!capable(CAP_SYS_ADMIN))
> >>> +             return -EPERM;
> >>
> >> Will not this above still block access to /dev/crashlogX for admin_group users
> >> in case root configured access e.g. similar to this:
> >>
> >> ls -alh /dev/
> >> crw-rw----.  1 root admin_group      1,   9 Sep 15 18:28 crashlogX
> >>
> >> If yes then that capable() check is probably superfluous and
> >> should be avoided in order not to block access to PMT data.
> >>
> >> Could you please clarify or comment?
> >>
> >> Thanks,
> >> Alexei
> >
> > Actually this should probably be updated to "if (!perfmon_capable())"
> > instead. The telemetry driver code originally had the CAP_SYS_ADMIN
> > check and it probably makes more sense to limit this user-wise to the
> > same users who have access to performon.
>
> Indeed, it is currently perfmon_capable() for performance part but it is unclear
> if it should be the same for crashlog since it's more like a debugging thing.
> It appears it all depends on usage models implemented in a user space tools e.g. Perf.
>
> However there is an important use case that is not covered
> neither by perfmon_capable() nor by capable(CAP_SYS_ADMIN).
>
> It is access and usage of PMT features in cluster or cloud environments by
> unprivileged users that don't have root credentials. The users however can run
> software tools (Perf, VTune etc.) once installed and configured by root.
>
> Even though Perf tool can be configured to use use CAP_PERFMON [1] the tool binary
> should still reside on a file system supporting xattr to convey capabilities
> into processes implementing monitoring.
>
> Unfortunately NFSv3 which is quite popular to be used for storing and sharing
> software tooling in large production systems doesn't support capabilities yet.
>
> Thus, capabilities approach still has limitation in HPC clusters and cloud environments
> and for PMT support this limitation has a chance to be lifted if
> suitable access control mechanism would be designed from the very beggining.
>
> Actually I tried to change group ownership of /dev and /sys directories and files, being root,
> and it appeared that for dev file it is possible:
> ls -alh /dev/
> crw-rw----.  1 root admin_group      1,   9 Sep 15 18:28 telem<X>
>
> So if e.g. perf tool having CAP_PERFMON and configured like:
>
> -rwxr-x---.  1 root admin_group  24M Mar  5  2020 perf.cap
>
> would mmap /dev/telem<X> to provide uncore performance insights
> to admin_group users only access control based on user/group/others ownership
> would suffice without capabilities requirement.
>
> Still haven't had chance to verify it for memory mapped PMT dev files and
> that is why I am asking you guys here.
>
> Alexei
>
> [1] https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html#privileged-perf-users-groups

We will have to see. There is a high likelihood this code will go away
if we switch over to binary sysfs attributes for the data. I'm still
working on the rewrite and hope to have something we can review as an
RFC in the next few days.

Thanks.

- Alex

Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

[Alexey Budankov]

On 21.09.2020 20:33, Alexander Duyck wrote:
> On Mon, Sep 21, 2020 at 9:07 AM Alexey Budankov
> <alexey.budankov@linux.intel.com> wrote:
>>
>>
>> On 21.09.2020 16:36, Alexander Duyck wrote:
>>> On Sat, Sep 19, 2020 at 1:01 AM Alexey Budankov
>>> <alexey.budankov@linux.intel.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Thanks for the patches.
>>>>
>>>> On 11.09.2020 22:45, David E. Box wrote:
>>>>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>>
>>>>> Add support for the Intel Platform Monitoring Technology crashlog
>>>>> interface.  This interface provides a few sysfs values to allow for
>>>>> controlling the crashlog telemetry interface as well as a character driver
>>>>> to allow for mapping the crashlog memory region so that it can be accessed
>>>>> after a crashlog has been recorded.
>>>>>
>>>>> This driver is meant to only support the server version of the crashlog
>>>>> which is identified as crash_type 1 with a version of zero. Currently no
>>>>> other types are supported.
>>>>>
>>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>>>>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
>>>>> ---
>>>>>  .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
>>>>>  drivers/platform/x86/Kconfig                  |  10 +
>>>>>  drivers/platform/x86/Makefile                 |   1 +
>>>>>  drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
>>>>>  4 files changed, 665 insertions(+)
>>>>>  create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
>>>>>  create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c
>>>>
>>>> <SNIP>
>>>>
>>>>> +
>>>>> +/*
>>>>> + * devfs
>>>>> + */
>>>>> +static int pmt_crashlog_open(struct inode *inode, struct file *filp)
>>>>> +{
>>>>> +     struct crashlog_entry *entry;
>>>>> +     struct pci_driver *pci_drv;
>>>>> +     struct pmt_crashlog_priv *priv;
>>>>> +
>>>>> +     if (!capable(CAP_SYS_ADMIN))
>>>>> +             return -EPERM;
>>>>
>>>> Will not this above still block access to /dev/crashlogX for admin_group users
>>>> in case root configured access e.g. similar to this:
>>>>
>>>> ls -alh /dev/
>>>> crw-rw----.  1 root admin_group      1,   9 Sep 15 18:28 crashlogX
>>>>
>>>> If yes then that capable() check is probably superfluous and
>>>> should be avoided in order not to block access to PMT data.
>>>>
>>>> Could you please clarify or comment?
>>>>
>>>> Thanks,
>>>> Alexei
>>>
>>> Actually this should probably be updated to "if (!perfmon_capable())"
>>> instead. The telemetry driver code originally had the CAP_SYS_ADMIN
>>> check and it probably makes more sense to limit this user-wise to the
>>> same users who have access to performon.
>>
>> Indeed, it is currently perfmon_capable() for performance part but it is unclear
>> if it should be the same for crashlog since it's more like a debugging thing.
>> It appears it all depends on usage models implemented in a user space tools e.g. Perf.
>>
>> However there is an important use case that is not covered
>> neither by perfmon_capable() nor by capable(CAP_SYS_ADMIN).
>>
>> It is access and usage of PMT features in cluster or cloud environments by
>> unprivileged users that don't have root credentials. The users however can run
>> software tools (Perf, VTune etc.) once installed and configured by root.
>>
>> Even though Perf tool can be configured to use use CAP_PERFMON [1] the tool binary
>> should still reside on a file system supporting xattr to convey capabilities
>> into processes implementing monitoring.
>>
>> Unfortunately NFSv3 which is quite popular to be used for storing and sharing
>> software tooling in large production systems doesn't support capabilities yet.
>>
>> Thus, capabilities approach still has limitation in HPC clusters and cloud environments
>> and for PMT support this limitation has a chance to be lifted if
>> suitable access control mechanism would be designed from the very beggining.
>>
>> Actually I tried to change group ownership of /dev and /sys directories and files, being root,
>> and it appeared that for dev file it is possible:
>> ls -alh /dev/
>> crw-rw----.  1 root admin_group      1,   9 Sep 15 18:28 telem<X>
>>
>> So if e.g. perf tool having CAP_PERFMON and configured like:
>>
>> -rwxr-x---.  1 root admin_group  24M Mar  5  2020 perf.cap
>>
>> would mmap /dev/telem<X> to provide uncore performance insights
>> to admin_group users only access control based on user/group/others ownership
>> would suffice without capabilities requirement.
>>
>> Still haven't had chance to verify it for memory mapped PMT dev files and
>> that is why I am asking you guys here.
>>
>> Alexei
>>
>> [1] https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html#privileged-perf-users-groups
> 
> We will have to see. There is a high likelihood this code will go away
> if we switch over to binary sysfs attributes for the data. I'm still
> working on the rewrite and hope to have something we can review as an
> RFC in the next few days.

Fair enough. Would appreciate to be CCed on the PMT topic patches.

Thanks,
Alexei

Re: [PATCH 1/3] mfd: intel_pmt: Add OOBMSM device ID

[Lee Jones]

On Fri, 11 Sep 2020, David E. Box wrote:

> Add Out of Band Management Services Module device ID to Intel PMT driver.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/mfd/intel_pmt.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mfd/intel_pmt.c b/drivers/mfd/intel_pmt.c
> index 0e572b105101..8f9970ab3026 100644
> --- a/drivers/mfd/intel_pmt.c
> +++ b/drivers/mfd/intel_pmt.c
> @@ -55,6 +55,8 @@ struct pmt_platform_info {
>  	unsigned long quirks;
>  };
>  
> +static const struct pmt_platform_info pmt_info;
> +
>  static const struct pmt_platform_info tgl_info = {
>  	.quirks = PMT_QUIRK_NO_WATCHER | PMT_QUIRK_NO_CRASHLOG |
>  		  PMT_QUIRK_TABLE_SHIFT,
> @@ -200,8 +202,10 @@ static void pmt_pci_remove(struct pci_dev *pdev)
>  	pm_runtime_get_sync(&pdev->dev);
>  }
>  
> +#define PCI_DEVICE_ID_INTEL_PMT_OOBMSM	0x09a7
>  #define PCI_DEVICE_ID_INTEL_PMT_TGL	0x9a0d
>  static const struct pci_device_id pmt_pci_ids[] = {
> +	{ PCI_DEVICE_DATA(INTEL, PMT_OOBMSM, &pmt_info) },

Why are you supplying an empty struct?

>  	{ PCI_DEVICE_DATA(INTEL, PMT_TGL, &tgl_info) },
>  	{ }
>  };

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/3] mfd: intel_pmt: Add OOBMSM device ID

[David E. Box]

On Tue, 2020-09-29 at 10:51 +0100, Lee Jones wrote:
> On Fri, 11 Sep 2020, David E. Box wrote:
> 
> > Add Out of Band Management Services Module device ID to Intel PMT
> > driver.
> > 
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> >  drivers/mfd/intel_pmt.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/mfd/intel_pmt.c b/drivers/mfd/intel_pmt.c
> > index 0e572b105101..8f9970ab3026 100644
> > --- a/drivers/mfd/intel_pmt.c
> > +++ b/drivers/mfd/intel_pmt.c
> > @@ -55,6 +55,8 @@ struct pmt_platform_info {
> >  	unsigned long quirks;
> >  };
> >  
> > +static const struct pmt_platform_info pmt_info;
> > +
> >  static const struct pmt_platform_info tgl_info = {
> >  	.quirks = PMT_QUIRK_NO_WATCHER | PMT_QUIRK_NO_CRASHLOG |
> >  		  PMT_QUIRK_TABLE_SHIFT,
> > @@ -200,8 +202,10 @@ static void pmt_pci_remove(struct pci_dev
> > *pdev)
> >  	pm_runtime_get_sync(&pdev->dev);
> >  }
> >  
> > +#define PCI_DEVICE_ID_INTEL_PMT_OOBMSM	0x09a7
> >  #define PCI_DEVICE_ID_INTEL_PMT_TGL	0x9a0d
> >  static const struct pci_device_id pmt_pci_ids[] = {
> > +	{ PCI_DEVICE_DATA(INTEL, PMT_OOBMSM, &pmt_info) },
> 
> Why are you supplying an empty struct?

Because the OOBMSM device doesn't need code provided driver data, but
info is dereferenced in several areas. We also use kmemdup to copy
driver_data under the assumption that it was provided. We could allow
for NULL if driver_data is referenced directly.

David

Re: [PATCH 1/3] mfd: intel_pmt: Add OOBMSM device ID

[Lee Jones]

On Tue, 29 Sep 2020, David E. Box wrote:

> On Tue, 2020-09-29 at 10:51 +0100, Lee Jones wrote:
> > On Fri, 11 Sep 2020, David E. Box wrote:
> > 
> > > Add Out of Band Management Services Module device ID to Intel PMT
> > > driver.
> > > 
> > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > ---
> > >  drivers/mfd/intel_pmt.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/mfd/intel_pmt.c b/drivers/mfd/intel_pmt.c
> > > index 0e572b105101..8f9970ab3026 100644
> > > --- a/drivers/mfd/intel_pmt.c
> > > +++ b/drivers/mfd/intel_pmt.c
> > > @@ -55,6 +55,8 @@ struct pmt_platform_info {
> > >  	unsigned long quirks;
> > >  };
> > >  
> > > +static const struct pmt_platform_info pmt_info;
> > > +
> > >  static const struct pmt_platform_info tgl_info = {
> > >  	.quirks = PMT_QUIRK_NO_WATCHER | PMT_QUIRK_NO_CRASHLOG |
> > >  		  PMT_QUIRK_TABLE_SHIFT,
> > > @@ -200,8 +202,10 @@ static void pmt_pci_remove(struct pci_dev
> > > *pdev)
> > >  	pm_runtime_get_sync(&pdev->dev);
> > >  }
> > >  
> > > +#define PCI_DEVICE_ID_INTEL_PMT_OOBMSM	0x09a7
> > >  #define PCI_DEVICE_ID_INTEL_PMT_TGL	0x9a0d
> > >  static const struct pci_device_id pmt_pci_ids[] = {
> > > +	{ PCI_DEVICE_DATA(INTEL, PMT_OOBMSM, &pmt_info) },
> > 
> > Why are you supplying an empty struct?
> 
> Because the OOBMSM device doesn't need code provided driver data, but
> info is dereferenced in several areas. We also use kmemdup to copy
> driver_data under the assumption that it was provided. We could allow
> for NULL if driver_data is referenced directly.

Just check for NULL.  No need to create and send bogus data.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/3] mfd: intel_pmt: Add OOBMSM device ID

[David E. Box]

On Wed, 2020-09-30 at 08:12 +0100, Lee Jones wrote:
> On Tue, 29 Sep 2020, David E. Box wrote:
> 
> > On Tue, 2020-09-29 at 10:51 +0100, Lee Jones wrote:
> > > On Fri, 11 Sep 2020, David E. Box wrote:
> > > 
> > > > Add Out of Band Management Services Module device ID to Intel
> > > > PMT
> > > > driver.
> > > > 
> > > > Signed-off-by: Alexander Duyck <
> > > > alexander.h.duyck@linux.intel.com>
> > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > ---
> > > >  drivers/mfd/intel_pmt.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/drivers/mfd/intel_pmt.c b/drivers/mfd/intel_pmt.c
> > > > index 0e572b105101..8f9970ab3026 100644
> > > > --- a/drivers/mfd/intel_pmt.c
> > > > +++ b/drivers/mfd/intel_pmt.c
> > > > @@ -55,6 +55,8 @@ struct pmt_platform_info {
> > > >  	unsigned long quirks;
> > > >  };
> > > >  
> > > > +static const struct pmt_platform_info pmt_info;
> > > > +
> > > >  static const struct pmt_platform_info tgl_info = {
> > > >  	.quirks = PMT_QUIRK_NO_WATCHER | PMT_QUIRK_NO_CRASHLOG
> > > > |
> > > >  		  PMT_QUIRK_TABLE_SHIFT,
> > > > @@ -200,8 +202,10 @@ static void pmt_pci_remove(struct pci_dev
> > > > *pdev)
> > > >  	pm_runtime_get_sync(&pdev->dev);
> > > >  }
> > > >  
> > > > +#define PCI_DEVICE_ID_INTEL_PMT_OOBMSM	0x09a7
> > > >  #define PCI_DEVICE_ID_INTEL_PMT_TGL	0x9a0d
> > > >  static const struct pci_device_id pmt_pci_ids[] = {
> > > > +	{ PCI_DEVICE_DATA(INTEL, PMT_OOBMSM, &pmt_info) },
> > > 
> > > Why are you supplying an empty struct?
> > 
> > Because the OOBMSM device doesn't need code provided driver data,
> > but
> > info is dereferenced in several areas. We also use kmemdup to copy
> > driver_data under the assumption that it was provided. We could
> > allow
> > for NULL if driver_data is referenced directly.
> 
> Just check for NULL.  No need to create and send bogus data.

Sure. If you haven't already, please note that this patch was pulled
into the V6 series in the link below. You accepted V5 but Hans
suggested some late changes after reviewing the new crashlog driver in
this patchset. So rather than have separate patchsets with a
dependency, we bundled them all into the original. We'll make these
changes in V7 now.

https://lore.kernel.org/patchwork/patch/1313166/

David

Re: [PATCH 1/3] mfd: intel_pmt: Add OOBMSM device ID

[Lee Jones]

On Wed, 30 Sep 2020, David E. Box wrote:

> On Wed, 2020-09-30 at 08:12 +0100, Lee Jones wrote:
> > On Tue, 29 Sep 2020, David E. Box wrote:
> > 
> > > On Tue, 2020-09-29 at 10:51 +0100, Lee Jones wrote:
> > > > On Fri, 11 Sep 2020, David E. Box wrote:
> > > > 
> > > > > Add Out of Band Management Services Module device ID to Intel
> > > > > PMT
> > > > > driver.
> > > > > 
> > > > > Signed-off-by: Alexander Duyck <
> > > > > alexander.h.duyck@linux.intel.com>
> > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > > ---
> > > > >  drivers/mfd/intel_pmt.c | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/mfd/intel_pmt.c b/drivers/mfd/intel_pmt.c
> > > > > index 0e572b105101..8f9970ab3026 100644
> > > > > --- a/drivers/mfd/intel_pmt.c
> > > > > +++ b/drivers/mfd/intel_pmt.c
> > > > > @@ -55,6 +55,8 @@ struct pmt_platform_info {
> > > > >  	unsigned long quirks;
> > > > >  };
> > > > >  
> > > > > +static const struct pmt_platform_info pmt_info;
> > > > > +
> > > > >  static const struct pmt_platform_info tgl_info = {
> > > > >  	.quirks = PMT_QUIRK_NO_WATCHER | PMT_QUIRK_NO_CRASHLOG
> > > > > |
> > > > >  		  PMT_QUIRK_TABLE_SHIFT,
> > > > > @@ -200,8 +202,10 @@ static void pmt_pci_remove(struct pci_dev
> > > > > *pdev)
> > > > >  	pm_runtime_get_sync(&pdev->dev);
> > > > >  }
> > > > >  
> > > > > +#define PCI_DEVICE_ID_INTEL_PMT_OOBMSM	0x09a7
> > > > >  #define PCI_DEVICE_ID_INTEL_PMT_TGL	0x9a0d
> > > > >  static const struct pci_device_id pmt_pci_ids[] = {
> > > > > +	{ PCI_DEVICE_DATA(INTEL, PMT_OOBMSM, &pmt_info) },
> > > > 
> > > > Why are you supplying an empty struct?
> > > 
> > > Because the OOBMSM device doesn't need code provided driver data,
> > > but
> > > info is dereferenced in several areas. We also use kmemdup to copy
> > > driver_data under the assumption that it was provided. We could
> > > allow
> > > for NULL if driver_data is referenced directly.
> > 
> > Just check for NULL.  No need to create and send bogus data.
> 
> Sure. If you haven't already, please note that this patch was pulled
> into the V6 series in the link below. You accepted V5 but Hans
> suggested some late changes after reviewing the new crashlog driver in
> this patchset. So rather than have separate patchsets with a
> dependency, we bundled them all into the original. We'll make these
> changes in V7 now.
> 
> https://lore.kernel.org/patchwork/patch/1313166/

Sounds reasonable. 

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog