[PATCH 07/10] platform/x86/intel/pmt: use a version struct

Ruhl, Michael J michael.j.ruhl at intel.com
Mon Jun 2 17:57:30 UTC 2025


>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen at linux.intel.com>
>Sent: Saturday, May 31, 2025 1:46 AM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>
>Cc: platform-driver-x86 at vger.kernel.org; intel-xe at lists.freedesktop.org; Hans
>de Goede <hdegoede at redhat.com>; De Marchi, Lucas
><lucas.demarchi at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>
>Subject: Re: [PATCH 07/10] platform/x86/intel/pmt: use a version struct
>
>On Fri, 30 May 2025, 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 | 177 ++++++++++++++--------
>>  1 file changed, 113 insertions(+), 64 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
>b/drivers/platform/x86/intel/pmt/crashlog.c
>> index 09cd0a1346f3..e6eea8809a56 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 = CONTROL_OFFSET,
>> +	.status.clear = TYPE1_VER0_CLEAR,
>> +	.status.complete = TYPE1_VER0_COMPLETE,
>> +	.status.disable = TYPE1_VER0_DISABLE,
>> +
>> +	.control.offset = CONTROL_OFFSET,
>> +	.control.trigger_mask = TYPE1_VER0_TRIGGER_MASK,
>> +	.control.clear = TYPE1_VER0_CLEAR,
>> +	.control.disable = TYPE1_VER0_DISABLE,
>> +	.control.manual = TYPE1_VER0_EXECUTE,
>> +};
>> +
>>  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 {
>> @@ -60,24 +98,10 @@ struct pmt_crashlog_priv {
>>  };
>>
>>  /*
>> - * I/O
>> + * 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_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_TRIGGER_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);
>> @@ -93,40 +117,64 @@ static bool pmt_crashlog_supported(struct
>intel_pmt_entry *entry)
>>  	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
>>  }
>>
>> +/*
>> + * I/O
>> + */
>> +static bool pmt_crashlog_complete(struct intel_pmt_entry *entry,
>> +				  const struct crashlog_status *status)
>
>I didn't fine comb this change but IMO it would be better to pass just
>crashlog_info here, the same applies to the other functions too taking
>just one of the substructs.

The issue I ran into was that the info and entry are separate members...  So I would pass crashlog_entry
and then do this:


static bool pmt_crashlog_complete(struct crashlog_entry *crashlog) 
{
	struct intel_pmt_entry *entry = &crashlog->entry;
	struct crashlog_status *status = &crashlog->info->status;
	int reg = readl(entry->disc_table + status->offset;

	return !!(reg & status->complete);
}

If that is preferred, I will update.

M

>Overall, this change looks definitely better than the earlier version
>that was based on those if () constructs. Good work! :-)

I agree.  This is a lot cleaner than the previous attempt.  Thanks for the hints! 😊

M

>> +{
>> +	u32 reg = readl(entry->disc_table + status->offset);
>> +
>> +	/* return current value of the crashlog complete flag */
>> +	return !!(reg & status->complete);
>> +}
>> +
>> +static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry,
>> +				  const struct crashlog_status *status)
>> +{
>> +	u32 reg = readl(entry->disc_table + status->offset);
>> +
>> +	/* return current value of the crashlog disabled flag */
>> +	return !!(reg & status->disable);
>> +}
>> +
>>  static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
>> +				     const struct crashlog_control *control,
>>  				     bool disable)
>>  {
>> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
>> +	u32 reg = readl(entry->disc_table + control->offset);
>>
>>  	/* clear trigger bits so we are only modifying disable flag */
>> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
>> +	reg &= ~control->trigger_mask;
>>
>>  	if (disable)
>> -		control |= CRASHLOG_FLAG_DISABLE;
>> +		reg |= control->disable;
>>  	else
>> -		control &= ~CRASHLOG_FLAG_DISABLE;
>> +		reg &= ~control->disable;
>>
>> -	writel(control, entry->disc_table + CONTROL_OFFSET);
>> +	writel(reg, entry->disc_table + control->offset);
>>  }
>>
>> -static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
>> +static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry,
>> +				   const struct crashlog_control *control)
>>  {
>> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
>> +	u32 reg = readl(entry->disc_table + control->offset);
>>
>> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
>> -	control |= CRASHLOG_FLAG_TRIGGER_CLEAR;
>> +	reg &= ~control->trigger_mask;
>> +	reg |= control->clear;
>>
>> -	writel(control, entry->disc_table + CONTROL_OFFSET);
>> +	writel(reg, entry->disc_table + control->offset);
>>  }
>>
>> -static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
>> +static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry,
>> +				     const struct crashlog_control *control)
>>  {
>> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
>> +	u32 reg = readl(entry->disc_table + control->offset);
>>
>> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
>> -	control |= CRASHLOG_FLAG_TRIGGER_EXECUTE;
>> +	reg &= ~control->trigger_mask;
>> +	reg |= control->manual;
>>
>> -	writel(control, entry->disc_table + CONTROL_OFFSET);
>> +	writel(reg, entry->disc_table + control->offset);
>>  }
>>
>>  /*
>> @@ -135,8 +183,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);
>> -	int enabled = !pmt_crashlog_disabled(entry);
>> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
>> +	int enabled = !pmt_crashlog_disabled(&crashlog->entry, &crashlog-
>>info->status);
>>
>>  	return sprintf(buf, "%d\n", enabled);
>>  }
>> @@ -145,19 +193,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->entry, &crashlog->info->control,
>!enabled);
>>
>>  	return count;
>>  }
>> @@ -166,11 +214,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;
>>  	int trigger;
>>
>> -	entry = dev_get_drvdata(dev);
>> -	trigger = pmt_crashlog_complete(entry);
>> +	crashlog = dev_get_drvdata(dev);
>> +	trigger = pmt_crashlog_complete(&crashlog->entry, &crashlog->info-
>>status);
>>
>>  	return sprintf(buf, "%d\n", trigger);
>>  }
>> @@ -179,32 +227,32 @@ 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 (!trigger) {
>> -		pmt_crashlog_set_clear(&entry->entry);
>> +		pmt_crashlog_set_clear(&crashlog->entry, &crashlog->info-
>>control);
>>  		return count;
>>  	}
>>
>>  	/* we cannot trigger a new crash if one is still pending */
>> -	if (pmt_crashlog_complete(&entry->entry))
>> +	if (pmt_crashlog_complete(&crashlog->entry, &crashlog->info-
>>status))
>>  		return -EEXIST;
>>
>>  	/* if device is currently disabled, return busy */
>> -	if (pmt_crashlog_disabled(&entry->entry))
>> +	if (pmt_crashlog_disabled(&crashlog->entry, &crashlog->info->status))
>>  		return -EBUSY;
>>
>> -	pmt_crashlog_set_execute(&entry->entry);
>> +	pmt_crashlog_set_execute(&crashlog->entry, &crashlog->info-
>>control);
>>
>>  	return count;
>>  }
>> @@ -230,9 +278,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);
>>
>
>--
> i.



More information about the Intel-xe mailing list