From: "David E. Box" <david.e.box@linux.intel.com>
To: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
platform-driver-x86@vger.kernel.org,
rajvi.jingar@linux.intel.com
Subject: Re: [PATCH 04/11] platform/x86/intel/pmt: telemetry: Export API to read telemetry
Date: Tue, 26 Sep 2023 16:59:02 -0700 [thread overview]
Message-ID: <3608730a228bd892ddf57def8a240da75ca0b50d.camel@linux.intel.com> (raw)
In-Reply-To: <8fd2ab40-975f-768-57d7-f14fda99860@linux.intel.com>
On Tue, 2023-09-26 at 18:40 +0300, Ilpo Järvinen wrote:
> On Fri, 22 Sep 2023, David E. Box wrote:
>
> > Export symbols to allow access to Intel PMT Telemetry data on available
> > devices. Provides APIs to search, register, and read telemetry using a
> > kref managed pointer that serves as a handle to a telemetry endpoint.
> >
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> > drivers/platform/x86/intel/pmt/class.c | 21 ++-
> > drivers/platform/x86/intel/pmt/class.h | 14 ++
> > drivers/platform/x86/intel/pmt/telemetry.c | 198 ++++++++++++++++++++-
> > drivers/platform/x86/intel/pmt/telemetry.h | 129 ++++++++++++++
> > 4 files changed, 354 insertions(+), 8 deletions(-)
> > create mode 100644 drivers/platform/x86/intel/pmt/telemetry.h
> >
> > diff --git a/drivers/platform/x86/intel/pmt/class.c
> > b/drivers/platform/x86/intel/pmt/class.c
> > index 142a24e3727d..4b53940a64e2 100644
> > --- a/drivers/platform/x86/intel/pmt/class.c
> > +++ b/drivers/platform/x86/intel/pmt/class.c
> > @@ -17,7 +17,7 @@
> > #include "../vsec.h"
> > #include "class.h"
> >
> > -#define PMT_XA_START 0
> > +#define PMT_XA_START 1
>
> How is that related to what the changelog states, that is, exporting some
> API???
...
>
> > #define PMT_XA_MAX INT_MAX
> > #define PMT_XA_LIMIT XA_LIMIT(PMT_XA_START, PMT_XA_MAX)
> > #define GUID_SPR_PUNIT 0x9956f43f
> > @@ -247,6 +247,7 @@ static int intel_pmt_dev_register(struct intel_pmt_entry
> > *entry,
> > struct intel_pmt_namespace *ns,
> > struct device *parent)
> > {
> > + struct intel_vsec_device *ivdev = dev_to_ivdev(parent);
> > struct resource res = {0};
> > struct device *dev;
> > int ret;
> > @@ -270,7 +271,7 @@ static int intel_pmt_dev_register(struct intel_pmt_entry
> > *entry,
> > if (ns->attr_grp) {
> > ret = sysfs_create_group(entry->kobj, ns->attr_grp);
> > if (ret)
> > - goto fail_sysfs;
> > + goto fail_sysfs_create_group;
> > }
> >
> > /* if size is 0 assume no data buffer, so no file needed */
> > @@ -295,13 +296,23 @@ static int intel_pmt_dev_register(struct
> > intel_pmt_entry *entry,
> > entry->pmt_bin_attr.size = entry->size;
> >
> > ret = sysfs_create_bin_file(&dev->kobj, &entry->pmt_bin_attr);
> > - if (!ret)
> > - return 0;
> > + if (ret)
> > + goto fail_ioremap;
> >
> > + if (ns->pmt_add_endpoint) {
> > + ret = ns->pmt_add_endpoint(entry, ivdev->pcidev);
> > + if (ret)
> > + goto fail_add_endpoint;
> > + }
> > +
> > + return 0;
> > +
> > +fail_add_endpoint:
> > + sysfs_remove_bin_file(entry->kobj, &entry->pmt_bin_attr);
> > fail_ioremap:
> > if (ns->attr_grp)
> > sysfs_remove_group(entry->kobj, ns->attr_grp);
> > -fail_sysfs:
> > +fail_sysfs_create_group:
> > device_unregister(dev);
> > fail_dev_create:
> > xa_erase(ns->xa, entry->devid);
> > diff --git a/drivers/platform/x86/intel/pmt/class.h
> > b/drivers/platform/x86/intel/pmt/class.h
> > index e477a19f6700..d23c63b73ab7 100644
> > --- a/drivers/platform/x86/intel/pmt/class.h
> > +++ b/drivers/platform/x86/intel/pmt/class.h
> > @@ -9,6 +9,7 @@
> > #include <linux/io.h>
> >
> > #include "../vsec.h"
> > +#include "telemetry.h"
> >
> > /* PMT access types */
> > #define ACCESS_BARID 2
> > @@ -18,6 +19,16 @@
> > #define GET_BIR(v) ((v) & GENMASK(2, 0))
> > #define GET_ADDRESS(v) ((v) & GENMASK(31, 3))
> >
> > +struct pci_dev;
> > +
> > +struct telem_endpoint {
> > + struct pci_dev *pcidev;
> > + struct telem_header header;
> > + void __iomem *base;
> > + bool present;
> > + struct kref kref;
> > +};
> > +
> > struct intel_pmt_header {
> > u32 base_offset;
> > u32 size;
> > @@ -26,6 +37,7 @@ struct intel_pmt_header {
> > };
> >
> > struct intel_pmt_entry {
> > + struct telem_endpoint *ep;
> > struct intel_pmt_header header;
> > struct bin_attribute pmt_bin_attr;
> > struct kobject *kobj;
> > @@ -43,6 +55,8 @@ struct intel_pmt_namespace {
> > const struct attribute_group *attr_grp;
> > int (*pmt_header_decode)(struct intel_pmt_entry *entry,
> > struct device *dev);
> > + int (*pmt_add_endpoint)(struct intel_pmt_entry *entry,
> > + struct pci_dev *pdev);
> > };
> >
> > bool intel_pmt_is_early_client_hw(struct device *dev);
> > diff --git a/drivers/platform/x86/intel/pmt/telemetry.c
> > b/drivers/platform/x86/intel/pmt/telemetry.c
> > index f86080e8bebd..8b099580cc2c 100644
> > --- a/drivers/platform/x86/intel/pmt/telemetry.c
> > +++ b/drivers/platform/x86/intel/pmt/telemetry.c
> > @@ -30,6 +30,14 @@
> > /* Used by client hardware to identify a fixed telemetry entry*/
> > #define TELEM_CLIENT_FIXED_BLOCK_GUID 0x10000000
> >
> > +#define NUM_BYTES_QWORD(v) ((v) << 3)
> > +#define SAMPLE_ID_OFFSET(v) ((v) << 3)
> > +
> > +#define NUM_BYTES_DWORD(v) ((v) << 2)
> > +#define SAMPLE_ID_OFFSET32(v) ((v) << 2)
> > +
> > +static DEFINE_MUTEX(ep_lock);
> > +
> > enum telem_type {
> > TELEM_TYPE_PUNIT = 0,
> > TELEM_TYPE_CRASHLOG,
> > @@ -84,21 +92,203 @@ static int pmt_telem_header_decode(struct
> > intel_pmt_entry *entry,
> > return 0;
> > }
> >
> > +static int pmt_telem_add_endpoint(struct intel_pmt_entry *entry,
> > + struct pci_dev *pdev)
> > +{
> > + struct telem_endpoint *ep;
> > +
> > + /*
> > + * Endpoint lifetimes are managed by kref, not devres.
> > + */
> > + entry->ep = kzalloc(sizeof(*(entry->ep)), GFP_KERNEL);
> > + if (!entry->ep)
> > + return -ENOMEM;
> > +
> > + ep = entry->ep;
> > + ep->pcidev = pdev;
> > + ep->header.access_type = entry->header.access_type;
> > + ep->header.guid = entry->header.guid;
> > + ep->header.base_offset = entry->header.base_offset;
> > + ep->header.size = entry->header.size;
> > + ep->base = entry->base;
> > + ep->present = true;
> > +
> > + kref_init(&ep->kref);
> > +
> > + return 0;
> > +}
> > +
> > static DEFINE_XARRAY_ALLOC(telem_array);
> > static struct intel_pmt_namespace pmt_telem_ns = {
> > .name = "telem",
> > .xa = &telem_array,
> > .pmt_header_decode = pmt_telem_header_decode,
> > + .pmt_add_endpoint = pmt_telem_add_endpoint,
> > };
> >
> > +/* Called when all users unregister and the device is removed */
> > +static void pmt_telem_ep_release(struct kref *kref)
> > +{
> > + struct telem_endpoint *ep;
> > +
> > + ep = container_of(kref, struct telem_endpoint, kref);
> > + kfree(ep);
> > +}
> > +
> > +/*
> > + * driver api
> > + */
> > +int pmt_telem_get_next_endpoint(int start)
> > +{
> > + struct intel_pmt_entry *entry;
> > + unsigned long found_idx;
> > +
> > + mutex_lock(&ep_lock);
> > + xa_for_each_start(&telem_array, found_idx, entry, start) {
> > + /*
> > + * Return first found index after start.
> > + * 0 is not valid id.
> > + */
>
> I guess this has to do with the 0->1 change I flagged above. But if that's
> the case, it absolutely must be mentioned in the changelog with
> explanation!
Yes it does. Will mention in the changelog.
>
> > + if (found_idx > start)
> > + break;
> > + }
> > + mutex_unlock(&ep_lock);
> > +
> > + return found_idx == start ? 0 : found_idx;
>
> Why you need signed values in/out? Should this function just be using
> unsigned int (or long) for start and return value?
It not needed anymore (originally was returning an error). Will change it to
unsigned.
>
> > +}
> > +EXPORT_SYMBOL_NS_GPL(pmt_telem_get_next_endpoint, INTEL_PMT_TELEMETRY);
> > +
> > +struct telem_endpoint *pmt_telem_register_endpoint(int devid)
> > +{
> > + struct intel_pmt_entry *entry;
> > + unsigned long index = devid;
> > +
> > + mutex_lock(&ep_lock);
> > + entry = xa_find(&telem_array, &index, index, XA_PRESENT);
> > + if (!entry) {
> > + mutex_unlock(&ep_lock);
> > + return ERR_PTR(-ENXIO);
> > + }
> > +
> > + kref_get(&entry->ep->kref);
> > + mutex_unlock(&ep_lock);
> > +
> > + return entry->ep;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(pmt_telem_register_endpoint, INTEL_PMT_TELEMETRY);
> > +
> > +void pmt_telem_unregister_endpoint(struct telem_endpoint *ep)
> > +{
> > + kref_put(&ep->kref, pmt_telem_ep_release);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(pmt_telem_unregister_endpoint, INTEL_PMT_TELEMETRY);
> > +
> > +int pmt_telem_get_endpoint_info(int devid,
> > + struct telem_endpoint_info *info)
> > +{
> > + struct intel_pmt_entry *entry;
> > + unsigned long index = devid;
> > + int err = 0;
> > +
> > + if (!info)
> > + return -EINVAL;
> > +
> > + mutex_lock(&ep_lock);
> > + entry = xa_find(&telem_array, &index, index, XA_PRESENT);
> > + if (!entry) {
> > + err = -ENXIO;
> > + goto unlock;
> > + }
> > +
> > + info->pdev = entry->ep->pcidev;
> > + info->header = entry->ep->header;
> > +
> > +unlock:
> > + mutex_unlock(&ep_lock);
> > + return err;
> > +
> > +}
> > +EXPORT_SYMBOL_NS_GPL(pmt_telem_get_endpoint_info, INTEL_PMT_TELEMETRY);
> > +
> > +int
> > +pmt_telem_read(struct telem_endpoint *ep, u32 id, u64 *data, u32 count)
> > +{
> > + u32 offset, size;
> > +
> > + if (!ep->present)
> > + return -ENODEV;
> > +
> > + offset = SAMPLE_ID_OFFSET(id);
> > + size = ep->header.size;
> > +
> > + if ((offset + NUM_BYTES_QWORD(count)) > size)
>
> Extra parenthesis.
Ack
David
>
>
next prev parent reply other threads:[~2023-09-27 0:46 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-22 21:30 [PATCH 00/11] intel_pmc: Add telemetry API to read counters David E. Box
2023-09-22 21:30 ` [PATCH 01/11] platform/x86/intel/vsec: Add intel_vsec_register David E. Box
2023-09-26 14:17 ` Ilpo Järvinen
2023-09-26 23:51 ` David E. Box
2023-09-22 21:30 ` [PATCH 02/11] platform/x86/intel/vsec: Add base address field David E. Box
2023-09-26 14:39 ` Ilpo Järvinen
2023-09-22 21:30 ` [PATCH 03/11] platform/x86/intel/pmt: Add header to struct intel_pmt_entry David E. Box
2023-09-26 14:43 ` Ilpo Järvinen
2023-09-22 21:30 ` [PATCH 04/11] platform/x86/intel/pmt: telemetry: Export API to read telemetry David E. Box
2023-09-26 15:40 ` Ilpo Järvinen
2023-09-26 23:59 ` David E. Box [this message]
2023-09-22 21:30 ` [PATCH 05/11] platform/x86:intel/pmc: Move get_low_power_modes function David E. Box
2023-09-26 15:56 ` Ilpo Järvinen
2023-09-27 0:16 ` David E. Box
2023-09-22 21:30 ` [PATCH 06/11] platform/x86/intel/pmc: Split pmc_core_ssram_get_pmc() David E. Box
2023-09-22 21:30 ` [PATCH 07/11] platform/x86/intel/pmc: Find and register PMC telemetry entries David E. Box
2023-09-22 21:30 ` [PATCH 08/11] platform/x86/intel/pmc: Display LPM requirements for multiple PMCs David E. Box
2023-09-22 21:30 ` [PATCH 09/11] platform/x86/intel/pmc: Retrieve LPM information using Intel PMT David E. Box
2023-09-26 16:07 ` Ilpo Järvinen
2023-09-22 21:30 ` [PATCH 10/11] platform/x86/intel/pmc: Read low power mode requirements for MTL-M and MTL-P David E. Box
2023-09-26 16:03 ` Ilpo Järvinen
2023-09-27 0:20 ` David E. Box
2023-09-22 21:30 ` [PATCH 11/11] platform/x86/intel/pmc: Add debug attribute for Die C6 counter David E. Box
2023-09-26 16:11 ` Ilpo Järvinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3608730a228bd892ddf57def8a240da75ca0b50d.camel@linux.intel.com \
--to=david.e.box@linux.intel.com \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=platform-driver-x86@vger.kernel.org \
--cc=rajvi.jingar@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox