Re: [PATCH v3 11/11] platform/x86/intel/pmt: support BMG crashlog

["David E. Box" <david.e.box@linux.intel.com>]

On Thu, 2025-06-05 at 14:44 -0400, Michael J. Ruhl wrote:
> The Battlemage GPU has the type 1 version 2 crashlog feature.
> 
> Update the crashlog driver to support this crashlog version.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 283 ++++++++++++++++++++--
>  1 file changed, 264 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
> b/drivers/platform/x86/intel/pmt/crashlog.c
> index fe6563721886..0fb60036a9bb 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -51,20 +51,53 @@
>  #define TYPE1_VER0_COMPLETE		BIT(31)
>  #define TYPE1_VER0_TRIGGER_MASK		GENMASK(31, 28)
>  
> +/*
> + * Type 1 Version 2
> + * status and control are two different registers
> + */
> +#define TYPE1_VER2_STATUS_OFFSET	0x00
> +#define TYPE1_VER2_CONTROL_OFFSET	0x14
> +
> +/* status register */
> +#define TYPE1_VER2_CLEAR_SUPPORT	BIT(20)
> +#define TYPE1_VER2_REARMED		BIT(25)
> +#define TYPE1_VER2_ERROR		BIT(26)
> +#define TYPE1_VER2_CONSUMED		BIT(27)
> +#define TYPE1_VER2_DISABLED		BIT(28)
> +#define TYPE1_VER2_CLEARED		BIT(29)
> +#define TYPE1_VER2_IN_PROGRESS		BIT(30)
> +#define TYPE1_VER2_COMPLETE		BIT(31)
> +
> +/* control register */
> +#define TYPE1_VER2_CONSUME		BIT(25)
> +#define TYPE1_VER2_REARM		BIT(28)
> +#define TYPE1_VER2_EXECUTE		BIT(29)
> +#define TYPE1_VER2_CLEAR		BIT(30)
> +#define TYPE1_VER2_DISABLE		BIT(31)
> +#define TYPE1_VER2_TRIGGER_MASK		(TYPE1_VER2_CONSUME |
> TYPE1_VER2_EXECUTE | \
> +					 TYPE1_VER2_CLEAR |
> TYPE1_VER2_DISABLE)
> +
>  /* After offset, order alphabetically, not bit ordered */
>  struct crashlog_status {
>  	u32 offset;
> -	u32 clear;
> +	u32 clear_supported;
> +	u32 cleared;
>  	u32 complete;
> -	u32 disable;
> +	u32 consumed;
> +	u32 disabled;
> +	u32 error;
> +	u32 in_progress;
> +	u32 rearmed;
>  };

Since this struct was just introduced, better to use the intended names in the
original patch to avoid unnecessary churn.

