Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200911194549.12780-1-david.e.box@linux.intel.com>
 <20200911194549.12780-4-david.e.box@linux.intel.com> <6e3738db-bfff-7fd2-65e6-bd0d126f9eaa@redhat.com>
In-Reply-To: <6e3738db-bfff-7fd2-65e6-bd0d126f9eaa@redhat.com>
From:   Alexander Duyck <alexander.duyck@gmail.com>
Date:   Mon, 14 Sep 2020 11:07:54 -0700
Message-ID: <CAKgT0UcxSwRseMBdMd0_HDUS=JGZDAZnAy-tkLkB-hMXLYtucw@mail.gmail.com>
Subject: Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     "David E. Box" <david.e.box@linux.intel.com>, lee.jones@linaro.org,
        dvhart@infradead.org, andy@infradead.org,
        Alexander Duyck <alexander.h.duyck@linux.intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        platform-driver-x86@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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

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

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

I'll update things to add the mutex.

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

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

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

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

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

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