Re: [PATCH v4 09/10] platform/x86/intel/pmt: use a version struct

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Tue, 10 Jun 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@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 174 ++++++++++++++--------
>  1 file changed, 108 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index e11865686f2a..7c259b1cf95b 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -23,21 +23,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
> @@ -49,10 +34,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 cleared;
> +	u32 complete;
> +	u32 disabled;
> +};
> +
> +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;
> +};
> +
> +static const struct crashlog_info crashlog_type1_ver0 = {
> +	.status.offset = TYPE1_VER0_STATUS_OFFSET,
> +	.status.cleared = TYPE1_VER0_CLEAR,
> +	.status.complete = TYPE1_VER0_COMPLETE,
> +	.status.disabled = 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,
> +};
> +
>  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,74 +98,76 @@ struct pmt_crashlog_priv {
>  	struct crashlog_entry	entry[];
>  };
>  
> +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
> +#define SET     true
> +#define CLEAR   false

Unrelated space change, and the pre-spacing with tabs is better.

> -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);
> +	return read_check(crashlog, crashlog->info->status.disabled);
>  }
>  
> -static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
> +static void pmt_crashlog_set_disable(struct crashlog_entry *crashlog, bool disable)
>  {
> -	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;
> +	read_modify_write(crashlog, crashlog->info->control.disable, disable);
>  }
>  
> -static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
> -				     bool disable)
> +static void pmt_crashlog_set_clear(struct crashlog_entry *crashlog)
>  {
> -	read_modify_write(entry, CRASHLOG_FLAG_DISABLE, disable);
> +	read_modify_write(crashlog, crashlog->info->control.clear, SET);
>  }
>  
> -static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
> +static void pmt_crashlog_set_execute(struct crashlog_entry *crashlog)
>  {
> -	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_CLEAR, SET);
> -}
> -
> -static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
> -{
> -	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_EXECUTE, SET);
> +	read_modify_write(crashlog, crashlog->info->control.manual, SET);
>  }
>  
>  /*
> @@ -136,8 +176,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);
>  }
> @@ -146,19 +186,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;
>  }
> @@ -167,11 +207,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);
>  }
> @@ -180,32 +220,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);

Could you please do the entry -> crashlog variable rename first in a 
separate patch for the cases where it's already struct crashlog_entry to 
keep this patch focused on real change.
 
>  	/* 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;
>  }
> @@ -231,9 +272,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);

Unrelated to this patch, there seems to be no mutex_destroy() done for 
this mutex anywhere.

> +	crashlog->info = &crashlog_type1_ver0;
>  
>  	header->access_type = GET_ACCESS(readl(disc_table));
>  	header->guid = readl(disc_table + GUID_OFFSET);
> 

-- 
 i.