>  
>  struct crashlog_control {
>  	u32 offset;
>  	u32 trigger_mask;
>  	u32 clear;
> +	u32 consume;
>  	u32 disable;
>  	u32 manual;
> +	u32 rearm;
>  };
>  
>  struct crashlog_info {
> @@ -74,9 +107,9 @@ struct crashlog_info {
>  
>  const struct crashlog_info crashlog_type1_ver0 = {
>  	.status.offset = TYPE1_VER0_STATUS_OFFSET,
> -	.status.clear = TYPE1_VER0_CLEAR,
> +	.status.cleared = TYPE1_VER0_CLEAR,
>  	.status.complete = TYPE1_VER0_COMPLETE,
> -	.status.disable = TYPE1_VER0_DISABLE,
> +	.status.disabled = TYPE1_VER0_DISABLE,
>  
>  	.control.offset = TYPE1_VER0_CONTROL_OFFSET,
>  	.control.trigger_mask = TYPE1_VER0_TRIGGER_MASK,
> @@ -85,6 +118,26 @@ const struct crashlog_info crashlog_type1_ver0 = {
>  	.control.manual = TYPE1_VER0_EXECUTE,
>  };
>  
> +const struct crashlog_info crashlog_type1_ver2 = {
> +	.status.offset = TYPE1_VER2_STATUS_OFFSET,
> +	.status.clear_supported = TYPE1_VER2_CLEAR_SUPPORT,
> +	.status.cleared = TYPE1_VER2_CLEARED,
> +	.status.complete = TYPE1_VER2_COMPLETE,
> +	.status.consumed = TYPE1_VER2_CONSUMED,
> +	.status.disabled = TYPE1_VER2_DISABLED,
> +	.status.error = TYPE1_VER2_ERROR,
> +	.status.in_progress = TYPE1_VER2_IN_PROGRESS,
> +	.status.rearmed = TYPE1_VER2_REARMED,
> +
> +	.control.offset = TYPE1_VER2_CONTROL_OFFSET,
> +	.control.trigger_mask = TYPE1_VER2_TRIGGER_MASK,
> +	.control.clear = TYPE1_VER2_CLEAR,
> +	.control.consume = TYPE1_VER2_CONSUME,
> +	.control.disable = TYPE1_VER2_DISABLE,
> +	.control.manual = TYPE1_VER2_EXECUTE,
> +	.control.rearm = TYPE1_VER2_REARM,
> +};
> +
>  struct crashlog_entry {
>  	/* entry must be first member of struct */
>  	struct intel_pmt_entry		entry;
> @@ -99,30 +152,35 @@ struct pmt_crashlog_priv {
>  
>  /*
>   * This is the generic access to a PMT struct. So the use of
> - * struct crashlog_entry
> - * doesn't "make sense" here.
> + *   struct crashlog_entry
> + * doesn't "make sense" here, i.e. use:
> + *   struct intel_pmt_entry
>   */

I don't think this comment is needed. Still, this could have been done in the
patch that added it.

> -static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
> +static bool pmt_crashlog_supported(struct intel_pmt_entry *entry, u32
> *crash_type, u32 *version)
>  {
>  	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
> -	u32 crash_type, version;
>  
> -	crash_type = GET_TYPE(discovery_header);
> -	version = GET_VERSION(discovery_header);
> +	*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.
> +	 * Currently we only recognize OOBMSM (type 1) and version 0 or 2
> +	 * devices.
> +	 *
> +	 * Ignore all other crashlog devices in the system.
>  	 */
> -	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
> +	if (*crash_type == CRASH_TYPE_OOBMSM && (*version == 0 || *version ==
> 2))
> +		return true;
> +
> +	return false;
>  }
>  
>  /*
>   * I/O
>   */
>  
> -#define SET     true
> -#define CLEAR   false
> +#define SET	true
> +#define CLEAR	false

Ditto

>  
>  static void read_modify_write(struct crashlog_entry *crashlog, u32 bit, bool
> set)
>  {
> @@ -157,7 +215,7 @@ static bool pmt_crashlog_complete(struct crashlog_entry
> *crashlog)
>  static bool pmt_crashlog_disabled(struct crashlog_entry *crashlog)
>  {
>  	/* return current value of the crashlog disabled flag */
> -	return read_check(crashlog, crashlog->info->status.disable);
> +	return read_check(crashlog, crashlog->info->status.disabled);
>  }
>  
>  static void pmt_crashlog_set_disable(struct crashlog_entry *crashlog, bool
> disable)
> @@ -175,9 +233,119 @@ static void pmt_crashlog_set_execute(struct
> crashlog_entry *crashlog)
>  	read_modify_write(crashlog, crashlog->info->control.manual, SET);
>  }
>  
> +/* version 2 support */
> +static bool pmt_crashlog_cleared(struct crashlog_entry *crashlog)
> +{
> +	/* return current value of the crashlog cleared flag */
> +	return read_check(crashlog, crashlog->info->status.cleared);
> +}
> +
> +static bool pmt_crashlog_consumed(struct crashlog_entry *crashlog)
> +{
> +	/* return current value of the crashlog consumedflag */
> +	return read_check(crashlog, crashlog->info->status.consumed);
> +}
> +
> +static void pmt_crashlog_set_consumed(struct crashlog_entry *crashlog)
> +{
> +	read_modify_write(crashlog, crashlog->info->control.consume, SET);
> +}
> +
> +static bool pmt_crashlog_error(struct crashlog_entry *crashlog)
> +{
> +	/* return current value of the crashlog error flag */
> +	return read_check(crashlog, crashlog->info->status.error);
> +}
> +
> +static bool pmt_crashlog_rearm(struct crashlog_entry *crashlog)
> +{
> +	/* return current value of the crashlog reamed flag */
> +	return read_check(crashlog, crashlog->info->status.rearmed);
> +}
> +
> +static void pmt_crashlog_set_rearm(struct crashlog_entry *crashlog)
> +{
> +	read_modify_write(crashlog, crashlog->info->control.rearm, SET);
> +}
> +
>  /*
>   * sysfs
>   */
> +static ssize_t
> +clear_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> +	bool cleared = pmt_crashlog_cleared(crashlog);
> +
> +	return sysfs_emit(buf, "%d\n", cleared);
> +}
> +
> +static ssize_t
> +clear_store(struct device *dev, struct device_attribute *attr,
> +	    const char *buf, size_t count)
> +{
> +	struct crashlog_entry *crashlog;
> +	bool clear;
> +	int result;
> +
> +	crashlog = dev_get_drvdata(dev);
> +
> +	result = kstrtobool(buf, &clear);
> +	if (result)
> +		return result;
> +
> +	/* set bit only */
> +	if (!clear)
> +		return -EINVAL;
> +
> +	guard(mutex)(&crashlog->control_mutex);
> +
> +	pmt_crashlog_set_clear(crashlog);
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(clear);
> +
> +static ssize_t
> +consumed_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> +	bool consumed = pmt_crashlog_consumed(crashlog);
> +
> +	return sysfs_emit(buf, "%d\n", consumed);
> +}
> +
> +static ssize_t consumed_store(struct device *dev, struct device_attribute
> *attr,
> +			      const char *buf, size_t count)
> +{
> +	struct crashlog_entry *crashlog;
> +	bool consumed;
> +	int result;
> +
> +	crashlog = dev_get_drvdata(dev);
> +
> +	result = kstrtobool(buf, &consumed);
> +	if (result)
> +		return result;
> +
> +	/* set bit only */
> +	if (!consumed)
> +		return -EINVAL;
> +
> +	guard(mutex)(&crashlog->control_mutex);
> +
> +	if (pmt_crashlog_disabled(crashlog))
> +		return -EBUSY;
> +
> +	if (!pmt_crashlog_complete(crashlog))
> +		return -EEXIST;
> +
> +	pmt_crashlog_set_consumed(crashlog);
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(consumed);
> +
>  static ssize_t
>  enable_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> @@ -209,6 +377,50 @@ enable_store(struct device *dev, struct device_attribute
> *attr,
>  }
>  static DEVICE_ATTR_RW(enable);
>  
> +static ssize_t
> +error_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> +	bool error = pmt_crashlog_error(crashlog);
> +
> +	return sysfs_emit(buf, "%d\n", error);
> +}
> +static DEVICE_ATTR_RO(error);
> +
> +static ssize_t
> +rearm_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> +	int rearmed = pmt_crashlog_rearm(crashlog);
> +
> +	return sysfs_emit(buf, "%d\n", rearmed);
> +}
> +
> +static ssize_t rearm_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	struct crashlog_entry *crashlog;
> +	bool rearm;
> +	int result;
> +
> +	crashlog = dev_get_drvdata(dev);
> +
> +	result = kstrtobool(buf, &rearm);
> +	if (result)
> +		return result;
> +
> +	/* set only */
> +	if (!rearm)
> +		return -EINVAL;
> +
> +	guard(mutex)(&crashlog->control_mutex);
> +
> +	pmt_crashlog_set_rearm(crashlog);
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(rearm);
> +
>  static ssize_t
>  trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> @@ -263,24 +475,57 @@ static struct attribute *pmt_crashlog_attrs[] = {
>  	NULL
>  };
> 

For what follows ...

>  
> +static struct attribute *pmt_crashlog_ver2_attrs[] = {
> +	&dev_attr_clear.attr,
> +	&dev_attr_consumed.attr,
> +	&dev_attr_enable.attr,
> +	&dev_attr_error.attr,
> +	&dev_attr_rearm.attr,
> +	&dev_attr_trigger.attr,
> +	NULL
> +};
> +
>  static const struct attribute_group pmt_crashlog_group = {
>  	.attrs	= pmt_crashlog_attrs,
>  };
>  
> +static const struct attribute_group pmt_crashlog_ver2_group = {
> +	.attrs = pmt_crashlog_ver2_attrs,
> +};

... I would have the group and attributes both reflect type and version in the
name. Update pmt_crashlog_group as well.

David

> +
> +static const struct crashlog_info *select_crashlog_info(u32 type, u32
> version)
> +{
> +	if (version == 0)
> +		return &crashlog_type1_ver0;
> +
> +	return &crashlog_type1_ver2;
> +}
> +
> +static const struct attribute_group *select_sysfs_grp(u32 type, u32 version)
> +{
> +	if (version == 0)
> +		return &pmt_crashlog_group;
> +
> +	return &pmt_crashlog_ver2_group;
> +}
> +
>  static int pmt_crashlog_header_decode(struct intel_pmt_entry *entry,
>  				      struct device *dev)
>  {
>  	void __iomem *disc_table = entry->disc_table;
>  	struct intel_pmt_header *header = &entry->header;
>  	struct crashlog_entry *crashlog;
> +	u32 version;
> +	u32 type;
>  
> -	if (!pmt_crashlog_supported(entry))
> +	if (!pmt_crashlog_supported(entry, &type, &version))
>  		return 1;
>  
>  	/* initialize the crashlog struct */
>  	crashlog = container_of(entry, struct crashlog_entry, entry);
>  	mutex_init(&crashlog->control_mutex);
> -	crashlog->info = &crashlog_type1_ver0;
> +
> +	crashlog->info = select_crashlog_info(type, version);
>  
>  	header->access_type = GET_ACCESS(readl(disc_table));
>  	header->guid = readl(disc_table + GUID_OFFSET);
> @@ -289,7 +534,7 @@ static int pmt_crashlog_header_decode(struct
> intel_pmt_entry *entry,
>  	/* Size is measured in DWORDS, but accessor returns bytes */
>  	header->size = GET_SIZE(readl(disc_table + SIZE_OFFSET));
>  
> -	entry->attr_grp = &pmt_crashlog_group;
> +	entry->attr_grp = select_sysfs_grp(type, version);
>  
>  	return 0;
>  }