* [PATCH V7 1/5] PCI: Add defines for Designated Vendor-Specific Extended Capability
2020-10-01 1:42 [PATCH V7 0/5] Intel Platform Monitoring Technology David E. Box
@ 2020-10-01 1:42 ` David E. Box
2020-10-01 1:42 ` [PATCH V7 2/5] mfd: Intel Platform Monitoring Technology support David E. Box
` (3 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: David E. Box @ 2020-10-01 1:42 UTC (permalink / raw)
To: lee.jones, david.e.box, dvhart, andy, bhelgaas,
alexander.h.duyck, hdegoede, alexey.budankov
Cc: linux-kernel, platform-driver-x86, linux-pci, Andy Shevchenko
Add PCIe Designated Vendor-Specific Extended Capability (DVSEC) and defines
for the header offsets. Defined in PCIe r5.0, sec 7.9.6.
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
include/uapi/linux/pci_regs.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index f9701410d3b5..beafeee39e44 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -720,6 +720,7 @@
#define PCI_EXT_CAP_ID_DPC 0x1D /* Downstream Port Containment */
#define PCI_EXT_CAP_ID_L1SS 0x1E /* L1 PM Substates */
#define PCI_EXT_CAP_ID_PTM 0x1F /* Precision Time Measurement */
+#define PCI_EXT_CAP_ID_DVSEC 0x23 /* Designated Vendor-Specific */
#define PCI_EXT_CAP_ID_DLF 0x25 /* Data Link Feature */
#define PCI_EXT_CAP_ID_PL_16GT 0x26 /* Physical Layer 16.0 GT/s */
#define PCI_EXT_CAP_ID_MAX PCI_EXT_CAP_ID_PL_16GT
@@ -1062,6 +1063,10 @@
#define PCI_L1SS_CTL1_LTR_L12_TH_SCALE 0xe0000000 /* LTR_L1.2_THRESHOLD_Scale */
#define PCI_L1SS_CTL2 0x0c /* Control 2 Register */
+/* Designated Vendor-Specific (DVSEC, PCI_EXT_CAP_ID_DVSEC) */
+#define PCI_DVSEC_HEADER1 0x4 /* Designated Vendor-Specific Header1 */
+#define PCI_DVSEC_HEADER2 0x8 /* Designated Vendor-Specific Header2 */
+
/* Data Link Feature */
#define PCI_DLF_CAP 0x04 /* Capabilities Register */
#define PCI_DLF_EXCHANGE_ENABLE 0x80000000 /* Data Link Feature Exchange Enable */
--
2.20.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH V7 2/5] mfd: Intel Platform Monitoring Technology support
2020-10-01 1:42 [PATCH V7 0/5] Intel Platform Monitoring Technology David E. Box
2020-10-01 1:42 ` [PATCH V7 1/5] PCI: Add defines for Designated Vendor-Specific Extended Capability David E. Box
@ 2020-10-01 1:42 ` David E. Box
2020-10-01 1:42 ` [PATCH V7 3/5] platform/x86: Intel PMT class driver David E. Box
` (2 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: David E. Box @ 2020-10-01 1:42 UTC (permalink / raw)
To: lee.jones, david.e.box, dvhart, andy, bhelgaas,
alexander.h.duyck, hdegoede, alexey.budankov
Cc: linux-kernel, platform-driver-x86, linux-pci, Andy Shevchenko
Intel Platform Monitoring Technology (PMT) is an architecture for
enumerating and accessing hardware monitoring facilities. PMT supports
multiple types of monitoring capabilities. This driver creates platform
devices for each type so that they may be managed by capability specific
drivers (to be introduced). Capabilities are discovered using PCIe DVSEC
ids. Support is included for the 3 current capability types, Telemetry,
Watcher, and Crashlog. The features are available on new Intel platforms
starting from Tiger Lake for which support is added. This patch adds
support for Tiger Lake (TGL), Alder Lake (ADL), and Out-of-Band Management
Services Module (OOBMSM).
Also add a quirk mechanism for several early hardware differences and bugs.
For Tiger Lake, do not support Watcher and Crashlog capabilities since they
will not be compatible with future product. Also, fix use a quirk to fix
the discovery table offset.
Co-developed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
MAINTAINERS | 5 +
drivers/mfd/Kconfig | 10 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/intel_pmt.c | 225 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 241 insertions(+)
create mode 100644 drivers/mfd/intel_pmt.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 190c7fa2ea01..0f2663b1d376 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8946,6 +8946,11 @@ F: drivers/mfd/intel_soc_pmic*
F: include/linux/mfd/intel_msic.h
F: include/linux/mfd/intel_soc_pmic*
+INTEL PMT DRIVER
+M: "David E. Box" <david.e.box@linux.intel.com>
+S: Maintained
+F: drivers/mfd/intel_pmt.c
+
INTEL PRO/WIRELESS 2100, 2200BG, 2915ABG NETWORK CONNECTION SUPPORT
M: Stanislav Yakovlev <stas.yakovlev@gmail.com>
L: linux-wireless@vger.kernel.org
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 33df0837ab41..f092db50e518 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -670,6 +670,16 @@ config MFD_INTEL_PMC_BXT
Register and P-unit access. In addition this creates devices
for iTCO watchdog and telemetry that are part of the PMC.
+config MFD_INTEL_PMT
+ tristate "Intel Platform Monitoring Technology (PMT) support"
+ depends on PCI
+ select MFD_CORE
+ help
+ The Intel Platform Monitoring Technology (PMT) is an interface that
+ provides access to hardware monitor registers. This driver supports
+ Telemetry, Watcher, and Crashlog PMT capabilities/devices for
+ platforms starting from Tiger Lake.
+
config MFD_IPAQ_MICRO
bool "Atmel Micro ASIC (iPAQ h3100/h3600/h3700) Support"
depends on SA1100_H3100 || SA1100_H3600
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a60e5f835283..b9565d98ab09 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -215,6 +215,7 @@ obj-$(CONFIG_MFD_INTEL_LPSS_PCI) += intel-lpss-pci.o
obj-$(CONFIG_MFD_INTEL_LPSS_ACPI) += intel-lpss-acpi.o
obj-$(CONFIG_MFD_INTEL_MSIC) += intel_msic.o
obj-$(CONFIG_MFD_INTEL_PMC_BXT) += intel_pmc_bxt.o
+obj-$(CONFIG_MFD_INTEL_PMT) += intel_pmt.o
obj-$(CONFIG_MFD_PALMAS) += palmas.o
obj-$(CONFIG_MFD_VIPERBOARD) += viperboard.o
obj-$(CONFIG_MFD_RC5T583) += rc5t583.o rc5t583-irq.o
diff --git a/drivers/mfd/intel_pmt.c b/drivers/mfd/intel_pmt.c
new file mode 100644
index 000000000000..71aabf8a38d5
--- /dev/null
+++ b/drivers/mfd/intel_pmt.c
@@ -0,0 +1,225 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Platform Monitoring Technology PMT driver
+ *
+ * Copyright (c) 2020, Intel Corporation.
+ * All Rights Reserved.
+ *
+ * Author: David E. Box <david.e.box@linux.intel.com>
+ */
+
+#include <linux/bits.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/types.h>
+
+/* Intel DVSEC capability vendor space offsets */
+#define INTEL_DVSEC_ENTRIES 0xA
+#define INTEL_DVSEC_SIZE 0xB
+#define INTEL_DVSEC_TABLE 0xC
+#define INTEL_DVSEC_TABLE_BAR(x) ((x) & GENMASK(2, 0))
+#define INTEL_DVSEC_TABLE_OFFSET(x) ((x) & GENMASK(31, 3))
+#define INTEL_DVSEC_ENTRY_SIZE 4
+
+/* PMT capabilities */
+#define DVSEC_INTEL_ID_TELEMETRY 2
+#define DVSEC_INTEL_ID_WATCHER 3
+#define DVSEC_INTEL_ID_CRASHLOG 4
+
+struct intel_dvsec_header {
+ u16 length;
+ u16 id;
+ u8 num_entries;
+ u8 entry_size;
+ u8 tbir;
+ u32 offset;
+};
+
+enum pmt_quirks {
+ /* Watcher capability not supported */
+ PMT_QUIRK_NO_WATCHER = BIT(0),
+
+ /* Crashlog capability not supported */
+ PMT_QUIRK_NO_CRASHLOG = BIT(1),
+
+ /* Use shift instead of mask to read discovery table offset */
+ PMT_QUIRK_TABLE_SHIFT = BIT(2),
+};
+
+struct pmt_platform_info {
+ unsigned long quirks;
+};
+
+static const struct pmt_platform_info tgl_info = {
+ .quirks = PMT_QUIRK_NO_WATCHER | PMT_QUIRK_NO_CRASHLOG |
+ PMT_QUIRK_TABLE_SHIFT,
+};
+
+static int pmt_add_dev(struct pci_dev *pdev, struct intel_dvsec_header *header,
+ unsigned long quirks)
+{
+ struct device *dev = &pdev->dev;
+ struct resource *res, *tmp;
+ struct mfd_cell *cell;
+ const char *name;
+ int count = header->num_entries;
+ int size = header->entry_size;
+ int id = header->id;
+ int i;
+
+ switch (id) {
+ case DVSEC_INTEL_ID_TELEMETRY:
+ name = "pmt_telemetry";
+ break;
+ case DVSEC_INTEL_ID_WATCHER:
+ if (quirks & PMT_QUIRK_NO_WATCHER) {
+ dev_info(dev, "Watcher not supported\n");
+ return 0;
+ }
+ name = "pmt_watcher";
+ break;
+ case DVSEC_INTEL_ID_CRASHLOG:
+ if (quirks & PMT_QUIRK_NO_CRASHLOG) {
+ dev_info(dev, "Crashlog not supported\n");
+ return 0;
+ }
+ name = "pmt_crashlog";
+ break;
+ default:
+ dev_err(dev, "Unrecognized PMT capability: %d\n", id);
+ return -EINVAL;
+ }
+
+ if (!header->num_entries || !header->entry_size) {
+ dev_err(dev, "Invalid count or size for %s header\n", name);
+ return -EINVAL;
+ }
+
+ cell = devm_kzalloc(dev, sizeof(*cell), GFP_KERNEL);
+ if (!cell)
+ return -ENOMEM;
+
+ res = devm_kcalloc(dev, count, sizeof(*res), GFP_KERNEL);
+ if (!res)
+ return -ENOMEM;
+
+ if (quirks & PMT_QUIRK_TABLE_SHIFT)
+ header->offset >>= 3;
+
+ /*
+ * The PMT DVSEC contains the starting offset and count for a block of
+ * discovery tables, each providing access to monitoring facilities for
+ * a section of the device. Create a resource list of these tables to
+ * provide to the driver.
+ */
+ for (i = 0, tmp = res; i < count; i++, tmp++) {
+ tmp->start = pdev->resource[header->tbir].start +
+ header->offset + i * (size << 2);
+ tmp->end = tmp->start + (size << 2) - 1;
+ tmp->flags = IORESOURCE_MEM;
+ }
+
+ cell->resources = res;
+ cell->num_resources = count;
+ cell->name = name;
+
+ return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cell, 1, NULL, 0,
+ NULL);
+}
+
+static int pmt_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+ struct pmt_platform_info *info;
+ unsigned long quirks = 0;
+ bool found_devices = false;
+ int ret, pos = 0;
+
+ ret = pcim_enable_device(pdev);
+ if (ret)
+ return ret;
+
+ info = (struct pmt_platform_info *)id->driver_data;
+
+ if (info)
+ quirks = info->quirks;
+
+ do {
+ struct intel_dvsec_header header;
+ u32 table;
+ u16 vid;
+
+ pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC);
+ if (!pos)
+ break;
+
+ pci_read_config_word(pdev, pos + PCI_DVSEC_HEADER1, &vid);
+ if (vid != PCI_VENDOR_ID_INTEL)
+ continue;
+
+ pci_read_config_word(pdev, pos + PCI_DVSEC_HEADER2,
+ &header.id);
+ pci_read_config_byte(pdev, pos + INTEL_DVSEC_ENTRIES,
+ &header.num_entries);
+ pci_read_config_byte(pdev, pos + INTEL_DVSEC_SIZE,
+ &header.entry_size);
+ pci_read_config_dword(pdev, pos + INTEL_DVSEC_TABLE,
+ &table);
+
+ header.tbir = INTEL_DVSEC_TABLE_BAR(table);
+ header.offset = INTEL_DVSEC_TABLE_OFFSET(table);
+
+ ret = pmt_add_dev(pdev, &header, quirks);
+ if (ret) {
+ dev_warn(&pdev->dev,
+ "Failed to add device for DVSEC id %d\n",
+ header.id);
+ continue;
+ }
+
+ found_devices = true;
+ } while (true);
+
+ if (!found_devices) {
+ dev_err(&pdev->dev, "No supported PMT capabilities found.\n");
+ return -ENODEV;
+ }
+
+ pm_runtime_put(&pdev->dev);
+ pm_runtime_allow(&pdev->dev);
+
+ return 0;
+}
+
+static void pmt_pci_remove(struct pci_dev *pdev)
+{
+ pm_runtime_forbid(&pdev->dev);
+ 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, NULL) },
+ { PCI_DEVICE_DATA(INTEL, PMT_TGL, &tgl_info) },
+ { }
+};
+MODULE_DEVICE_TABLE(pci, pmt_pci_ids);
+
+static struct pci_driver pmt_pci_driver = {
+ .name = "intel-pmt",
+ .id_table = pmt_pci_ids,
+ .probe = pmt_pci_probe,
+ .remove = pmt_pci_remove,
+};
+module_pci_driver(pmt_pci_driver);
+
+MODULE_AUTHOR("David E. Box <david.e.box@linux.intel.com>");
+MODULE_DESCRIPTION("Intel Platform Monitoring Technology PMT driver");
+MODULE_LICENSE("GPL v2");
--
2.20.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH V7 3/5] platform/x86: Intel PMT class driver
2020-10-01 1:42 [PATCH V7 0/5] Intel Platform Monitoring Technology David E. Box
2020-10-01 1:42 ` [PATCH V7 1/5] PCI: Add defines for Designated Vendor-Specific Extended Capability David E. Box
2020-10-01 1:42 ` [PATCH V7 2/5] mfd: Intel Platform Monitoring Technology support David E. Box
@ 2020-10-01 1:42 ` David E. Box
2020-10-01 16:23 ` Andy Shevchenko
2020-10-01 1:42 ` [PATCH V7 4/5] platform/x86: Intel PMT Telemetry capability driver David E. Box
2020-10-01 1:42 ` [PATCH V7 5/5] platform/x86: Intel PMT Crashlog " David E. Box
4 siblings, 1 reply; 17+ messages in thread
From: David E. Box @ 2020-10-01 1:42 UTC (permalink / raw)
To: lee.jones, david.e.box, dvhart, andy, bhelgaas,
alexander.h.duyck, hdegoede, alexey.budankov
Cc: linux-kernel, platform-driver-x86, linux-pci
From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
Intel Platform Monitoring Technology is meant to provide a common way to
access telemetry and system metrics.
Register mappings are not provided by the driver. Instead, a GUID is read
from a header for each endpoint. The GUID identifies the device and is to
be used with an XML, provided by the vendor, to discover the available set
of metrics and their register mapping. This allows firmware updates to
modify the register space without needing to update the driver every time
with new mappings. Firmware writes a new GUID in this case to specify the
new mapping. Software tools with access to the associated XML file can
then interpret the changes.
The module manages access to all Intel PMT endpoints on a system,
independent of the device exporting them. It creates an intel_pmt class to
manage the devices. For each telemetry endpoint, sysfs files provide GUID
and size information as well as a pointer to the parent device the
telemetry came from. Software may discover the association between
endpoints and devices by iterating through the list in sysfs, or by looking
for the existence of the class folder under the device of interest. A
binary sysfs attribute of the same name allows software to then read or map
the telemetry space for direct access.
Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
.../ABI/testing/sysfs-class-intel_pmt | 54 ++++
MAINTAINERS | 1 +
drivers/platform/x86/Kconfig | 9 +
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/intel_pmt_class.c | 286 ++++++++++++++++++
drivers/platform/x86/intel_pmt_class.h | 57 ++++
6 files changed, 408 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-intel_pmt
create mode 100644 drivers/platform/x86/intel_pmt_class.c
create mode 100644 drivers/platform/x86/intel_pmt_class.h
diff --git a/Documentation/ABI/testing/sysfs-class-intel_pmt b/Documentation/ABI/testing/sysfs-class-intel_pmt
new file mode 100644
index 000000000000..926b5cf95fd1
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-intel_pmt
@@ -0,0 +1,54 @@
+What: /sys/class/intel_pmt/
+Date: October 2020
+KernelVersion: 5.10
+Contact: David Box <david.e.box@linux.intel.com>
+Description:
+ The intel_pmt/ class directory contains information for
+ devices that expose hardware telemetry using Intel Platform
+ Monitoring Technology (PMT)
+
+What: /sys/class/intel_pmt/telem<x>
+Date: October 2020
+KernelVersion: 5.10
+Contact: David Box <david.e.box@linux.intel.com>
+Description:
+ The telem<x> directory contains files describing an instance of
+ a PMT telemetry device that exposes hardware telemetry. Each
+ telem<x> directory has an associated telem file. This file
+ may be opened and mapped or read to access the telemetry space
+ of the device. The register layout of the telemetry space is
+ determined from an XML file that matches the PCI device id and
+ GUID for the device.
+
+What: /sys/class/intel_pmt/telem<x>/telem
+Date: October 2020
+KernelVersion: 5.10
+Contact: David Box <david.e.box@linux.intel.com>
+Description:
+ (RO) The telemetry data for this telemetry device. This file
+ may be mapped or read to obtain the data.
+
+What: /sys/class/intel_pmt/telem<x>/guid
+Date: October 2020
+KernelVersion: 5.10
+Contact: David Box <david.e.box@linux.intel.com>
+Description:
+ (RO) The GUID for this telemetry device. The GUID identifies
+ the version of the XML file for the parent device that is to
+ be used to get the register layout.
+
+What: /sys/class/intel_pmt/telem<x>/size
+Date: October 2020
+KernelVersion: 5.10
+Contact: David Box <david.e.box@linux.intel.com>
+Description:
+ (RO) The size of telemetry region in bytes that corresponds to
+ the mapping size for the telem file.
+
+What: /sys/class/intel_pmt/telem<x>/offset
+Date: October 2020
+KernelVersion: 5.10
+Contact: David Box <david.e.box@linux.intel.com>
+Description:
+ (RO) The offset of telemetry region in bytes that corresponds to
+ the mapping for the telem file.
diff --git a/MAINTAINERS b/MAINTAINERS
index 0f2663b1d376..47fdb8a6e151 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8950,6 +8950,7 @@ INTEL PMT DRIVER
M: "David E. Box" <david.e.box@linux.intel.com>
S: Maintained
F: drivers/mfd/intel_pmt.c
+F: drivers/platform/x86/intel_pmt_*
INTEL PRO/WIRELESS 2100, 2200BG, 2915ABG NETWORK CONNECTION SUPPORT
M: Stanislav Yakovlev <stas.yakovlev@gmail.com>
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 40219bba6801..82465d0e8fd3 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1360,6 +1360,15 @@ config INTEL_PMC_CORE
- LTR Ignore
- MPHY/PLL gating status (Sunrisepoint PCH only)
+config INTEL_PMT_CLASS
+ tristate "Intel Platform Monitoring Technology (PMT) Class driver"
+ help
+ The Intel Platform Monitoring Technology (PMT) class driver provides
+ the basic sysfs interface and file hierarchy uses by PMT devices.
+
+ For more information, see:
+ <file:Documentation/ABI/testing/sysfs-class-intel_pmt>
+
config INTEL_PUNIT_IPC
tristate "Intel P-Unit IPC Driver"
help
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 5f823f7eff45..f4b1f87f2401 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_CLASS) += intel_pmt_class.o
obj-$(CONFIG_INTEL_PUNIT_IPC) += intel_punit_ipc.o
obj-$(CONFIG_INTEL_SCU_IPC) += intel_scu_ipc.o
obj-$(CONFIG_INTEL_SCU_PCI) += intel_scu_pcidrv.o
diff --git a/drivers/platform/x86/intel_pmt_class.c b/drivers/platform/x86/intel_pmt_class.c
new file mode 100644
index 000000000000..d7726042d6dc
--- /dev/null
+++ b/drivers/platform/x86/intel_pmt_class.c
@@ -0,0 +1,286 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Platform Monitory Technology Telemetry driver
+ *
+ * Copyright (c) 2020, Intel Corporation.
+ * All Rights Reserved.
+ *
+ * Author: "Alexander Duyck" <alexander.h.duyck@linux.intel.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mm.h>
+#include <linux/io.h>
+#include <linux/pci.h>
+
+#include "intel_pmt_class.h"
+
+#define PMT_XA_START 0
+#define PMT_XA_MAX INT_MAX
+#define PMT_XA_LIMIT XA_LIMIT(PMT_XA_START, PMT_XA_MAX)
+
+static const struct pci_device_id pmt_telem_early_client_pci_ids[] = {
+ { PCI_VDEVICE(INTEL, 0x9a0d) }, /* TGL */
+ { }
+};
+
+bool intel_pmt_is_early_client_hw(struct device *dev)
+{
+ struct pci_dev *parent = to_pci_dev(dev->parent);
+
+ return !!pci_match_id(pmt_telem_early_client_pci_ids, parent);
+}
+EXPORT_SYMBOL_GPL(intel_pmt_is_early_client_hw);
+
+/*
+ * sysfs
+ */
+static ssize_t
+intel_pmt_read(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *attr, char *buf, loff_t off,
+ size_t count)
+{
+ struct intel_pmt_entry *entry = container_of(attr,
+ struct intel_pmt_entry,
+ pmt_bin_attr);
+
+ if (off < 0)
+ return -EINVAL;
+
+ if (off >= entry->size)
+ return 0;
+
+ if (count > entry->size - off)
+ count = entry->size - off;
+
+ if (count)
+ memcpy_fromio(buf, entry->base + off, count);
+
+ return count;
+}
+
+static int
+intel_pmt_mmap(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *attr, struct vm_area_struct *vma)
+{
+ struct intel_pmt_entry *entry = container_of(attr,
+ struct intel_pmt_entry,
+ pmt_bin_attr);
+ unsigned long vsize = vma->vm_end - vma->vm_start;
+ struct device *dev = kobj_to_dev(kobj);
+ unsigned long phys = entry->base_addr;
+ unsigned long pfn = PFN_DOWN(phys);
+ unsigned long psize;
+
+ if (vma->vm_flags & (VM_WRITE | VM_MAYWRITE))
+ return -EROFS;
+
+ psize = (PFN_UP(entry->base_addr + entry->size) - pfn) * PAGE_SIZE;
+ if (vsize > psize) {
+ dev_err(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 ssize_t
+guid_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct intel_pmt_entry *entry = dev_get_drvdata(dev);
+
+ return sprintf(buf, "0x%x\n", entry->guid);
+}
+static DEVICE_ATTR_RO(guid);
+
+static ssize_t size_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct intel_pmt_entry *entry = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%zu\n", entry->size);
+}
+static DEVICE_ATTR_RO(size);
+
+static ssize_t
+offset_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct intel_pmt_entry *entry = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%lu\n", offset_in_page(entry->base_addr));
+}
+static DEVICE_ATTR_RO(offset);
+
+static struct attribute *intel_pmt_attrs[] = {
+ &dev_attr_guid.attr,
+ &dev_attr_size.attr,
+ &dev_attr_offset.attr,
+ NULL
+};
+
+ATTRIBUTE_GROUPS(intel_pmt);
+
+static struct class intel_pmt_class = {
+ .name = "intel_pmt",
+ .owner = THIS_MODULE,
+ .dev_groups = intel_pmt_groups,
+};
+
+int intel_pmt_populate_entry(struct intel_pmt_entry *entry,
+ struct intel_pmt_header *header,
+ struct device *dev, struct resource *disc_res)
+{
+ struct pci_dev *pci_dev = to_pci_dev(dev->parent);
+ u8 bir;
+
+ /*
+ * The base offset should always be 8 byte aligned.
+ *
+ * For non-local access types the lower 3 bits of base offset
+ * contains the index of the base address register where the
+ * telemetry can be found.
+ */
+ bir = GET_BIR(header->base_offset);
+
+ /* Local access and BARID only for now */
+ switch (header->access_type) {
+ case ACCESS_LOCAL:
+ if (bir) {
+ dev_err(dev,
+ "Unsupported BAR index %d for access type %d\n",
+ bir, header->access_type);
+ return -EINVAL;
+ }
+ /*
+ * For access_type LOCAL, the base address is as follows:
+ * base address = end of discovery region + base offset
+ */
+ entry->base_addr = disc_res->end + 1 + header->base_offset;
+ break;
+ case ACCESS_BARID:
+ /*
+ * If another BAR was specified then the base offset
+ * represents the offset within that BAR. SO retrieve the
+ * address from the parent PCI device and add offset.
+ */
+ entry->base_addr = pci_resource_start(pci_dev, bir) +
+ GET_ADDRESS(header->base_offset);
+ break;
+ default:
+ dev_err(dev, "Unsupported access type %d\n",
+ header->access_type);
+ return -EINVAL;
+ }
+
+ entry->guid = header->guid;
+ entry->size = header->size;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmt_populate_entry);
+
+int intel_pmt_dev_create(struct intel_pmt_entry *entry,
+ struct intel_pmt_namespace *ns, struct device *parent)
+{
+ struct resource res;
+ struct device *dev;
+ int ret;
+
+ ret = xa_alloc(ns->xa, &entry->devid, entry, PMT_XA_LIMIT, GFP_KERNEL);
+ if (ret)
+ return ret;
+
+ dev = device_create(&intel_pmt_class, parent, MKDEV(0, 0), entry,
+ "%s%d", ns->name, entry->devid);
+
+ if (IS_ERR(dev)) {
+ dev_err(parent, "Could not create %s%d device node\n",
+ ns->name, entry->devid);
+ ret = PTR_ERR(dev);
+ goto fail_dev_create;
+ }
+
+ entry->kobj = &dev->kobj;
+
+ if (ns->attr_grp) {
+ ret = sysfs_create_group(entry->kobj, ns->attr_grp);
+ if (ret)
+ goto fail_sysfs;
+ }
+
+ /* if size is 0 assume no data buffer, so no file needed */
+ if (!entry->size)
+ return 0;
+
+ res.start = entry->base_addr;
+ res.end = res.start + entry->size - 1;
+ res.flags = IORESOURCE_MEM;
+
+ entry->base = devm_ioremap_resource(dev, &res);
+ if (IS_ERR(entry->base)) {
+ dev_err(dev, "Failed to ioremap device region\n");
+ ret = -EIO;
+ goto fail_ioremap;
+ }
+
+ sysfs_bin_attr_init(&entry->pmt_bin_attr);
+ entry->pmt_bin_attr.attr.name = ns->name;
+ entry->pmt_bin_attr.attr.mode = 0440;
+ entry->pmt_bin_attr.mmap = intel_pmt_mmap;
+ entry->pmt_bin_attr.read = intel_pmt_read;
+ entry->pmt_bin_attr.size = entry->size;
+
+ ret = sysfs_create_bin_file(&dev->kobj, &entry->pmt_bin_attr);
+ if (!ret)
+ return 0;
+
+ iounmap(entry->base);
+fail_ioremap:
+ sysfs_remove_group(entry->kobj, ns->attr_grp);
+fail_sysfs:
+ device_unregister(dev);
+fail_dev_create:
+ xa_erase(ns->xa, entry->devid);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(intel_pmt_dev_create);
+
+void intel_pmt_dev_destroy(struct intel_pmt_entry *entry,
+ struct intel_pmt_namespace *ns)
+{
+ struct device *dev = kobj_to_dev(entry->kobj);
+
+ if (entry->size)
+ sysfs_remove_bin_file(entry->kobj, &entry->pmt_bin_attr);
+
+ if (ns->attr_grp)
+ sysfs_remove_group(entry->kobj, ns->attr_grp);
+
+ device_unregister(dev);
+ xa_erase(ns->xa, entry->devid);
+}
+EXPORT_SYMBOL_GPL(intel_pmt_dev_destroy);
+
+static int __init pmt_class_init(void)
+{
+ return class_register(&intel_pmt_class);
+}
+
+static void __exit pmt_class_exit(void)
+{
+ class_unregister(&intel_pmt_class);
+}
+
+module_init(pmt_class_init);
+module_exit(pmt_class_exit);
+
+MODULE_AUTHOR("Alexander Duyck <alexander.h.duyck@linux.intel.com>");
+MODULE_DESCRIPTION("Intel PMT Class driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/platform/x86/intel_pmt_class.h b/drivers/platform/x86/intel_pmt_class.h
new file mode 100644
index 000000000000..ccb4e56a1984
--- /dev/null
+++ b/drivers/platform/x86/intel_pmt_class.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _INTEL_PMT_CLASS_H
+#define _INTEL_PMT_CLASS_H
+
+#include <linux/platform_device.h>
+#include <linux/xarray.h>
+
+/* PMT access types */
+#define ACCESS_BARID 2
+#define ACCESS_LOCAL 3
+
+/* PMT discovery base address/offset register layout */
+#define GET_BIR(v) ((v) & GENMASK(2, 0))
+#define GET_ADDRESS(v) ((v) & GENMASK(31, 3))
+
+struct intel_pmt_entry {
+ struct bin_attribute pmt_bin_attr;
+ struct kobject *kobj;
+ void __iomem *disc_table;
+ void __iomem *base;
+ unsigned long base_addr;
+ size_t size;
+ u32 guid;
+ int devid;
+};
+
+struct intel_pmt_header {
+ u32 base_offset;
+ u32 size;
+ u32 guid;
+ u8 access_type;
+};
+
+struct intel_pmt_namespace {
+ const char *name;
+ struct xarray *xa;
+ const struct attribute_group *attr_grp;
+};
+
+bool intel_pmt_is_early_client_hw(struct device *dev);
+int intel_pmt_populate_entry(struct intel_pmt_entry *entry,
+ struct intel_pmt_header *header,
+ struct device *dev, struct resource *disc_res);
+int intel_pmt_dev_create(struct intel_pmt_entry *entry,
+ struct intel_pmt_namespace *ns, struct device *parent);
+void intel_pmt_dev_destroy(struct intel_pmt_entry *entry,
+ struct intel_pmt_namespace *ns);
+
+static inline int
+intel_pmt_ioremap_discovery_table(struct intel_pmt_entry *entry,
+ struct platform_device *pdev, int i)
+{
+ entry->disc_table = devm_platform_ioremap_resource(pdev, i);
+
+ return PTR_ERR_OR_ZERO(entry->disc_table);
+}
+#endif
--
2.20.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH V7 3/5] platform/x86: Intel PMT class driver
2020-10-01 1:42 ` [PATCH V7 3/5] platform/x86: Intel PMT class driver David E. Box
@ 2020-10-01 16:23 ` Andy Shevchenko
2020-10-01 17:44 ` Alexander Duyck
0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2020-10-01 16:23 UTC (permalink / raw)
To: David E. Box
Cc: Lee Jones, Darren Hart, Andy Shevchenko, Bjorn Helgaas,
Alexander Duyck, Hans de Goede, alexey.budankov,
Linux Kernel Mailing List, Platform Driver, linux-pci
On Thu, Oct 1, 2020 at 4:43 AM David E. Box <david.e.box@linux.intel.com> wrote:
>
> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>
> Intel Platform Monitoring Technology is meant to provide a common way to
> access telemetry and system metrics.
>
> Register mappings are not provided by the driver. Instead, a GUID is read
> from a header for each endpoint. The GUID identifies the device and is to
> be used with an XML, provided by the vendor, to discover the available set
> of metrics and their register mapping. This allows firmware updates to
> modify the register space without needing to update the driver every time
> with new mappings. Firmware writes a new GUID in this case to specify the
> new mapping. Software tools with access to the associated XML file can
> then interpret the changes.
Where one may find a database of these reserved GUIDs / XMLs?
How do you prevent a chaos which happens with other registries?
> The module manages access to all Intel PMT endpoints on a system,
> independent of the device exporting them. It creates an intel_pmt class to
> manage the devices. For each telemetry endpoint, sysfs files provide GUID
> and size information as well as a pointer to the parent device the
> telemetry came from. Software may discover the association between
> endpoints and devices by iterating through the list in sysfs, or by looking
> for the existence of the class folder under the device of interest. A
> binary sysfs attribute of the same name allows software to then read or map
> the telemetry space for direct access.
What are the security implications by direct access?
...
> +static const struct pci_device_id pmt_telem_early_client_pci_ids[] = {
> + { PCI_VDEVICE(INTEL, 0x9a0d) }, /* TGL */
> + { }
> +};
> +bool intel_pmt_is_early_client_hw(struct device *dev)
> +{
> + struct pci_dev *parent = to_pci_dev(dev->parent);
> +
> + return !!pci_match_id(pmt_telem_early_client_pci_ids, parent);
> +}
> +EXPORT_SYMBOL_GPL(intel_pmt_is_early_client_hw);
What is this and why is it in the class driver?
> +static ssize_t
> +intel_pmt_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *attr, char *buf, loff_t off,
> + size_t count)
> +{
> + struct intel_pmt_entry *entry = container_of(attr,
> + struct intel_pmt_entry,
> + pmt_bin_attr);
> + if (off < 0)
> + return -EINVAL;
Is this real or theoretical?
> + if (count)
Useless.
> + memcpy_fromio(buf, entry->base + off, count);
> +
> + return count;
> +}
...
> + psize = (PFN_UP(entry->base_addr + entry->size) - pfn) * PAGE_SIZE;
PFN_PHYS(PFN_UP(...)) ?
...
> +static struct attribute *intel_pmt_attrs[] = {
> + &dev_attr_guid.attr,
> + &dev_attr_size.attr,
> + &dev_attr_offset.attr,
> + NULL
> +};
> +
Unneeded blank line.
> +ATTRIBUTE_GROUPS(intel_pmt);
...
> + /* if size is 0 assume no data buffer, so no file needed */
> + if (!entry->size)
> + return 0;
Hmm... But presence of the file is also an information that might be
useful for user, no?
...
> + entry->base = devm_ioremap_resource(dev, &res);
(1)
> + if (IS_ERR(entry->base)) {
> + dev_err(dev, "Failed to ioremap device region\n");
Duplicates core message.
> + ret = -EIO;
Why shadowing real error code?
> + goto fail_ioremap;
> + }
> + iounmap(entry->base);
This is interesting. How do you avoid double unmap with (1)?
> +#include <linux/platform_device.h>
> +#include <linux/xarray.h>
> +
> +/* PMT access types */
> +#define ACCESS_BARID 2
> +#define ACCESS_LOCAL 3
> +
> +/* PMT discovery base address/offset register layout */
> +#define GET_BIR(v) ((v) & GENMASK(2, 0))
> +#define GET_ADDRESS(v) ((v) & GENMASK(31, 3))
bits.h
> +struct intel_pmt_entry {
> + struct bin_attribute pmt_bin_attr;
> + struct kobject *kobj;
> + void __iomem *disc_table;
> + void __iomem *base;
> + unsigned long base_addr;
> + size_t size;
> + u32 guid;
types.h
> + int devid;
> +};
> +static inline int
> +intel_pmt_ioremap_discovery_table(struct intel_pmt_entry *entry,
> + struct platform_device *pdev, int i)
> +{
> + entry->disc_table = devm_platform_ioremap_resource(pdev, i);
io.h ?
> +
> + return PTR_ERR_OR_ZERO(entry->disc_table);
err.h
> +}
The rule of thumb is to include all headers that you have direct users of.
Then you may optimize by removing those which are guaranteed to be
included by others, like
bits.h always included by bitops.h.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V7 3/5] platform/x86: Intel PMT class driver
2020-10-01 16:23 ` Andy Shevchenko
@ 2020-10-01 17:44 ` Alexander Duyck
2020-10-01 18:06 ` Andy Shevchenko
0 siblings, 1 reply; 17+ messages in thread
From: Alexander Duyck @ 2020-10-01 17:44 UTC (permalink / raw)
To: Andy Shevchenko
Cc: David E. Box, Lee Jones, Darren Hart, Andy Shevchenko,
Bjorn Helgaas, Alexander Duyck, Hans de Goede, Alexey Budankov,
Linux Kernel Mailing List, Platform Driver, linux-pci
On Thu, Oct 1, 2020 at 9:26 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Oct 1, 2020 at 4:43 AM David E. Box <david.e.box@linux.intel.com> wrote:
> >
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > Intel Platform Monitoring Technology is meant to provide a common way to
> > access telemetry and system metrics.
> >
> > Register mappings are not provided by the driver. Instead, a GUID is read
> > from a header for each endpoint. The GUID identifies the device and is to
> > be used with an XML, provided by the vendor, to discover the available set
> > of metrics and their register mapping. This allows firmware updates to
> > modify the register space without needing to update the driver every time
> > with new mappings. Firmware writes a new GUID in this case to specify the
> > new mapping. Software tools with access to the associated XML file can
> > then interpret the changes.
>
> Where one may find a database of these reserved GUIDs / XMLs?
> How do you prevent a chaos which happens with other registries?
The database will be posted on intel.com eventually. Although I don't
believe the URL is public yet.
> > The module manages access to all Intel PMT endpoints on a system,
> > independent of the device exporting them. It creates an intel_pmt class to
> > manage the devices. For each telemetry endpoint, sysfs files provide GUID
> > and size information as well as a pointer to the parent device the
> > telemetry came from. Software may discover the association between
> > endpoints and devices by iterating through the list in sysfs, or by looking
> > for the existence of the class folder under the device of interest. A
> > binary sysfs attribute of the same name allows software to then read or map
> > the telemetry space for direct access.
>
> What are the security implications by direct access?
In this case minimal as it would really be no different than the read.
The registers in the memory regions themselves are read-only with no
read side effects.
> ...
>
> > +static const struct pci_device_id pmt_telem_early_client_pci_ids[] = {
> > + { PCI_VDEVICE(INTEL, 0x9a0d) }, /* TGL */
> > + { }
> > +};
> > +bool intel_pmt_is_early_client_hw(struct device *dev)
> > +{
> > + struct pci_dev *parent = to_pci_dev(dev->parent);
> > +
> > + return !!pci_match_id(pmt_telem_early_client_pci_ids, parent);
> > +}
> > +EXPORT_SYMBOL_GPL(intel_pmt_is_early_client_hw);
>
> What is this and why is it in the class driver?
I chose to use the class driver as a central place to store code
common to all of the instances of the class. In this case we have
quirks that are specific to Tiger Lake and so I chose to store the
function to test for the device here.
> > +static ssize_t
> > +intel_pmt_read(struct file *filp, struct kobject *kobj,
> > + struct bin_attribute *attr, char *buf, loff_t off,
> > + size_t count)
> > +{
> > + struct intel_pmt_entry *entry = container_of(attr,
> > + struct intel_pmt_entry,
> > + pmt_bin_attr);
>
> > + if (off < 0)
> > + return -EINVAL;
> Is this real or theoretical?
Not sure. I am not that familiar with the interface. It was something
I copied from read_bmof which is what I based this code on based on an
earlier suggestion.
> > + if (count)
>
> Useless.
I'm assuming that is because memcpy_fromio is assumed to handle this case?
> > + memcpy_fromio(buf, entry->base + off, count);
> > +
> > + return count;
> > +}
>
> ...
>
> > + psize = (PFN_UP(entry->base_addr + entry->size) - pfn) * PAGE_SIZE;
>
> PFN_PHYS(PFN_UP(...)) ?
I'm not sure how that would work. Basically what we are doing here is
determining the size of the mapping based on the number of pages that
will be needed. So we wake the pfn of the start of the region,
subtract that from the pfn for the end of the region and multiply by
the size of a page.
> > +static struct attribute *intel_pmt_attrs[] = {
> > + &dev_attr_guid.attr,
> > + &dev_attr_size.attr,
> > + &dev_attr_offset.attr,
> > + NULL
> > +};
>
> > +
>
> Unneeded blank line.
>
> > +ATTRIBUTE_GROUPS(intel_pmt);
>
> ...
>
> > + /* if size is 0 assume no data buffer, so no file needed */
> > + if (!entry->size)
> > + return 0;
>
> Hmm... But presence of the file is also an information that might be
> useful for user, no?
I'm not sure what you mean? If the size of the region is zero it means
there is no data there. There are cases in future devices where we may
have controls for a telemetry function that is actually streaming the
data elsewhere. That will be the use case for the entry size of 0 and
in that case it doesn't make any sense to have a file as there is no
data to present in it.
> ...
>
> > + entry->base = devm_ioremap_resource(dev, &res);
>
> (1)
>
> > + if (IS_ERR(entry->base)) {
>
> > + dev_err(dev, "Failed to ioremap device region\n");
>
> Duplicates core message.
I'll drop it since it is a redundant.
> > + ret = -EIO;
>
> Why shadowing real error code?
I will just convert it instead. I think there might have been a bug
here in an earlier version where it was testing for NULL instead.
> > + goto fail_ioremap;
> > + }
>
> > + iounmap(entry->base);
>
> This is interesting. How do you avoid double unmap with (1)?
I think I get what you are trying to say. This is redundant since we
used the devm_ioremap_resource it will already be freed when the
driver is detached, correct?
> > +#include <linux/platform_device.h>
> > +#include <linux/xarray.h>
> > +
> > +/* PMT access types */
> > +#define ACCESS_BARID 2
> > +#define ACCESS_LOCAL 3
> > +
> > +/* PMT discovery base address/offset register layout */
> > +#define GET_BIR(v) ((v) & GENMASK(2, 0))
> > +#define GET_ADDRESS(v) ((v) & GENMASK(31, 3))
>
> bits.h
That is already included from a few different sources.
> > +struct intel_pmt_entry {
> > + struct bin_attribute pmt_bin_attr;
> > + struct kobject *kobj;
> > + void __iomem *disc_tabl
> > + void __iomem *base;
> > + unsigned long base_addr;
> > + size_t size;
>
> > + u32 guid;
>
> types.h
Is included through xarray.h.
> > + int devid;
> > +};
>
> > +static inline int
> > +intel_pmt_ioremap_discovery_table(struct intel_pmt_entry *entry,
> > + struct platform_device *pdev, int i)
> > +{
>
> > + entry->disc_table = devm_platform_ioremap_resource(pdev, i);
>
> io.h ?
That one I will move from the class.c file.
> > +
> > + return PTR_ERR_OR_ZERO(entry->disc_table);
>
> err.h
That is included as a part of io.h.
> > +}
>
> The rule of thumb is to include all headers that you have direct users of.
> Then you may optimize by removing those which are guaranteed to be
> included by others, like bits.h always included by bitops.h.
Yeah, from what I can tell the only one I didn't have was io.h and
part of that is because this was something I had moved to the header
file in order to commonize it since it was being used in the other
drivers.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V7 3/5] platform/x86: Intel PMT class driver
2020-10-01 17:44 ` Alexander Duyck
@ 2020-10-01 18:06 ` Andy Shevchenko
2020-10-01 19:02 ` Alexander Duyck
0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2020-10-01 18:06 UTC (permalink / raw)
To: Alexander Duyck
Cc: David E. Box, Lee Jones, Darren Hart, Andy Shevchenko,
Bjorn Helgaas, Alexander Duyck, Hans de Goede, Alexey Budankov,
Linux Kernel Mailing List, Platform Driver, linux-pci
On Thu, Oct 1, 2020 at 8:44 PM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Thu, Oct 1, 2020 at 9:26 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Thu, Oct 1, 2020 at 4:43 AM David E. Box <david.e.box@linux.intel.com> wrote:
...
> > > Intel Platform Monitoring Technology is meant to provide a common way to
> > > access telemetry and system metrics.
> > >
> > > Register mappings are not provided by the driver. Instead, a GUID is read
> > > from a header for each endpoint. The GUID identifies the device and is to
> > > be used with an XML, provided by the vendor, to discover the available set
> > > of metrics and their register mapping. This allows firmware updates to
> > > modify the register space without needing to update the driver every time
> > > with new mappings. Firmware writes a new GUID in this case to specify the
> > > new mapping. Software tools with access to the associated XML file can
> > > then interpret the changes.
> >
> > Where one may find a database of these reserved GUIDs / XMLs?
> > How do you prevent a chaos which happens with other registries?
>
> The database will be posted on intel.com eventually. Although I don't
> believe the URL is public yet.
How can we be sure that this won't be forgotten? How can we be sure it
will be public at the end? Please, elaborate this in the commit
message.
...
> > > +static const struct pci_device_id pmt_telem_early_client_pci_ids[] = {
> > > + { PCI_VDEVICE(INTEL, 0x9a0d) }, /* TGL */
> > > + { }
> > > +};
> > > +bool intel_pmt_is_early_client_hw(struct device *dev)
> > > +{
> > > + struct pci_dev *parent = to_pci_dev(dev->parent);
> > > +
> > > + return !!pci_match_id(pmt_telem_early_client_pci_ids, parent);
> > > +}
> > > +EXPORT_SYMBOL_GPL(intel_pmt_is_early_client_hw);
> >
> > What is this and why is it in the class driver?
>
> I chose to use the class driver as a central place to store code
> common to all of the instances of the class. In this case we have
> quirks that are specific to Tiger Lake and so I chose to store the
> function to test for the device here.
Can it be done in another file module at least (let's say intel_pmt_quirks.c)?
...
> > > + if (off < 0)
> > > + return -EINVAL;
> > Is this real or theoretical?
>
> Not sure. I am not that familiar with the interface. It was something
> I copied from read_bmof which is what I based this code on based on an
> earlier suggestion.
I'm not a fan of cargo cult when there is no understanding why certain
code appears in the driver.
...
> > > + if (count)
> >
> > Useless.
>
> I'm assuming that is because memcpy_fromio is assumed to handle this case?
Right.
> > > + memcpy_fromio(buf, entry->base + off, count);
...
> > > + psize = (PFN_UP(entry->base_addr + entry->size) - pfn) * PAGE_SIZE;
> >
> > PFN_PHYS(PFN_UP(...)) ?
>
> I'm not sure how that would work. Basically what we are doing here is
> determining the size of the mapping based on the number of pages that
> will be needed. So we wake the pfn of the start of the region,
> subtract that from the pfn for the end of the region and multiply by
> the size of a page.
PFN_PHYS() is a replacement for multiplication. You may check its
implementation.
...
> > > + /* if size is 0 assume no data buffer, so no file needed */
> > > + if (!entry->size)
> > > + return 0;
> >
> > Hmm... But presence of the file is also an information that might be
> > useful for user, no?
>
> I'm not sure what you mean? If the size of the region is zero it means
> there is no data there. There are cases in future devices where we may
> have controls for a telemetry function that is actually streaming the
> data elsewhere. That will be the use case for the entry size of 0 and
> in that case it doesn't make any sense to have a file as there is no
> data to present in it.
This is not understandable from 'no data buffer' above. Can you
elaborate in the comment?
...
> > > + entry->base = devm_ioremap_resource(dev, &res);
> > > + if (IS_ERR(entry->base)) {
> > > + dev_err(dev, "Failed to ioremap device region\n");
> > > + goto fail_ioremap;
> > > + }
> >
> > > + iounmap(entry->base);
> >
> > This is interesting. How do you avoid double unmap with (1)?
>
> I think I get what you are trying to say. This is redundant since we
> used the devm_ioremap_resource it will already be freed when the
> driver is detached, correct?
Right. Above is the leftover that shows the poor testing of v7.
> > > +#include <linux/platform_device.h>
> > > +#include <linux/xarray.h>
> > > +
> > > +/* PMT access types */
> > > +#define ACCESS_BARID 2
> > > +#define ACCESS_LOCAL 3
> > > +
> > > +/* PMT discovery base address/offset register layout */
> > > +#define GET_BIR(v) ((v) & GENMASK(2, 0))
> > > +#define GET_ADDRESS(v) ((v) & GENMASK(31, 3))
> >
> > bits.h
>
> That is already included from a few different sources.
Please, read again what I wrote below. Ditto for other inclusions.
> > > +struct intel_pmt_entry {
> > > + struct bin_attribute pmt_bin_attr;
> > > + struct kobject *kobj;
> > > + void __iomem *disc_tabl
> > > + void __iomem *base;
> > > + unsigned long base_addr;
> > > + size_t size;
> >
> > > + u32 guid;
> >
> > types.h
>
> Is included through xarray.h.
>
> > > + int devid;
> > > +};
> >
> > > +static inline int
> > > +intel_pmt_ioremap_discovery_table(struct intel_pmt_entry *entry,
> > > + struct platform_device *pdev, int i)
> > > +{
> >
> > > + entry->disc_table = devm_platform_ioremap_resource(pdev, i);
> >
> > io.h ?
>
> That one I will move from the class.c file.
>
> > > +
> > > + return PTR_ERR_OR_ZERO(entry->disc_table);
> >
> > err.h
>
> That is included as a part of io.h.
>
> > > +}
> >
> > The rule of thumb is to include all headers that you have direct users of.
> > Then you may optimize by removing those which are guaranteed to be
> > included by others, like bits.h always included by bitops.h.
>
> Yeah, from what I can tell the only one I didn't have was io.h and
> part of that is because this was something I had moved to the header
> file in order to commonize it since it was being used in the other
> drivers.
types.h not necessarily be included by above and so on...
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V7 3/5] platform/x86: Intel PMT class driver
2020-10-01 18:06 ` Andy Shevchenko
@ 2020-10-01 19:02 ` Alexander Duyck
0 siblings, 0 replies; 17+ messages in thread
From: Alexander Duyck @ 2020-10-01 19:02 UTC (permalink / raw)
To: Andy Shevchenko
Cc: David E. Box, Lee Jones, Darren Hart, Andy Shevchenko,
Bjorn Helgaas, Alexander Duyck, Hans de Goede, Alexey Budankov,
Linux Kernel Mailing List, Platform Driver, linux-pci
On Thu, Oct 1, 2020 at 11:06 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Oct 1, 2020 at 8:44 PM Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
> > On Thu, Oct 1, 2020 at 9:26 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Thu, Oct 1, 2020 at 4:43 AM David E. Box <david.e.box@linux.intel.com> wrote:
>
> ...
>
> > > > Intel Platform Monitoring Technology is meant to provide a common way to
> > > > access telemetry and system metrics.
> > > >
> > > > Register mappings are not provided by the driver. Instead, a GUID is read
> > > > from a header for each endpoint. The GUID identifies the device and is to
> > > > be used with an XML, provided by the vendor, to discover the available set
> > > > of metrics and their register mapping. This allows firmware updates to
> > > > modify the register space without needing to update the driver every time
> > > > with new mappings. Firmware writes a new GUID in this case to specify the
> > > > new mapping. Software tools with access to the associated XML file can
> > > > then interpret the changes.
> > >
> > > Where one may find a database of these reserved GUIDs / XMLs?
> > > How do you prevent a chaos which happens with other registries?
> >
> > The database will be posted on intel.com eventually. Although I don't
> > believe the URL is public yet.
>
> How can we be sure that this won't be forgotten? How can we be sure it
> will be public at the end? Please, elaborate this in the commit
> message.
Okay, I will work with David on that.
> ...
>
> > > > +static const struct pci_device_id pmt_telem_early_client_pci_ids[] = {
> > > > + { PCI_VDEVICE(INTEL, 0x9a0d) }, /* TGL */
> > > > + { }
> > > > +};
> > > > +bool intel_pmt_is_early_client_hw(struct device *dev)
> > > > +{
> > > > + struct pci_dev *parent = to_pci_dev(dev->parent);
> > > > +
> > > > + return !!pci_match_id(pmt_telem_early_client_pci_ids, parent);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(intel_pmt_is_early_client_hw);
> > >
> > > What is this and why is it in the class driver?
> >
> > I chose to use the class driver as a central place to store code
> > common to all of the instances of the class. In this case we have
> > quirks that are specific to Tiger Lake and so I chose to store the
> > function to test for the device here.
>
> Can it be done in another file module at least (let's say intel_pmt_quirks.c)?
I suppose, but then it is adding a file for essentially 13 lines of
code. Maybe I will just move this back to intel_pmt_telemetry.c and we
can revisit where this should go if/when we add the watcher driver.
> ...
>
> > > > + if (off < 0)
> > > > + return -EINVAL;
> > > Is this real or theoretical?
> >
> > Not sure. I am not that familiar with the interface. It was something
> > I copied from read_bmof which is what I based this code on based on an
> > earlier suggestion.
>
> I'm not a fan of cargo cult when there is no understanding why certain
> code appears in the driver.
Well with something like this I usually question if it provides any
value. The problem is I don't know enough about binary sysfs
attributes to say one way or another. If you know that the offset
provided cannot be negative I can drop it. However I haven't seen
anything that seems to say one way or another.
> ...
>
> > > > + if (count)
> > >
> > > Useless.
> >
> > I'm assuming that is because memcpy_fromio is assumed to handle this case?
>
> Right.
>
> > > > + memcpy_fromio(buf, entry->base + off, count);
>
> ...
>
> > > > + psize = (PFN_UP(entry->base_addr + entry->size) - pfn) * PAGE_SIZE;
> > >
> > > PFN_PHYS(PFN_UP(...)) ?
> >
> > I'm not sure how that would work. Basically what we are doing here is
> > determining the size of the mapping based on the number of pages that
> > will be needed. So we wake the pfn of the start of the region,
> > subtract that from the pfn for the end of the region and multiply by
> > the size of a page.
>
> PFN_PHYS() is a replacement for multiplication. You may check its
> implementation.
Ah, okay so you meant PFN_PHYS(PFN_UP(...)-pfn)
> ...
>
> > > > + /* if size is 0 assume no data buffer, so no file needed */
> > > > + if (!entry->size)
> > > > + return 0;
> > >
> > > Hmm... But presence of the file is also an information that might be
> > > useful for user, no?
> >
> > I'm not sure what you mean? If the size of the region is zero it means
> > there is no data there. There are cases in future devices where we may
> > have controls for a telemetry function that is actually streaming the
> > data elsewhere. That will be the use case for the entry size of 0 and
> > in that case it doesn't make any sense to have a file as there is no
> > data to present in it.
>
> This is not understandable from 'no data buffer' above. Can you
> elaborate in the comment?
Having a file here implies there is something to read. There are
devices supported by PMT which are essentially just control only,
specifically some of the watchers. So the file would have nothing to
read/mmap. In such a case it wouldn't make sense to provide a binary
sysfs attribute as there is no data to associate with it.
> ...
>
> > > > + entry->base = devm_ioremap_resource(dev, &res);
> > > > + if (IS_ERR(entry->base)) {
> > > > + dev_err(dev, "Failed to ioremap device region\n");
> > > > + goto fail_ioremap;
> > > > + }
> > >
> > > > + iounmap(entry->base);
> > >
> > > This is interesting. How do you avoid double unmap with (1)?
> >
> > I think I get what you are trying to say. This is redundant since we
> > used the devm_ioremap_resource it will already be freed when the
> > driver is detached, correct?
>
> Right. Above is the leftover that shows the poor testing of v7.
I will admit I don't think we tested the case where
sysfs_create_bin_file failed.
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/xarray.h>
> > > > +
> > > > +/* PMT access types */
> > > > +#define ACCESS_BARID 2
> > > > +#define ACCESS_LOCAL 3
> > > > +
> > > > +/* PMT discovery base address/offset register layout */
> > > > +#define GET_BIR(v) ((v) & GENMASK(2, 0))
> > > > +#define GET_ADDRESS(v) ((v) & GENMASK(31, 3))
> > >
> > > bits.h
> >
> > That is already included from a few different sources.
>
> Please, read again what I wrote below. Ditto for other inclusions.
>
> > > > +struct intel_pmt_entry {
>
> > > > + struct bin_attribute pmt_bin_attr;
> > > > + struct kobject *kobj;
> > > > + void __iomem *disc_tabl
> > > > + void __iomem *base;
> > > > + unsigned long base_addr;
> > > > + size_t size;
> > >
> > > > + u32 guid;
> > >
> > > types.h
> >
> > Is included through xarray.h.
> >
> > > > + int devid;
> > > > +};
> > >
> > > > +static inline int
> > > > +intel_pmt_ioremap_discovery_table(struct intel_pmt_entry *entry,
> > > > + struct platform_device *pdev, int i)
> > > > +{
> > >
> > > > + entry->disc_table = devm_platform_ioremap_resource(pdev, i);
> > >
> > > io.h ?
> >
> > That one I will move from the class.c file.
> >
> > > > +
> > > > + return PTR_ERR_OR_ZERO(entry->disc_table);
> > >
> > > err.h
> >
> > That is included as a part of io.h.
> >
> > > > +}
> > >
> > > The rule of thumb is to include all headers that you have direct users of.
> > > Then you may optimize by removing those which are guaranteed to be
> > > included by others, like bits.h always included by bitops.h.
> >
> > Yeah, from what I can tell the only one I didn't have was io.h and
> > part of that is because this was something I had moved to the header
> > file in order to commonize it since it was being used in the other
> > drivers.
>
> types.h not necessarily be included by above and so on...
Okay, I will just explicitly add those headers then.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH V7 4/5] platform/x86: Intel PMT Telemetry capability driver
2020-10-01 1:42 [PATCH V7 0/5] Intel Platform Monitoring Technology David E. Box
` (2 preceding siblings ...)
2020-10-01 1:42 ` [PATCH V7 3/5] platform/x86: Intel PMT class driver David E. Box
@ 2020-10-01 1:42 ` David E. Box
2020-10-01 16:00 ` Andy Shevchenko
2020-10-01 1:42 ` [PATCH V7 5/5] platform/x86: Intel PMT Crashlog " David E. Box
4 siblings, 1 reply; 17+ messages in thread
From: David E. Box @ 2020-10-01 1:42 UTC (permalink / raw)
To: lee.jones, david.e.box, dvhart, andy, bhelgaas,
alexander.h.duyck, hdegoede, alexey.budankov
Cc: linux-kernel, platform-driver-x86, linux-pci
From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
PMT Telemetry is a capability of the Intel Platform Monitoring Technology.
The Telemetry capability provides access to device telemetry metrics that
provide hardware performance data to users from read-only register spaces.
With this driver present the intel_pmt directory can be populated with
telem<x> devices. These devices will contain the standard intel_pmt sysfs
data and a "telem" binary sysfs attribute which can be used to access the
telemetry data.
Co-developed-by: David E. Box <david.e.box@linux.intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
drivers/platform/x86/Kconfig | 8 ++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/intel_pmt_telemetry.c | 158 +++++++++++++++++++++
3 files changed, 167 insertions(+)
create mode 100644 drivers/platform/x86/intel_pmt_telemetry.c
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 82465d0e8fd3..8eae17a57a5b 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1369,6 +1369,14 @@ config INTEL_PMT_CLASS
For more information, see:
<file:Documentation/ABI/testing/sysfs-class-intel_pmt>
+config INTEL_PMT_TELEMETRY
+ tristate "Intel Platform Monitoring Technology (PMT) Telemetry driver"
+ select INTEL_PMT_CLASS
+ help
+ The Intel Platform Monitory Technology (PMT) Telemetry driver provides
+ access to hardware telemetry metrics on devices that support the
+ feature.
+
config INTEL_PUNIT_IPC
tristate "Intel P-Unit IPC Driver"
help
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index f4b1f87f2401..6a7b61f59ea8 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -141,6 +141,7 @@ 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_CLASS) += intel_pmt_class.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
obj-$(CONFIG_INTEL_SCU_PCI) += intel_scu_pcidrv.o
diff --git a/drivers/platform/x86/intel_pmt_telemetry.c b/drivers/platform/x86/intel_pmt_telemetry.c
new file mode 100644
index 000000000000..d4819aefdd65
--- /dev/null
+++ b/drivers/platform/x86/intel_pmt_telemetry.c
@@ -0,0 +1,158 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Platform Monitory Technology Telemetry driver
+ *
+ * Copyright (c) 2020, Intel Corporation.
+ * All Rights Reserved.
+ *
+ * Author: "David E. Box" <david.e.box@linux.intel.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#include "intel_pmt_class.h"
+
+#define TELEM_DEV_NAME "pmt_telemetry"
+
+#define TELEM_SIZE_OFFSET 0x0
+#define TELEM_GUID_OFFSET 0x4
+#define TELEM_BASE_OFFSET 0x8
+#define TELEM_ACCESS(v) ((v) & GENMASK(3, 0))
+/* size is in bytes */
+#define TELEM_SIZE(v) (((v) & GENMASK(27, 12)) >> 10)
+
+/* Used by client hardware to identify a fixed telemetry entry*/
+#define TELEM_CLIENT_FIXED_BLOCK_GUID 0x10000000
+
+struct pmt_telem_priv {
+ int num_entries;
+ struct intel_pmt_entry entry[];
+};
+
+static DEFINE_XARRAY_ALLOC(telem_array);
+static struct intel_pmt_namespace pmt_telem_ns = {
+ .name = "telem",
+ .xa = &telem_array
+};
+
+/*
+ * driver initialization
+ */
+static int pmt_telem_add_entry(struct intel_pmt_entry *entry,
+ struct device *parent,
+ struct resource *disc_res)
+{
+ void __iomem *disc_table = entry->disc_table;
+ struct intel_pmt_header header;
+ int ret;
+
+ header.access_type = TELEM_ACCESS(readl(disc_table));
+ header.guid = readl(disc_table + TELEM_GUID_OFFSET);
+ header.base_offset = readl(disc_table + TELEM_BASE_OFFSET);
+
+ /* Size is measured in DWORDS, but accessor returns bytes */
+ header.size = TELEM_SIZE(readl(disc_table));
+
+ ret = intel_pmt_populate_entry(entry, &header, parent, disc_res);
+ if (ret)
+ return ret;
+
+ return intel_pmt_dev_create(entry, &pmt_telem_ns, parent);
+}
+
+static bool pmt_telem_region_overlaps(struct intel_pmt_entry *entry)
+{
+ u32 guid;
+
+ guid = readl(entry->disc_table + TELEM_GUID_OFFSET);
+
+ return guid == TELEM_CLIENT_FIXED_BLOCK_GUID;
+}
+
+static int pmt_telem_remove(struct platform_device *pdev)
+{
+ struct pmt_telem_priv *priv = platform_get_drvdata(pdev);
+ int i;
+
+ for (i = 0; i < priv->num_entries; i++)
+ intel_pmt_dev_destroy(&priv->entry[i], &pmt_telem_ns);
+
+ return 0;
+}
+
+static int pmt_telem_probe(struct platform_device *pdev)
+{
+ struct pmt_telem_priv *priv;
+ bool early_hw;
+ size_t size;
+ int i, ret;
+
+ size = offsetof(struct pmt_telem_priv, entry[pdev->num_resources]);
+ priv = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, priv);
+
+ if (intel_pmt_is_early_client_hw(&pdev->dev))
+ early_hw = true;
+
+ for (i = 0; i < pdev->num_resources; i++) {
+ struct intel_pmt_entry *entry = &priv->entry[i];
+ struct resource *disc_res;
+
+ ret = -ENODEV;
+
+ disc_res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+ if (!disc_res)
+ goto abort_probe;
+
+ ret = intel_pmt_ioremap_discovery_table(entry, pdev, i);
+ if (ret)
+ goto abort_probe;
+
+ if (pmt_telem_region_overlaps(entry) && early_hw)
+ continue;
+
+ ret = pmt_telem_add_entry(entry, &pdev->dev, disc_res);
+ if (ret)
+ goto abort_probe;
+
+ priv->num_entries++;
+ }
+
+ return 0;
+abort_probe:
+ pmt_telem_remove(pdev);
+ return ret;
+}
+
+static struct platform_driver pmt_telem_driver = {
+ .driver = {
+ .name = TELEM_DEV_NAME,
+ },
+ .remove = pmt_telem_remove,
+ .probe = pmt_telem_probe,
+};
+
+static int __init pmt_telem_init(void)
+{
+ return platform_driver_register(&pmt_telem_driver);
+}
+module_init(pmt_telem_init);
+
+static void __exit pmt_telem_exit(void)
+{
+ platform_driver_unregister(&pmt_telem_driver);
+ xa_destroy(&telem_array);
+}
+module_exit(pmt_telem_exit);
+
+MODULE_AUTHOR("David E. Box <david.e.box@linux.intel.com>");
+MODULE_DESCRIPTION("Intel PMT Telemetry driver");
+MODULE_ALIAS("platform:" TELEM_DEV_NAME);
+MODULE_LICENSE("GPL v2");
--
2.20.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH V7 4/5] platform/x86: Intel PMT Telemetry capability driver
2020-10-01 1:42 ` [PATCH V7 4/5] platform/x86: Intel PMT Telemetry capability driver David E. Box
@ 2020-10-01 16:00 ` Andy Shevchenko
2020-10-01 16:46 ` Alexander Duyck
0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2020-10-01 16:00 UTC (permalink / raw)
To: David E. Box
Cc: Lee Jones, Darren Hart, Andy Shevchenko, Bjorn Helgaas,
Alexander Duyck, Hans de Goede, alexey.budankov,
Linux Kernel Mailing List, Platform Driver, linux-pci
On Thu, Oct 1, 2020 at 4:43 AM David E. Box <david.e.box@linux.intel.com> wrote:
>
> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>
> PMT Telemetry is a capability of the Intel Platform Monitoring Technology.
> The Telemetry capability provides access to device telemetry metrics that
> provide hardware performance data to users from read-only register spaces.
>
> With this driver present the intel_pmt directory can be populated with
> telem<x> devices. These devices will contain the standard intel_pmt sysfs
> data and a "telem" binary sysfs attribute which can be used to access the
> telemetry data.
...
> +static DEFINE_XARRAY_ALLOC(telem_array);
> +static struct intel_pmt_namespace pmt_telem_ns = {
> + .name = "telem",
> + .xa = &telem_array
Leave comma at the end.
> +};
> +
> +/*
> + * driver initialization
> + */
This is a useless comment.
> + size = offsetof(struct pmt_telem_priv, entry[pdev->num_resources]);
> + priv = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
Please, use struct_size() from overflow.h instead of custom approach.
...
> +static struct platform_driver pmt_telem_driver = {
> + .driver = {
> + .name = TELEM_DEV_NAME,
I'm not sure I have interpreted this:
- Use 'raw' string instead of defines for device names
correctly. Can you elaborate?
> + },
> + .remove = pmt_telem_remove,
> + .probe = pmt_telem_probe,
> +};
...
> +MODULE_ALIAS("platform:" TELEM_DEV_NAME);
Ditto.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V7 4/5] platform/x86: Intel PMT Telemetry capability driver
2020-10-01 16:00 ` Andy Shevchenko
@ 2020-10-01 16:46 ` Alexander Duyck
2020-10-01 17:54 ` Andy Shevchenko
0 siblings, 1 reply; 17+ messages in thread
From: Alexander Duyck @ 2020-10-01 16:46 UTC (permalink / raw)
To: Andy Shevchenko
Cc: David E. Box, Lee Jones, Darren Hart, Andy Shevchenko,
Bjorn Helgaas, Alexander Duyck, Hans de Goede, Alexey Budankov,
Linux Kernel Mailing List, Platform Driver, linux-pci
On Thu, Oct 1, 2020 at 9:03 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Oct 1, 2020 at 4:43 AM David E. Box <david.e.box@linux.intel.com> wrote:
> >
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > PMT Telemetry is a capability of the Intel Platform Monitoring Technology.
> > The Telemetry capability provides access to device telemetry metrics that
> > provide hardware performance data to users from read-only register spaces.
> >
> > With this driver present the intel_pmt directory can be populated with
> > telem<x> devices. These devices will contain the standard intel_pmt sysfs
> > data and a "telem" binary sysfs attribute which can be used to access the
> > telemetry data.
>
> ...
>
> > +static DEFINE_XARRAY_ALLOC(telem_array);
> > +static struct intel_pmt_namespace pmt_telem_ns = {
> > + .name = "telem",
> > + .xa = &telem_array
>
> Leave comma at the end.
>
> > +};
> > +
> > +/*
> > + * driver initialization
> > + */
>
> This is a useless comment.
>
> > + size = offsetof(struct pmt_telem_priv, entry[pdev->num_resources]);
> > + priv = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
>
> Please, use struct_size() from overflow.h instead of custom approach.
>
> ...
So all of the above make sense and can be fixed shortly and pushed as
a v8 for both the telemetry and crashlog drivers.
> > +static struct platform_driver pmt_telem_driver = {
> > + .driver = {
> > + .name = TELEM_DEV_NAME,
>
> I'm not sure I have interpreted this:
> - Use 'raw' string instead of defines for device names
> correctly. Can you elaborate?
Can you point me to a reference of that? I'm not sure what you are referring to.
> > + },
> > + .remove = pmt_telem_remove,
> > + .probe = pmt_telem_probe,
> > +};
>
> ...
>
> > +MODULE_ALIAS("platform:" TELEM_DEV_NAME);
>
> Ditto.
This doesn't make sense to me. Are you saying we are expected to use
"pmt_telemetry" everywhere instead of the define? It seems like that
would be much more error prone. It seems like common practice to use
DRV_NAME throughout a driver for these sort of things so if you are
wanting us to rename it to that I am fine with that, but I am not sure
getting rid of the use of a define makes sense.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V7 4/5] platform/x86: Intel PMT Telemetry capability driver
2020-10-01 16:46 ` Alexander Duyck
@ 2020-10-01 17:54 ` Andy Shevchenko
0 siblings, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2020-10-01 17:54 UTC (permalink / raw)
To: Alexander Duyck
Cc: David E. Box, Lee Jones, Darren Hart, Andy Shevchenko,
Bjorn Helgaas, Alexander Duyck, Hans de Goede, Alexey Budankov,
Linux Kernel Mailing List, Platform Driver, linux-pci
On Thu, Oct 1, 2020 at 7:46 PM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Thu, Oct 1, 2020 at 9:03 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > > +static struct platform_driver pmt_telem_driver = {
> > > + .driver = {
> > > + .name = TELEM_DEV_NAME,
> >
> > I'm not sure I have interpreted this:
> > - Use 'raw' string instead of defines for device names
> > correctly. Can you elaborate?
>
> Can you point me to a reference of that? I'm not sure what you are referring to.
It's a changelog of this very series.
> > > + },
> > > + .remove = pmt_telem_remove,
> > > + .probe = pmt_telem_probe,
> > > +};
> >
> > ...
> >
> > > +MODULE_ALIAS("platform:" TELEM_DEV_NAME);
> >
> > Ditto.
>
> This doesn't make sense to me. Are you saying we are expected to use
> "pmt_telemetry" everywhere instead of the define? It seems like that
> would be much more error prone. It seems like common practice to use
> DRV_NAME throughout a driver for these sort of things so if you are
> wanting us to rename it to that I am fine with that, but I am not sure
> getting rid of the use of a define makes sense.
I'm just wondering why changelog mentioned that and what it meant.
If it's indeed conversion to explicit naming, then this has to be
followed (somebody seems asked for that, right?) or commented why not.
Or maybe I understood it wrong?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH V7 5/5] platform/x86: Intel PMT Crashlog capability driver
2020-10-01 1:42 [PATCH V7 0/5] Intel Platform Monitoring Technology David E. Box
` (3 preceding siblings ...)
2020-10-01 1:42 ` [PATCH V7 4/5] platform/x86: Intel PMT Telemetry capability driver David E. Box
@ 2020-10-01 1:42 ` David E. Box
2020-10-01 16:35 ` Andy Shevchenko
4 siblings, 1 reply; 17+ messages in thread
From: David E. Box @ 2020-10-01 1:42 UTC (permalink / raw)
To: lee.jones, david.e.box, dvhart, andy, bhelgaas,
alexander.h.duyck, hdegoede, alexey.budankov
Cc: linux-kernel, platform-driver-x86, linux-pci
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>
---
.../ABI/testing/sysfs-class-intel_pmt | 65 ++++
drivers/platform/x86/Kconfig | 8 +
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/intel_pmt_crashlog.c | 339 ++++++++++++++++++
4 files changed, 413 insertions(+)
create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c
diff --git a/Documentation/ABI/testing/sysfs-class-intel_pmt b/Documentation/ABI/testing/sysfs-class-intel_pmt
index 926b5cf95fd1..67ca47123cbf 100644
--- a/Documentation/ABI/testing/sysfs-class-intel_pmt
+++ b/Documentation/ABI/testing/sysfs-class-intel_pmt
@@ -52,3 +52,68 @@ Contact: David Box <david.e.box@linux.intel.com>
Description:
(RO) The offset of telemetry region in bytes that corresponds to
the mapping for the telem file.
+
+What: /sys/class/intel_pmt/crashlog<x>
+Date: October 2020
+KernelVersion: 5.10
+Contact: Alexander Duyck <alexander.h.duyck@linux.intel.com>
+Description:
+ The crashlog<x> directory contains files for configuring an
+ instance of a PMT crashlog device that can perform crash data
+ recoring. Each crashlog<x> device has an associated crashlog
+ file. This file can be opened and mapped or read to access the
+ resulting crashlog buffer. The register layout for the buffer
+ can be determined from an XML file of specified guid for the
+ parent device.
+
+What: /sys/class/intel_pmt/crashlog<x>/crashlog
+Date: October 2020
+KernelVersion: 5.10
+Contact: David Box <david.e.box@linux.intel.com>
+Description:
+ (RO) The crashlog buffer for this crashlog device. This file
+ may be mapped or read to obtain the data.
+
+What: /sys/class/intel_pmt/crashlog<x>/guid
+Date: October 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/intel_pmt/crashlog<x>/size
+Date: October 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 size for the crashlog buffer.
+
+What: /sys/class/intel_pmt/crashlog<x>/offset
+Date: October 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 crashlog device.
+
+What: /sys/class/intel_pmt/crashlog<x>/enable
+Date: October 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 crashlog device.
+
+What: /sys/class/intel_pmt/crashlog<x>/trigger
+Date: October 2020
+KernelVersion: 5.10
+Contact: Alexander Duyck <alexander.h.duyck@linux.intel.com>
+Description:
+ (RW) Boolean value controlling the triggering of the crashlog
+ 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 8eae17a57a5b..529c5ee2eabf 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1377,6 +1377,14 @@ config INTEL_PMT_TELEMETRY
access to hardware telemetry metrics on devices that support the
feature.
+config INTEL_PMT_CRASHLOG
+ tristate "Intel Platform Monitoring Technology (PMT) Crashlog driver"
+ select INTEL_PMT_CLASS
+ help
+ The Intel Platform Monitoring Technology (PMT) crashlog driver provides
+ access to hardware crashlog capabilities on devices that support the
+ feature.
+
config INTEL_PUNIT_IPC
tristate "Intel P-Unit IPC Driver"
help
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 6a7b61f59ea8..ca82c1344977 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -142,6 +142,7 @@ 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_CLASS) += intel_pmt_class.o
obj-$(CONFIG_INTEL_PMT_TELEMETRY) += intel_pmt_telemetry.o
+obj-$(CONFIG_INTEL_PMT_CRASHLOG) += intel_pmt_crashlog.o
obj-$(CONFIG_INTEL_PUNIT_IPC) += intel_punit_ipc.o
obj-$(CONFIG_INTEL_SCU_IPC) += intel_scu_ipc.o
obj-$(CONFIG_INTEL_SCU_PCI) += intel_scu_pcidrv.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..9c77be39ac16
--- /dev/null
+++ b/drivers/platform/x86/intel_pmt_crashlog.c
@@ -0,0 +1,339 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Platform Monitoring Technology Crashlog driver
+ *
+ * Copyright (c) 2020, Intel Corporation.
+ * All Rights Reserved.
+ *
+ * Author: "Alexander Duyck" <alexander.h.duyck@linux.intel.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#include "intel_pmt_class.h"
+
+#define DRV_NAME "pmt_crashlog"
+
+/* 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)
+
+/* Crashlog Discovery 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)
+/* size is in bytes */
+#define GET_SIZE(v) ((v) * sizeof(u32))
+
+struct crashlog_entry {
+ /* entry must be first member of struct */
+ struct intel_pmt_entry entry;
+ struct mutex control_mutex;
+};
+
+struct pmt_crashlog_priv {
+ int num_entries;
+ struct crashlog_entry entry[];
+};
+
+/*
+ * I/O
+ */
+static bool pmt_crashlog_complete(struct intel_pmt_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 intel_pmt_entry *entry)
+{
+ u32 control = readl(entry->disc_table + CONTROL_OFFSET);
+
+ /* return current value of the crashlog disabled flag */
+ return !!(control & CRASHLOG_FLAG_DISABLE);
+}
+
+static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
+{
+ u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
+ u32 crash_type, version;
+
+ crash_type = GET_TYPE(discovery_header);
+ version = GET_VERSION(discovery_header);
+
+ /*
+ * Currenty we only recognize OOBMSM version 0 devices.
+ * We can ignore all other crashlog devices in the system.
+ */
+ return crash_type == CRASH_TYPE_OOBMSM && version == 0;
+}
+
+static void pmt_crashlog_set_disable(struct intel_pmt_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 intel_pmt_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 intel_pmt_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);
+}
+
+/*
+ * sysfs
+ */
+static ssize_t
+enable_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct intel_pmt_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;
+
+ mutex_lock(&entry->control_mutex);
+ pmt_crashlog_set_disable(&entry->entry, !enabled);
+ mutex_unlock(&entry->control_mutex);
+
+ return strnlen(buf, count);
+}
+static DEVICE_ATTR_RW(enable);
+
+static ssize_t
+trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct intel_pmt_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;
+
+ mutex_lock(&entry->control_mutex);
+
+ if (!trigger) {
+ pmt_crashlog_set_clear(&entry->entry);
+ } else if (pmt_crashlog_complete(&entry->entry)) {
+ /* we cannot trigger a new crash if one is still pending */
+ result = -EEXIST;
+ goto err;
+ } else if (pmt_crashlog_disabled(&entry->entry)) {
+ /* if device is currently disabled, return busy */
+ result = -EBUSY;
+ goto err;
+ } else {
+ pmt_crashlog_set_execute(&entry->entry);
+ }
+
+ result = strnlen(buf, count);
+err:
+ mutex_unlock(&entry->control_mutex);
+ return result;
+}
+static DEVICE_ATTR_RW(trigger);
+
+static struct attribute *pmt_crashlog_attrs[] = {
+ &dev_attr_enable.attr,
+ &dev_attr_trigger.attr,
+ NULL
+};
+
+static struct attribute_group pmt_crashlog_group = {
+ .attrs = pmt_crashlog_attrs,
+};
+
+static DEFINE_XARRAY_ALLOC(crashlog_array);
+static struct intel_pmt_namespace pmt_crashlog_ns = {
+ .name = "crashlog",
+ .xa = &crashlog_array,
+ .attr_grp = &pmt_crashlog_group
+};
+
+/*
+ * initialization
+ */
+static int pmt_crashlog_add_entry(struct intel_pmt_entry *entry,
+ struct device *parent,
+ struct resource *disc_res)
+{
+ void __iomem *disc_table = entry->disc_table;
+ struct intel_pmt_header header;
+ int ret;
+
+ header.access_type = GET_ACCESS(readl(disc_table));
+ header.guid = readl(disc_table + GUID_OFFSET);
+ header.base_offset = readl(disc_table + BASE_OFFSET);
+
+ /* Size is measured in DWORDS, but accessor returns bytes */
+ header.size = GET_SIZE(readl(disc_table + SIZE_OFFSET));
+
+ ret = intel_pmt_populate_entry(entry, &header, parent, disc_res);
+ if (ret)
+ return ret;
+
+ ret = intel_pmt_dev_create(entry, &pmt_crashlog_ns, parent);
+ if (!ret)
+ return 0;
+
+ dev_err(parent, "Failed to add crashlog controls\n");
+ intel_pmt_dev_destroy(entry, &pmt_crashlog_ns);
+
+ return ret;
+}
+
+static int pmt_crashlog_remove(struct platform_device *pdev)
+{
+ struct pmt_crashlog_priv *priv = platform_get_drvdata(pdev);
+ int i;
+
+ for (i = 0; i < priv->num_entries; i++)
+ intel_pmt_dev_destroy(&priv->entry[i].entry, &pmt_crashlog_ns);
+
+ return 0;
+}
+
+static int pmt_crashlog_probe(struct platform_device *pdev)
+{
+ struct pmt_crashlog_priv *priv;
+ size_t size;
+ int i, ret;
+
+ size = offsetof(struct pmt_crashlog_priv, entry[pdev->num_resources]);
+ priv = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, priv);
+
+ for (i = 0; i < pdev->num_resources; i++) {
+ struct intel_pmt_entry *entry = &priv->entry[i].entry;
+ struct resource *disc_res;
+
+ ret = -ENODEV;
+
+ /* initialize control mutex */
+ mutex_init(&priv->entry[i].control_mutex);
+
+ disc_res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+ if (!disc_res)
+ goto abort_probe;
+
+ ret = intel_pmt_ioremap_discovery_table(entry, pdev, i);
+ if (ret)
+ goto abort_probe;
+
+ if (!pmt_crashlog_supported(entry))
+ continue;
+
+ ret = pmt_crashlog_add_entry(entry, &pdev->dev, disc_res);
+ if (ret)
+ goto abort_probe;
+
+ priv->num_entries++;
+ }
+
+ return 0;
+abort_probe:
+ pmt_crashlog_remove(pdev);
+ return ret;
+}
+
+static struct platform_driver pmt_crashlog_driver = {
+ .driver = {
+ .name = DRV_NAME,
+ },
+ .remove = pmt_crashlog_remove,
+ .probe = pmt_crashlog_probe,
+};
+
+static int __init pmt_crashlog_init(void)
+{
+ return platform_driver_register(&pmt_crashlog_driver);
+}
+
+static void __exit pmt_crashlog_exit(void)
+{
+ platform_driver_unregister(&pmt_crashlog_driver);
+ xa_destroy(&crashlog_array);
+}
+
+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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH V7 5/5] platform/x86: Intel PMT Crashlog capability driver
2020-10-01 1:42 ` [PATCH V7 5/5] platform/x86: Intel PMT Crashlog " David E. Box
@ 2020-10-01 16:35 ` Andy Shevchenko
2020-10-01 18:33 ` Alexander Duyck
0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2020-10-01 16:35 UTC (permalink / raw)
To: David E. Box
Cc: Lee Jones, Darren Hart, Andy Shevchenko, Bjorn Helgaas,
Alexander Duyck, Hans de Goede, alexey.budankov,
Linux Kernel Mailing List, Platform Driver, linux-pci
On Thu, Oct 1, 2020 at 4:43 AM David E. Box <david.e.box@linux.intel.com> wrote:
> 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.
...
> + The crashlog<x> directory contains files for configuring an
> + instance of a PMT crashlog device that can perform crash data
> + recoring. Each crashlog<x> device has an associated crashlog
recording
> + file. This file can be opened and mapped or read to access the
> + resulting crashlog buffer. The register layout for the buffer
> + can be determined from an XML file of specified guid for the
> + parent device.
...
> + (RO) The guid for this crashlog device. The guid identifies the
guid -> GUID
Please, spell check all ABI files in this series.
...
> +config INTEL_PMT_CRASHLOG
> + tristate "Intel Platform Monitoring Technology (PMT) Crashlog driver"
> + select INTEL_PMT_CLASS
> + help
> + The Intel Platform Monitoring Technology (PMT) crashlog driver provides
> + access to hardware crashlog capabilities on devices that support the
> + feature.
Name of the module?
...
> + /*
> + * Currenty we only recognize OOBMSM version 0 devices.
Currently. Please spell check all comments in the code.
> + * We can ignore all other crashlog devices in the system.
> + */
...
> + /* clear control bits */
What new information readers get from this comment?
> + control &= ~(CRASHLOG_FLAG_MASK | CRASHLOG_FLAG_DISABLE);
How does the second constant play any role here?
...
> + /* clear control bits */
Ditto. And moreover it's ambiguous due to joined two lines below.
> + control &= ~CRASHLOG_FLAG_MASK;
> + control |= CRASHLOG_FLAG_EXECUTE;
...
> + return strnlen(buf, count);
How is this different to count?
...
> + struct crashlog_entry *entry;
> + bool trigger;
> + int result;
> +
> + entry = dev_get_drvdata(dev);
You may reduce LOCs by direct assigning in the definition block above.
...
> + result = strnlen(buf, count);
How is it different from count?
...
> +static DEFINE_XARRAY_ALLOC(crashlog_array);
> +static struct intel_pmt_namespace pmt_crashlog_ns = {
> + .name = "crashlog",
> + .xa = &crashlog_array,
> + .attr_grp = &pmt_crashlog_group
Leave the comma here.
> +};
...
> + ret = intel_pmt_dev_create(entry, &pmt_crashlog_ns, parent);
> + if (!ret)
> + return 0;
> +
> + dev_err(parent, "Failed to add crashlog controls\n");
> + intel_pmt_dev_destroy(entry, &pmt_crashlog_ns);
> +
> + return ret;
Can we use traditional patterns?
if (ret) {
...
}
return ret;
...
> + size = offsetof(struct pmt_crashlog_priv, entry[pdev->num_resources]);
> + priv = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
struct_size()
...
> + /* initialize control mutex */
> + mutex_init(&priv->entry[i].control_mutex);
> +
> + disc_res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> + if (!disc_res)
> + goto abort_probe;
> +
> + ret = intel_pmt_ioremap_discovery_table(entry, pdev, i);
> + if (ret)
> + goto abort_probe;
> +
> + if (!pmt_crashlog_supported(entry))
> + continue;
> +
> + ret = pmt_crashlog_add_entry(entry, &pdev->dev, disc_res);
> + if (ret)
> + goto abort_probe;
> +
> + priv->num_entries++;
Are you going to duplicate this in each driver? Consider to refactor
to avoid duplication of a lot of code.
...
> + .name = DRV_NAME,
> +MODULE_ALIAS("platform:" DRV_NAME);
I'm not sure I have interpreted this:
- Use 'raw' string instead of defines for device names
correctly. Can you elaborate?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V7 5/5] platform/x86: Intel PMT Crashlog capability driver
2020-10-01 16:35 ` Andy Shevchenko
@ 2020-10-01 18:33 ` Alexander Duyck
2020-10-01 18:46 ` Andy Shevchenko
0 siblings, 1 reply; 17+ messages in thread
From: Alexander Duyck @ 2020-10-01 18:33 UTC (permalink / raw)
To: Andy Shevchenko
Cc: David E. Box, Lee Jones, Darren Hart, Andy Shevchenko,
Bjorn Helgaas, Alexander Duyck, Hans de Goede, Alexey Budankov,
Linux Kernel Mailing List, Platform Driver, linux-pci
On Thu, Oct 1, 2020 at 9:37 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Oct 1, 2020 at 4:43 AM David E. Box <david.e.box@linux.intel.com> wrote:
> > 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.
>
> ...
>
> > + The crashlog<x> directory contains files for configuring an
> > + instance of a PMT crashlog device that can perform crash data
> > + recoring. Each crashlog<x> device has an associated crashlog
>
> recording
>
> > + file. This file can be opened and mapped or read to access the
> > + resulting crashlog buffer. The register layout for the buffer
> > + can be determined from an XML file of specified guid for the
> > + parent device.
>
> ...
>
> > + (RO) The guid for this crashlog device. The guid identifies the
>
> guid -> GUID
>
> Please, spell check all ABI files in this series.
I'll run through it again. I am suspecting I must have deleted the "d"
in recording with a fat fingering when I was editing something else.
> ...
>
> > +config INTEL_PMT_CRASHLOG
> > + tristate "Intel Platform Monitoring Technology (PMT) Crashlog driver"
> > + select INTEL_PMT_CLASS
> > + help
> > + The Intel Platform Monitoring Technology (PMT) crashlog driver provides
> > + access to hardware crashlog capabilities on devices that support the
> > + feature.
>
> Name of the module?
I will add the verbiage:
To compile this driver as a module, choose M here: the module
will be called intel_pmt_crashlog.
> ...
>
> > + /*
>
> > + * Currenty we only recognize OOBMSM version 0 devices.
>
> Currently. Please spell check all comments in the code.
I'll make another pass.
> > + * We can ignore all other crashlog devices in the system.
> > + */
>
> ...
>
> > + /* clear control bits */
>
> What new information readers get from this comment?
Arguably not much. I'll drop the comment.
> > + control &= ~(CRASHLOG_FLAG_MASK | CRASHLOG_FLAG_DISABLE);
>
> How does the second constant play any role here?
The "control" flags are bits 28-31, while the disable flag is bit 27
if I recall.
Specifically bit 31 is read only, bit 28 will clear bit 31, bit 29
will cause the crashlog to be generated and set bit 31, and bit 30 is
just reserved 0.
> ...
>
> > + /* clear control bits */
>
> Ditto. And moreover it's ambiguous due to joined two lines below.
I'll just drop the comments.
> > + control &= ~CRASHLOG_FLAG_MASK;
> > + control |= CRASHLOG_FLAG_EXECUTE;
>
> ...
>
> > + return strnlen(buf, count);
>
> How is this different to count?
I guess they should be equivalent so I can probably just switch to count.
> ...
>
> > + struct crashlog_entry *entry;
> > + bool trigger;
> > + int result;
> > +
>
> > + entry = dev_get_drvdata(dev);
>
> You may reduce LOCs by direct assigning in the definition block above.
Okay. I can move it if you prefer.
> ...
>
> > + result = strnlen(buf, count);
>
> How is it different from count?
I'll switch it.
> ...
>
> > +static DEFINE_XARRAY_ALLOC(crashlog_array);
> > +static struct intel_pmt_namespace pmt_crashlog_ns = {
> > + .name = "crashlog",
> > + .xa = &crashlog_array,
> > + .attr_grp = &pmt_crashlog_group
>
> Leave the comma here.
Already fixed based on similar comments you had for the telemetry driver.. :-)
> > +};
>
> ...
>
> > + ret = intel_pmt_dev_create(entry, &pmt_crashlog_ns, parent);
> > + if (!ret)
> > + return 0;
> > +
> > + dev_err(parent, "Failed to add crashlog controls\n");
> > + intel_pmt_dev_destroy(entry, &pmt_crashlog_ns);
> > +
> > + return ret;
>
> Can we use traditional patterns?
> if (ret) {
> ...
> }
> return ret;
I can switch it if that is preferred.
> ...
>
> > + size = offsetof(struct pmt_crashlog_priv, entry[pdev->num_resources]);
> > + priv = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
>
> struct_size()
>
> ...
>
> > + /* initialize control mutex */
> > + mutex_init(&priv->entry[i].control_mutex);
> > +
> > + disc_res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > + if (!disc_res)
> > + goto abort_probe;
> > +
> > + ret = intel_pmt_ioremap_discovery_table(entry, pdev, i);
> > + if (ret)
> > + goto abort_probe;
> > +
> > + if (!pmt_crashlog_supported(entry))
> > + continue;
> > +
> > + ret = pmt_crashlog_add_entry(entry, &pdev->dev, disc_res);
> > + if (ret)
> > + goto abort_probe;
> > +
> > + priv->num_entries++;
>
> Are you going to duplicate this in each driver? Consider to refactor
> to avoid duplication of a lot of code.
So the issue lies in the complexity of pmt_telem_add_entry versus
pmt_crashlog_add_entry. Specifically I end up needing disc_res and the
discovery table when I go to create the controls for the crashlog
device. Similarly we have a third device that we plan to add called a
watcher which will require us to keep things split up like this so we
thought it best to split it up this way.
> ...
>
> > + .name = DRV_NAME,
>
> > +MODULE_ALIAS("platform:" DRV_NAME);
>
> I'm not sure I have interpreted this:
> - Use 'raw' string instead of defines for device names
> correctly. Can you elaborate?
Again I am not sure what this is in reference to. If you can point me
to some documentation somewhere I can take a look.
Thanks.
- Alex
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V7 5/5] platform/x86: Intel PMT Crashlog capability driver
2020-10-01 18:33 ` Alexander Duyck
@ 2020-10-01 18:46 ` Andy Shevchenko
2020-10-01 19:15 ` Alexander Duyck
0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2020-10-01 18:46 UTC (permalink / raw)
To: Alexander Duyck
Cc: David E. Box, Lee Jones, Darren Hart, Andy Shevchenko,
Bjorn Helgaas, Alexander Duyck, Hans de Goede, Alexey Budankov,
Linux Kernel Mailing List, Platform Driver, linux-pci
On Thu, Oct 1, 2020 at 9:33 PM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Thu, Oct 1, 2020 at 9:37 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Thu, Oct 1, 2020 at 4:43 AM David E. Box <david.e.box@linux.intel.com> wrote:
...
> Arguably not much. I'll drop the comment.
>
> > > + control &= ~(CRASHLOG_FLAG_MASK | CRASHLOG_FLAG_DISABLE);
> >
> > How does the second constant play any role here?
>
> The "control" flags are bits 28-31, while the disable flag is bit 27
> if I recall.
Okay, then it adds more confusion to the same comment here and there.
Good you are about to drop the comment.
> Specifically bit 31 is read only, bit 28 will clear bit 31, bit 29
> will cause the crashlog to be generated and set bit 31, and bit 30 is
> just reserved 0.
Can this be added as a comment somewhere in the code?
...
> > > + ret = intel_pmt_dev_create(entry, &pmt_crashlog_ns, parent);
> > > + if (!ret)
> > > + return 0;
(2)
> > > +
> > > + dev_err(parent, "Failed to add crashlog controls\n");
> > > + intel_pmt_dev_destroy(entry, &pmt_crashlog_ns);
> > > +
> > > + return ret;
> >
> > Can we use traditional patterns?
> > if (ret) {
> > ...
> > }
> > return ret;
>
> I can switch it if that is preferred.
Yes, please. The (2) is really hard to parse (easy to miss ! part and
be confused by return 0 one).
...
> > Are you going to duplicate this in each driver? Consider to refactor
> > to avoid duplication of a lot of code.
>
> So the issue lies in the complexity of pmt_telem_add_entry versus
> pmt_crashlog_add_entry. Specifically I end up needing disc_res and the
> discovery table when I go to create the controls for the crashlog
> device. Similarly we have a third device that we plan to add called a
> watcher which will require us to keep things split up like this so we
> thought it best to split it up this way.
Could you revisit and think how this can be deduplicated. I see at
least one variant with a hooks (callbacks) which you supply depending
on the driver, but the for-loop is kept in one place.
...
> > > + .name = DRV_NAME,
> >
> > > +MODULE_ALIAS("platform:" DRV_NAME);
> >
> > I'm not sure I have interpreted this:
> > - Use 'raw' string instead of defines for device names
> > correctly. Can you elaborate?
>
> Again I am not sure what this is in reference to. If you can point me
> to some documentation somewhere I can take a look.
Reference to your own changelog of this series!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V7 5/5] platform/x86: Intel PMT Crashlog capability driver
2020-10-01 18:46 ` Andy Shevchenko
@ 2020-10-01 19:15 ` Alexander Duyck
0 siblings, 0 replies; 17+ messages in thread
From: Alexander Duyck @ 2020-10-01 19:15 UTC (permalink / raw)
To: Andy Shevchenko
Cc: David E. Box, Lee Jones, Darren Hart, Andy Shevchenko,
Bjorn Helgaas, Alexander Duyck, Hans de Goede, Alexey Budankov,
Linux Kernel Mailing List, Platform Driver, linux-pci
On Thu, Oct 1, 2020 at 11:47 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Oct 1, 2020 at 9:33 PM Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
> > On Thu, Oct 1, 2020 at 9:37 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Thu, Oct 1, 2020 at 4:43 AM David E. Box <david.e.box@linux.intel.com> wrote:
>
> ...
>
> > Arguably not much. I'll drop the comment.
> >
> > > > + control &= ~(CRASHLOG_FLAG_MASK | CRASHLOG_FLAG_DISABLE);
> > >
> > > How does the second constant play any role here?
> >
> > The "control" flags are bits 28-31, while the disable flag is bit 27
> > if I recall.
>
> Okay, then it adds more confusion to the same comment here and there.
> Good you are about to drop the comment.
>
> > Specifically bit 31 is read only, bit 28 will clear bit 31, bit 29
> > will cause the crashlog to be generated and set bit 31, and bit 30 is
> > just reserved 0.
>
> Can this be added as a comment somewhere in the code?
I'll do that with the definitions themselves.
> ...
>
> > > > + ret = intel_pmt_dev_create(entry, &pmt_crashlog_ns, parent);
> > > > + if (!ret)
> > > > + return 0;
>
> (2)
>
> > > > +
> > > > + dev_err(parent, "Failed to add crashlog controls\n");
> > > > + intel_pmt_dev_destroy(entry, &pmt_crashlog_ns);
> > > > +
> > > > + return ret;
> > >
> > > Can we use traditional patterns?
> > > if (ret) {
> > > ...
> > > }
> > > return ret;
> >
> > I can switch it if that is preferred.
>
> Yes, please. The (2) is really hard to parse (easy to miss ! part and
> be confused by return 0 one).
>
> ...
>
> > > Are you going to duplicate this in each driver? Consider to refactor
> > > to avoid duplication of a lot of code.
> >
> > So the issue lies in the complexity of pmt_telem_add_entry versus
> > pmt_crashlog_add_entry. Specifically I end up needing disc_res and the
> > discovery table when I go to create the controls for the crashlog
> > device. Similarly we have a third device that we plan to add called a
> > watcher which will require us to keep things split up like this so we
> > thought it best to split it up this way.
>
> Could you revisit and think how this can be deduplicated. I see at
> least one variant with a hooks (callbacks) which you supply depending
> on the driver, but the for-loop is kept in one place.
I'll see what I can do.
> ...
>
> > > > + .name = DRV_NAME,
> > >
> > > > +MODULE_ALIAS("platform:" DRV_NAME);
> > >
> > > I'm not sure I have interpreted this:
> > > - Use 'raw' string instead of defines for device names
> > > correctly. Can you elaborate?
> >
> > Again I am not sure what this is in reference to. If you can point me
> > to some documentation somewhere I can take a look.
>
> Reference to your own changelog of this series!
So the issue is we have two authors so it is a matter of keeping track
of who is working on what.
So apparently that was in reference to the MFD driver which was
instantiating the devices using defines and there was only one spot
where they were being used. The reason why I was confused is because
the commit message had nothing to do with this patch and it I haven't
really done any work on the MFD driver myself. The link to the 'raw'
discussion can be found here:
https://lore.kernel.org/lkml/20200728075859.GH1850026@dell/
^ permalink raw reply [flat|nested] 17+ messages in thread