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

[David E. Box]

On Tue, 2020-05-05 at 16:49 +0300, Andy Shevchenko wrote:
> On Tue, May 5, 2020 at 5:32 AM David E. Box <
> david.e.box@xxxxxxxxxxxxxxx> wrote:
> 
> ...
> 
> > 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.
> 
> Is old hardware going to support this in the future?
> (I have in mind Apollo Lake / Broxton)

I don't know of any plans for this.

> 
> > This module manages access to all PMT Telemetry endpoints on a
> > system,
> > regardless of the device exporting them. It creates an
> > intel_pmt_telem
> 
> Name is not the best we can come up with. Would anyone else use PMT?
> Would it be vendor-agnostic ABI?
> (For example, I know that MIPI standardizes tracing protocols, like
> STM, do we have any plans to standardize this one?)

Not at this time. The technology may be used as a feature on non-Intel
devices, but it is Intel owned. Hence the use of DVSEC which allows
hardware to enumerate and get driver support for IP from other vendors.

> 
> telem -> telemetry.
> 
> > class to manage the list. For each endpoint, sysfs files provide
> > GUID and
> > size information as well as a pointer to the parent device the
> > telemetry
> > comes 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
> > device node
> > of the same name allows software to then map the telemetry space
> > for direct
> > access.
> 
> ...
> 
> > +       tristate "Intel PMT telemetry driver"
> 
> I think user should understand what is it from the title (hint: spell
> PMT fully).
> 
> ...
> 
> >  obj-$(CONFIG_PMC_ATOM)                 += pmc_atom.o
> > +obj-$(CONFIG_INTEL_PMT_TELEM)          += intel_pmt_telem.o
> 
> Keep this and Kconfig section in order with the other stuff.
> 
> ...
> 
> bits.h?
> 
> > +#include <linux/cdev.h>
> > +#include <linux/intel-dvsec.h>
> > +#include <linux/io-64-nonatomic-lo-hi.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/xarray.h>
> 
> ...
> 
> > +/* platform device name to bind to driver */
> > +#define TELEM_DRV_NAME         "pmt_telemetry"
> 
> Shouldn't be part of MFD header?

Can place in the dvsec header shared by MFD and drivers.

> 
> ...
> 
> > +#define TELEM_TBIR_MASK                0x7
> 
> GENMASK() ?
> 
> > +struct pmt_telem_priv {
> > +       struct device                   *dev;
> > +       struct intel_dvsec_header       *dvsec;
> > +       struct telem_header             header;
> > +       unsigned long                   base_addr;
> > +       void __iomem                    *disc_table;
> > +       struct cdev                     cdev;
> > +       dev_t                           devt;
> > +       int                             devid;
> > +};
> 
> ...
> 
> > +       unsigned long phys = priv->base_addr;
> > +       unsigned long pfn = PFN_DOWN(phys);
> > +       unsigned long psize;
> > +
> > +       psize = (PFN_UP(priv->base_addr + priv->header.size) - pfn)
> > * PAGE_SIZE;
> > +       if (vsize > psize) {
> > +               dev_err(priv->dev, "Requested mmap size is too
> > large\n");
> > +               return -EINVAL;
> > +       }
> 
> ...
> 
> 
> > +static ssize_t guid_show(struct device *dev, struct
> > device_attribute *attr,
> > +                        char *buf)
> > +{
> > +       struct pmt_telem_priv *priv = dev_get_drvdata(dev);
> > +
> > +       return sprintf(buf, "0x%x\n", priv->header.guid);
> > +}
> 
> So, it's not a GUID but rather some custom number? Can we actually do
> a real GUID / UUID here?

I wish but this is the name it was called. We should have pushed back
more on this. My concern now in calling the attribute something
different is that it will not align with public documentation.

...

> 
> > +       /* Local access and BARID only for now */
> > +       switch (priv->header.access_type) {
> > +       case TELEM_ACCESS_LOCAL:
> > +               if (priv->header.tbir) {
> > +                       dev_err(&pdev->dev,
> > +                               "Unsupported BAR index %d for
> > access type %d\n",
> > +                               priv->header.tbir, priv-
> > >header.access_type);
> > +                       return -EINVAL;
> > +               }
> > +               fallthrough;
> 
> What's the point?

The next case has the break. That case is only there to validate that
it's not the default which would be an error. Will switch this to break
though to make it explicit.

Ack on everything else. Thanks.