[PATCH v3 10/11] platform/x86/intel/pmt: use a version struct
Ruhl, Michael J
michael.j.ruhl at intel.com
Mon Jun 9 14:43:11 UTC 2025
>-----Original Message-----
>From: David E. Box <david.e.box at linux.intel.com>
>Sent: Friday, June 6, 2025 6:58 PM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>; platform-driver-
>x86 at vger.kernel.org; intel-xe at lists.freedesktop.org; hdegoede at redhat.com;
>ilpo.jarvinen at linux.intel.com; De Marchi, Lucas <lucas.demarchi at intel.com>;
>Vivi, Rodrigo <rodrigo.vivi at intel.com>; thomas.hellstrom at linux.intel.com;
>airlied at gmail.com; simona at ffwll.ch
>Subject: Re: [PATCH v3 10/11] platform/x86/intel/pmt: use a version struct
>
>On Thu, 2025-06-05 at 14:44 -0400, Michael J. Ruhl wrote:
>> In preparation for supporting multiple crashlog versions, use a struct
>> to keep bit offset info for the status and control bits.
>>
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl at intel.com>
>> ---
>> drivers/platform/x86/intel/pmt/crashlog.c | 176 ++++++++++++++--------
>> 1 file changed, 112 insertions(+), 64 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
>> b/drivers/platform/x86/intel/pmt/crashlog.c
>> index 318d7a21f00e..fe6563721886 100644
>> --- a/drivers/platform/x86/intel/pmt/crashlog.c
>> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
>> @@ -22,21 +22,6 @@
>> /* Crashlog discovery header types */
>> #define CRASH_TYPE_OOBMSM 1
>>
>> -/* Control Flags */
>> -#define CRASHLOG_FLAG_DISABLE BIT(28)
>> -
>> -/*
>> - * Bits 29 and 30 control the state of bit 31.
>> - *
>> - * Bit 29 will clear bit 31, if set, allowing a new crashlog to be captured.
>> - * Bit 30 will immediately trigger a crashlog to be generated, setting bit
>> 31.
>> - * Bit 31 is the read-only status with a 1 indicating log is complete.
>> - */
>> -#define CRASHLOG_FLAG_TRIGGER_CLEAR BIT(29)
>> -#define CRASHLOG_FLAG_TRIGGER_EXECUTE BIT(30)
>> -#define CRASHLOG_FLAG_TRIGGER_COMPLETE BIT(31)
>> -#define CRASHLOG_FLAG_TRIGGER_MASK GENMASK(31, 28)
>> -
>> /* Crashlog Discovery Header */
>> #define CONTROL_OFFSET 0x0
>> #define GUID_OFFSET 0x4
>> @@ -48,10 +33,63 @@
>> /* size is in bytes */
>> #define GET_SIZE(v) ((v) * sizeof(u32))
>>
>> +/*
>> + * Type 1 Version 0
>> + * status and control registers are combined.
>> + *
>> + * Bits 29 and 30 control the state of bit 31.
>> + * Bit 29 will clear bit 31, if set, allowing a new crashlog to be captured.
>> + * Bit 30 will immediately trigger a crashlog to be generated, setting bit
>> 31.
>> + * Bit 31 is the read-only status with a 1 indicating log is complete.
>> + */
>> +#define TYPE1_VER0_STATUS_OFFSET 0x00
>> +#define TYPE1_VER0_CONTROL_OFFSET 0x00
>> +
>> +#define TYPE1_VER0_DISABLE BIT(28)
>> +#define TYPE1_VER0_CLEAR BIT(29)
>> +#define TYPE1_VER0_EXECUTE BIT(30)
>> +#define TYPE1_VER0_COMPLETE BIT(31)
>> +#define TYPE1_VER0_TRIGGER_MASK GENMASK(31, 28)
>> +
>> +/* After offset, order alphabetically, not bit ordered */
>> +struct crashlog_status {
>> + u32 offset;
>> + u32 clear;
>> + u32 complete;
>> + u32 disable;
>> +};
>> +
>> +struct crashlog_control {
>> + u32 offset;
>> + u32 trigger_mask;
>> + u32 clear;
>> + u32 disable;
>> + u32 manual;
>> +};
>> +
>> +struct crashlog_info {
>> + struct crashlog_status status;
>> + struct crashlog_control control;
>> +};
>> +
>> +const struct crashlog_info crashlog_type1_ver0 = {
>> + .status.offset = TYPE1_VER0_STATUS_OFFSET,
>> + .status.clear = TYPE1_VER0_CLEAR,
>> + .status.complete = TYPE1_VER0_COMPLETE,
>> + .status.disable = TYPE1_VER0_DISABLE,
>> +
>> + .control.offset = TYPE1_VER0_CONTROL_OFFSET,
>> + .control.trigger_mask = TYPE1_VER0_TRIGGER_MASK,
>> + .control.clear = TYPE1_VER0_CLEAR,
>> + .control.disable = TYPE1_VER0_DISABLE,
>> + .control.manual = TYPE1_VER0_EXECUTE,
>> +};
>> +
>
>static?
Will update.
M
>David
>
>> struct crashlog_entry {
>> /* entry must be first member of struct */
>> struct intel_pmt_entry entry;
>> struct mutex control_mutex;
>> + const struct crashlog_info *info;
>> };
>>
>> struct pmt_crashlog_priv {
>> @@ -59,74 +97,82 @@ struct pmt_crashlog_priv {
>> struct crashlog_entry entry[];
>> };
>>
>> +/*
>> + * This is the generic access to a PMT struct. So the use of
>> + * struct crashlog_entry
>> + * doesn't "make sense" here.
>> + */
>> +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);
>> +
>> + /*
>> + * Currently 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;
>> +}
>> +
>> /*
>> * I/O
>> */
>> +
>> #define SET true
>> #define CLEAR false
>>
>> -static void read_modify_write(struct intel_pmt_entry *entry, u32 bit, bool
>> set)
>> +static void read_modify_write(struct crashlog_entry *crashlog, u32 bit, bool
>> set)
>> {
>> - u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
>> + const struct crashlog_control *control = &crashlog->info->control;
>> + struct intel_pmt_entry *entry = &crashlog->entry;
>> + u32 reg = readl(entry->disc_table + control->offset);
>>
>> - reg &= ~CRASHLOG_FLAG_TRIGGER_MASK;
>> + reg &= ~control->trigger_mask;
>>
>> if (set)
>> reg |= bit;
>> else
>> reg &= bit;
>>
>> - writel(reg, entry->disc_table + CONTROL_OFFSET);
>> + writel(reg, entry->disc_table + control->offset);
>> }
>>
>> -static bool read_check(struct intel_pmt_entry *entry, u32 bit)
>> +static bool read_check(struct crashlog_entry *crashlog, u32 bit)
>> {
>> - u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
>> + const struct crashlog_status *status = &crashlog->info->status;
>> + u32 reg = readl(crashlog->entry.disc_table + status->offset);
>>
>> return !!(reg & bit);
>> }
>>
>> -static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
>> +static bool pmt_crashlog_complete(struct crashlog_entry *crashlog)
>> {
>> /* return current value of the crashlog complete flag */
>> - return read_check(entry, CRASHLOG_FLAG_TRIGGER_COMPLETE);
>> + return read_check(crashlog, crashlog->info->status.complete);
>> }
>>
>> -static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry)
>> +static bool pmt_crashlog_disabled(struct crashlog_entry *crashlog)
>> {
>> /* return current value of the crashlog disabled flag */
>> - return read_check(entry, 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);
>> -
>> - /*
>> - * Currently 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;
>> + return read_check(crashlog, crashlog->info->status.disable);
>> }
>>
>> -static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
>> - bool disable)
>> +static void pmt_crashlog_set_disable(struct crashlog_entry *crashlog, bool
>> disable)
>> {
>> - read_modify_write(entry, CRASHLOG_FLAG_DISABLE, disable);
>> + read_modify_write(crashlog, crashlog->info->control.disable,
>> disable);
>> }
>>
>> -static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
>> +static void pmt_crashlog_set_clear(struct crashlog_entry *crashlog)
>> {
>> - read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_CLEAR, SET);
>> + read_modify_write(crashlog, crashlog->info->control.clear, SET);
>> }
>>
>> -static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
>> +static void pmt_crashlog_set_execute(struct crashlog_entry *crashlog)
>> {
>> - read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_EXECUTE, SET);
>> + read_modify_write(crashlog, crashlog->info->control.manual, SET);
>> }
>>
>> /*
>> @@ -135,8 +181,8 @@ static void pmt_crashlog_set_execute(struct
>> intel_pmt_entry *entry)
>> static ssize_t
>> enable_show(struct device *dev, struct device_attribute *attr, char *buf)
>> {
>> - struct intel_pmt_entry *entry = dev_get_drvdata(dev);
>> - bool enabled = !pmt_crashlog_disabled(entry);
>> + struct crashlog_entry *crashlog = dev_get_drvdata(dev);
>> + bool enabled = !pmt_crashlog_disabled(crashlog);
>>
>> return sprintf(buf, "%d\n", enabled);
>> }
>> @@ -145,19 +191,19 @@ static ssize_t
>> enable_store(struct device *dev, struct device_attribute *attr,
>> const char *buf, size_t count)
>> {
>> - struct crashlog_entry *entry;
>> + struct crashlog_entry *crashlog;
>> bool enabled;
>> int result;
>>
>> - entry = dev_get_drvdata(dev);
>> + crashlog = dev_get_drvdata(dev);
>>
>> result = kstrtobool(buf, &enabled);
>> if (result)
>> return result;
>>
>> - guard(mutex)(&entry->control_mutex);
>> + guard(mutex)(&crashlog->control_mutex);
>>
>> - pmt_crashlog_set_disable(&entry->entry, !enabled);
>> + pmt_crashlog_set_disable(crashlog, !enabled);
>>
>> return count;
>> }
>> @@ -166,11 +212,11 @@ static DEVICE_ATTR_RW(enable);
>> static ssize_t
>> trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
>> {
>> - struct intel_pmt_entry *entry;
>> + struct crashlog_entry *crashlog;
>> bool trigger;
>>
>> - entry = dev_get_drvdata(dev);
>> - trigger = pmt_crashlog_complete(entry);
>> + crashlog = dev_get_drvdata(dev);
>> + trigger = pmt_crashlog_complete(crashlog);
>>
>> return sprintf(buf, "%d\n", trigger);
>> }
>> @@ -179,32 +225,33 @@ static ssize_t
>> trigger_store(struct device *dev, struct device_attribute *attr,
>> const char *buf, size_t count)
>> {
>> - struct crashlog_entry *entry;
>> + struct crashlog_entry *crashlog;
>> bool trigger;
>> int result;
>>
>> - entry = dev_get_drvdata(dev);
>> + crashlog = dev_get_drvdata(dev);
>>
>> result = kstrtobool(buf, &trigger);
>> if (result)
>> return result;
>>
>> - guard(mutex)(&entry->control_mutex);
>> + guard(mutex)(&crashlog->control_mutex);
>>
>> /* if device is currently disabled, return busy */
>> - if (pmt_crashlog_disabled(&entry->entry))
>> + if (pmt_crashlog_disabled(crashlog))
>> return -EBUSY;
>>
>> if (!trigger) {
>> - pmt_crashlog_set_clear(&entry->entry);
>> + pmt_crashlog_set_clear(crashlog);
>> return count;
>> }
>>
>> /* we cannot trigger a new crash if one is still pending */
>> - if (pmt_crashlog_complete(&entry->entry))
>> + if (pmt_crashlog_complete(crashlog))
>> return -EEXIST;
>>
>> - pmt_crashlog_set_execute(&entry->entry);
>> + pmt_crashlog_set_execute(crashlog);
>> +
>>
>> return count;
>> }
>> @@ -230,9 +277,10 @@ static int pmt_crashlog_header_decode(struct
>> intel_pmt_entry *entry,
>> if (!pmt_crashlog_supported(entry))
>> return 1;
>>
>> - /* initialize control mutex */
>> + /* initialize the crashlog struct */
>> crashlog = container_of(entry, struct crashlog_entry, entry);
>> mutex_init(&crashlog->control_mutex);
>> + crashlog->info = &crashlog_type1_ver0;
>>
>> header->access_type = GET_ACCESS(readl(disc_table));
>> header->guid = readl(disc_table + GUID_OFFSET);
More information about the Intel-xe
mailing list