RE: [PATCH 4/4] platform/x86/intel/pmt: support BMG crashlog

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

[-- Attachment #1: Type: text/plain, Size: 20786 bytes --]

On Wed, 21 May 2025, Ruhl, Michael J wrote:

> >-----Original Message-----
> >From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >Sent: Monday, May 19, 2025 11:52 AM
> >To: Ruhl, Michael J <michael.j.ruhl@intel.com>
> >Cc: platform-driver-x86@vger.kernel.org
> >Subject: Re: [PATCH 4/4] platform/x86/intel/pmt: support BMG crashlog
> >
> >On Fri, 16 May 2025, Michael J. Ruhl wrote:
> >
> >> The Battlemage GPU has the type 1 version 2 crashlog
> >> feature.
> >>
> >> Update the crashlog driver to support this crashlog
> >> version.
> >
> >Too short lines.
> >
> >> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> >> ---
> >>  drivers/platform/x86/intel/pmt/class.h    |   2 +
> >>  drivers/platform/x86/intel/pmt/crashlog.c | 328 +++++++++++++++++++---
> >>  2 files changed, 288 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/intel/pmt/class.h
> >b/drivers/platform/x86/intel/pmt/class.h
> >> index 6b3455a86471..9c0c7e2efecf 100644
> >> --- a/drivers/platform/x86/intel/pmt/class.h
> >> +++ b/drivers/platform/x86/intel/pmt/class.h
> >> @@ -31,6 +31,8 @@ struct telem_endpoint {
> >>  };
> >>
> >>  struct intel_pmt_header {
> >> +	u32	type;
> >> +	u32	version;
> >>  	u32	base_offset;
> >>  	u32	size;
> >>  	u32	guid;
> >> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
> >b/drivers/platform/x86/intel/pmt/crashlog.c
> >> index c9bfe1c26311..700a51d2563a 100644
> >> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> >> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> >> @@ -23,10 +23,17 @@
> >>  #define CRASH_TYPE_OOBMSM	1
> >>
> >>  /* Crashlog Discovery Header */
> >> -#define CONTROL_OFFSET		0x0
> >> -#define GUID_OFFSET		0x4
> >> -#define BASE_OFFSET		0x8
> >> -#define SIZE_OFFSET		0xC
> >> +#define CONTROL_OFFSET		0x00
> >> +#define GUID_OFFSET		0x04
> >> +#define BASE_OFFSET		0x08
> >> +#define SIZE_OFFSET		0x0C
> >
> >Why did you change this group at all?
> 
> Was going for "consistency" on the byte definitions, but then apparently forgot in the
> following section (0x00)...
> 
> will remove the change.
> 
> >> +#define TYPE1_VER0_CONTROL_OFFSET	0x0
> >> +#define TYPE1_VER0_STATUS_OFFSET	0x0
> >> +
> >> +#define TYPE1_VER2_CONTROL_OFFSET	0x14
> >> +#define TYPE1_VER2_STATUS_OFFSET	0x0
> >> +
> >>  #define GET_ACCESS(v)		((v) & GENMASK(3, 0))
> >>  /* size is in bytes */
> >>  #define GET_SIZE(v)		((v) * sizeof(u32))
> >> @@ -56,9 +63,37 @@ struct type1_ver0_base {
> >>  	u32 complete:		1;  /* ro/v  31:31 */
> >>  };
> >>
> >> +struct type1_ver2_status {
> >> +	u32 access_type:	4;  /* ro    0:3  */
> >> +	u32 crash_type:		4;  /* ro    4:7  */
> >> +	u32 count:		8;  /* ro    8:15 */
> >> +	u32 version:		4;  /* ro   16:19 */
> >> +	u32 clear_support:	1;  /* ro   20:20 */
> >> +	u32 rsvd:		4;  /* ro   21:24 */
> >> +	u32 rearmed:		1;  /* ro   25:25 */
> >> +	u32 error:		1;  /* ro   26:26 */
> >> +	u32 consumed:		1;  /* ro   27:27 */
> >> +	u32 disable:		1;  /* ro   28:28 */
> >> +	u32 cleared:		1;  /* ro   29:29 */
> >> +	u32 in_progress:	1;  /* ro   30:30 */
> >> +	u32 complete:		1;  /* ro   31:31 */
> >
> >See, now you're adding GENMASK()/BIT() arguments into the comment when
> >the
> >struct/C bitfield doesn't document that for you. I think this conversion
> >away from GENMASK()/BIT() was a step backwards.
> >
> >> +};
> >> +
> >> +struct type1_ver2_control {
> >> +	u32 rsvd0:		25; /* ro    0:24 */
> >> +	u32 consumed:		1;  /* rw/v 25:25 */
> >> +	u32 rsvd1:		1;  /* ro/v 26:26 */
> >> +	u32 rsvd2:		1;  /* ro/v 27:27 */
> >
> >Why aren't these two combined?
> 
> There was a different bit defined there before (not rsvd)  I will combine.
> 
> >> +	u32 rearm:		1;  /* rw/v 28:28 */
> >> +	u32 manual:		1;  /* rw/v 29:29 */
> >> +	u32 clear:		1;  /* rw/v 30:30 */
> >> +	u32 disable:		1;  /* rw/v 31:31 */
> >
> >Really, they converted most bits into reserved (which is fine) but not
> >only that, they also relocated the remaining bits just because they
> >could. :-(
> 
> Yup, which is why I was having difficulty with the BIT() definitions...

I think I managed to get confused myself about control and status ones.

> >> +};
> >> +
> >>  struct crashlog_status {
> >>  	union {
> >>  		struct type1_ver0_base stat;
> >> +		struct type1_ver2_status stat2;
> >>  		u32 status;
> >>  	};
> >>  };
> >> @@ -66,6 +101,7 @@ struct crashlog_status {
> >>  struct crashlog_control {
> >>  	union {
> >>  		struct type1_ver0_base ctrl;
> >> +		struct type1_ver2_control ctrl2;
> >>  		u32 control;
> >>  	};
> >>  };
> >> @@ -75,97 +111,174 @@ struct pmt_crashlog_priv {
> >>  	struct crashlog_entry	entry[];
> >>  };
> >>
> >> +static u32 get_control_offset(struct intel_pmt_header *hdr)
> >> +{
> >> +	return hdr->version == 0 ? TYPE1_VER0_CONTROL_OFFSET :
> >TYPE1_VER2_CONTROL_OFFSET;
> >> +}
> >> +
> >> +static u32 get_status_offset(struct intel_pmt_header *hdr)
> >> +{
> >> +	return hdr->version == 0 ? TYPE1_VER0_STATUS_OFFSET :
> >TYPE1_VER2_STATUS_OFFSET;
> >> +}
> >
> >I suggest you create some per version const version info struct which
> >holds all the relevant offsets, field bitmasks that got changed, and
> >attrs (perhaps more, the code is complex enough I didn't try to
> >understand evenything until it look cleaner). Basically, whenever you want
> >to check what the version is and behave differently, consider if you can
> >put that somehow into the per version info struct without needed to use
> >if () / elvis op logic at all.
> 
> So something like this:
> 
> struct crashlog_offset {
> 	int disabled;
> 	int cleared;
> 	int manual;
> 	int complete;
> 	int rearmed;
> 	int error;
> 	int consumed;
> 	int in_progress;
> 	int set_consumed;
> 	int set_rearm;
> 	int set_manual;
> 	int set_clear;
> 	int set_disable;

Use u32 and put the mask/bit into them. But only put the ones which are 
really different, not the ones which are the same.

> } offset;
> 
> if (version == 0) {
> 	offset.disabled(28);
> 	offset.set_disabled = BIT(28);

Not in the function code at all, you construct them like:

const struct crahslog_offset *crashlog_v0 = {
	.control_offset = TYPE1_VER0_CONTROL_OFFSET,
	...
	.control_disable = CRASHLOG_FLAG_VER0_CONTROL_DISABLE,
};

Then, at init time, save pointer to crashlog_v0 or crashlog_v2 somewhere 
so you don't need any ifs in the other functions as you can just deref
the correct one.

I suggest you consider removing low-value chars from those define names 
such as "FLAG", and shortening CONTROL to CTRL, VER -> V, etc. to control 
the horizontal space better.

> }
> if (version == 2) {
> 	offset.disabled = BIT(28);
> 	offset.set_disabled = BIT(31);
> }
> 
> ?  (or did you have a different example?)
> 
> I am not sure I am following what you would like to see here...
> 
> >Also now, after reading this patch, I'm even more convinced you want to
> >keep using BIT/GENMASK(), not C bitfields because the former just happens
> >to be more flexible allowing the mask values to be easily put into the
> >info struct.
> >
> >>  /*
> >>   * I/O
> >>   */
> >>  static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
> >>  {
> >> +	u32 offset = get_status_offset(&entry->header);
> >>  	struct crashlog_status status = {
> >> -		.status = readl(entry->disc_table + CONTROL_OFFSET),
> >> +		.status = readl(entry->disc_table + offset),
> >>  	};
> >>
> >>  	/* return current value of the crashlog complete flag */
> >> -	return status.stat.complete;
> >> +	if (entry->header.version == 0)
> >> +		return status.stat.complete;
> >>
> >> +	return status.stat2.complete;
> >>  }
> >>
> >>  static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry)
> >>  {
> >> +	u32 offset = get_status_offset(&entry->header);
> >>  	struct crashlog_status status = {
> >> -		.status = readl(entry->disc_table + CONTROL_OFFSET),
> >> +		.status = readl(entry->disc_table + offset),
> >>  	};
> >>
> >>  	/* return current value of the crashlog disabled flag */
> >> -	return status.stat.disable;
> >> +	if (entry->header.version == 0)
> >> +		return status.stat.disable;
> >> +
> >> +	return status.stat2.disable;
> >>  }
> >>
> >> -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)
> >>  {
> >>  	struct crashlog_control discovery_header = {
> >>  		.control = readl(entry->disc_table + CONTROL_OFFSET),
> >>  	};
> >> -	u32 crash_type, version;
> >>
> >> -	crash_type = discovery_header.ctrl.crash_type;
> >> -	version = discovery_header.ctrl.version;
> >> +	*crash_type = discovery_header.ctrl.crash_type;
> >> +	*version = discovery_header.ctrl.version;
> >>
> >>  	/*
> >> -	 * 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;
> >>  }
> >>
> >>  static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
> >>  				     bool disable)
> >>  {
> >> +	u32 offset = get_control_offset(&entry->header);
> >>  	struct crashlog_control control = {
> >> -		.control = readl(entry->disc_table + CONTROL_OFFSET),
> >> +		.control = readl(entry->disc_table + offset),
> >>  	};
> >>
> >> -	/* clear trigger bits so we are only modifying disable flag */
> >> -	control.ctrl.clear = 0;
> >> -	control.ctrl.manual = 0;
> >> -	control.ctrl.complete = 0;
> >> +	if (entry->header.version == 0) {
> >> +		/* clear trigger bits so we are only modifying disable flag */
> >> +		control.ctrl.clear = 0;
> >> +		control.ctrl.manual = 0;
> >> +		control.ctrl.complete = 0;
> >>
> >> -	if (disable)
> >> -		control.ctrl.disable = 1;
> >> -	else
> >> -		control.ctrl.disable = 0;
> >> +		control.ctrl.disable = disable;
> >> +	} else {
> >> +		control.ctrl2.manual = 0;
> >> +		control.ctrl2.clear = 0;
> >>
> >> -	writel(control.control, entry->disc_table + CONTROL_OFFSET);
> >> +		control.ctrl2.disable = disable;
> >> +	}
> >> +
> >> +	writel(control.control, entry->disc_table + offset);
> >>  }
> >>
> >>  static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
> >>  {
> >> +	u32 offset = get_control_offset(&entry->header);
> >>  	struct crashlog_control control = {
> >> -		.control = readl(entry->disc_table + CONTROL_OFFSET),
> >> +		.control = readl(entry->disc_table + offset),
> >>  	};
> >>
> >> -	/* clear trigger bits so we are only modifying disable flag */
> >> -	control.ctrl.disable = 0;
> >> -	control.ctrl.manual = 0;
> >> -	control.ctrl.complete = 0;
> >> +	if (entry->header.version == 0) {
> >> +		/* clear trigger bits so we are only modifying disable flag */
> >> +		control.ctrl.disable = 0;
> >> +		control.ctrl.manual = 0;
> >> +		control.ctrl.complete = 0;
> >> +
> >> +		control.ctrl.clear = 1;
> >> +	} else {
> >> +		control.ctrl2.disable = 0;
> >> +		control.ctrl2.manual = 0;
> >>
> >> -	control.ctrl.clear = 1;
> >> +		control.ctrl2.clear = 1;
> >> +	}
> >>
> >> -	writel(control.control, entry->disc_table + CONTROL_OFFSET);
> >> +	writel(control.control, entry->disc_table + offset);
> >>  }
> >>
> >>  static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
> >>  {
> >> +	u32 offset = get_control_offset(&entry->header);
> >>  	struct crashlog_control control = {
> >> -		.control = readl(entry->disc_table + CONTROL_OFFSET),
> >> +		.control = readl(entry->disc_table + offset),
> >> +	};
> >> +
> >> +	if (entry->header.version == 0) {
> >> +		/* clear trigger bits so we are only modifying disable flag */
> >> +		control.ctrl.disable = 0;
> >> +		control.ctrl.clear = 0;
> >> +		control.ctrl.complete = 0;
> >> +
> >> +		control.ctrl.manual = 1;
> >> +	} else {
> >> +		control.ctrl2.disable = 0;
> >> +		control.ctrl2.clear = 0;
> >> +
> >> +		control.ctrl2.manual = 1;
> >> +	}
> >> +
> >> +	writel(control.control, entry->disc_table + offset);
> >> +}
> >> +
> >> +/* version 2 support */
> >> +static void pmt_crashlog_set_consumed(struct intel_pmt_entry *entry)
> >> +{
> >> +	u32 offset = get_control_offset(&entry->header);
> >> +	struct crashlog_control control = {
> >> +		.control = readl(entry->disc_table + offset),
> >>  	};
> >>
> >> -	/* clear trigger bits so we are only modifying disable flag */
> >> -	control.ctrl.disable = 0;
> >> -	control.ctrl.clear = 0;
> >> -	control.ctrl.complete = 0;
> >> +	control.ctrl2.consumed = 1;
> >> +
> >> +	writel(control.control, entry->disc_table + offset);
> >> +}
> >>
> >> -	control.ctrl.manual = 1;
> >> +static bool pmt_crashlog_rearm(struct intel_pmt_entry *entry)
> >> +{
> >> +	u32 offset = get_status_offset(&entry->header);
> >> +	struct crashlog_status status = {
> >> +		.status = readl(entry->disc_table + offset),
> >> +	};
> >>
> >> -	writel(control.control, entry->disc_table + CONTROL_OFFSET);
> >> +	return status.stat2.rearmed;
> >> +}
> >> +
> >> +static void pmt_crashlog_set_rearm(struct intel_pmt_entry *entry)
> >> +{
> >> +	u32 offset = get_control_offset(&entry->header);
> >> +	struct crashlog_control control = {
> >> +		.control = readl(entry->disc_table + offset),
> >> +	};
> >> +
> >> +	control.ctrl2.rearm = 1;
> >> +
> >> +	writel(control.control, entry->disc_table + offset);
> >>  }
> >>
> >>  /*
> >> @@ -177,7 +290,7 @@ 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);
> >>
> >> -	return sprintf(buf, "%d\n", enabled);
> >> +	return sysfs_emit(buf, "%d\n", enabled);
> >>  }
> >>
> >>  static ssize_t
> >> @@ -251,16 +364,135 @@ trigger_store(struct device *dev, struct
> >device_attribute *attr,
> >>  }
> >>  static DEVICE_ATTR_RW(trigger);
> >>
> >> +static ssize_t consumed_store(struct device *dev, struct device_attribute
> >*attr,
> >> +			      const char *buf, size_t count)
> >> +{
> >> +	struct crashlog_entry *entry;
> >> +	bool consumed;
> >> +	int result;
> >> +
> >> +	entry = dev_get_drvdata(dev);
> >> +
> >> +	result = kstrtobool(buf, &consumed);
> >> +	if (result)
> >> +		return result;
> >> +
> >> +	/* set bit only */
> >> +	if (!consumed)
> >> +		return -EINVAL;
> >> +
> >> +	mutex_lock(&entry->control_mutex);
> >
> >Please use guard() and remove the gotos.
> 
> Will do.  
> 
> Thank you for your comments!
> 
> Mike
> 
> >> +
> >> +	if (pmt_crashlog_disabled(&entry->entry)) {
> >> +		result = -EBUSY;
> >> +		goto err;
> >> +	} else if (!pmt_crashlog_complete(&entry->entry)) {
> >> +		result = -EEXIST;
> >> +		goto err;
> >> +	} else {
> >> +		pmt_crashlog_set_consumed(&entry->entry);
> >> +	}
> >> +
> >> +err:
> >> +	mutex_unlock(&entry->control_mutex);
> >> +	return count;
> >> +}
> >> +static DEVICE_ATTR_WO(consumed);
> >> +
> >> +static ssize_t
> >> +rearm_show(struct device *dev, struct device_attribute *attr, char *buf)
> >> +{
> >> +	struct intel_pmt_entry *entry = dev_get_drvdata(dev);
> >> +	int rearmed = pmt_crashlog_rearm(entry);
> >> +
> >> +	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 *entry;
> >> +	bool trigger;
> >> +	int result;
> >> +
> >> +	entry = dev_get_drvdata(dev);
> >> +
> >> +	result = kstrtobool(buf, &trigger);
> >> +	if (result)
> >> +		return result;
> >> +
> >> +	/* set only */
> >> +	if (!trigger)
> >> +		return -EINVAL;
> >> +
> >> +	mutex_lock(&entry->control_mutex);
> >> +	pmt_crashlog_set_rearm(&entry->entry);
> >> +	mutex_unlock(&entry->control_mutex);
> >> +
> >> +	return count;
> >> +}
> >> +static DEVICE_ATTR_RW(rearm);
> >> +
> >> +#define DEBUG_REGISTER_INFO
> >> +#ifdef DEBUG_REGISTER_INFO
> >> +static ssize_t
> >> +status_show(struct device *dev, struct device_attribute *attr, char *buf)
> >> +{
> >> +       struct intel_pmt_entry *entry = dev_get_drvdata(dev);
> >> +       u32 sts_off = get_status_offset(&entry->header);
> >> +       u32 ctl_off = get_control_offset(&entry->header);
> >> +       struct crashlog_status status = {
> >> +               .status = readl(entry->disc_table + sts_off),
> >> +       };
> >> +       struct crashlog_control control = {
> >> +               .control = readl(entry->disc_table + ctl_off),
> >> +       };
> >> +       int len = 0;
> >> +
> >> +       len += sysfs_emit_at(buf, len, "clear_support: %d\n",
> >status.stat2.clear_support);
> >> +       len += sysfs_emit_at(buf, len, "rearmed: %d\n", status.stat2.rearmed);
> >> +       len += sysfs_emit_at(buf, len, "error: %d\n", status.stat2.error);
> >> +       len += sysfs_emit_at(buf, len, "consumed: %d\n",
> >status.stat2.consumed);
> >> +       len += sysfs_emit_at(buf, len, "disable: %d\n", status.stat2.disable);
> >> +       len += sysfs_emit_at(buf, len, "cleared: %d\n", status.stat2.cleared);
> >> +       len += sysfs_emit_at(buf, len, "in_progress: %d\n",
> >status.stat2.in_progress);
> >> +       len += sysfs_emit_at(buf, len, "complete: %d\n", status.stat2.complete);
> >> +       len += sysfs_emit_at(buf, len, "sts_off: 0x%02x  ctl_off: 0x%02x\n",
> >sts_off, ctl_off);
> >> +       len += sysfs_emit_at(buf, len, "status:  0x%08x\n", status.status);
> >> +       len += sysfs_emit_at(buf, len, "control: 0x%08x\n", control.control);
> >> +
> >> +       return len;
> >> +}
> >> +static DEVICE_ATTR_RO(status);
> >> +#endif
> >> +
> >>  static struct attribute *pmt_crashlog_attrs[] = {
> >>  	&dev_attr_enable.attr,
> >>  	&dev_attr_trigger.attr,
> >>  	NULL
> >>  };
> >>
> >> +static struct attribute *pmt_crashlog_ver2_attrs[] = {
> >> +	&dev_attr_enable.attr,
> >> +	&dev_attr_trigger.attr,
> >> +	&dev_attr_consumed.attr,
> >> +	&dev_attr_rearm.attr,
> >> +#ifdef DEBUG_REGISTER_INFO
> >> +	&dev_attr_status.attr,
> >> +#endif
> >
> >You could use a define to hold this so you don't need to use ifdef here.
> >Check WMAX_DEV_GROUPS if you want an example what I mean.
> >
> >> +	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,
> >> +};
> >> +
> >> +static int pmt_crashlog_header_decode(struct intel_pmt_entry *entry, struct
> >device *dev);
> >> +
> >>  static int pmt_crashlog_add_endpoint(struct intel_vsec_device *ivdev,
> >>  				     struct intel_pmt_entry *entry)
> >>  {
> >> @@ -286,14 +518,24 @@ static int pmt_crashlog_add_endpoint(struct
> >intel_vsec_device *ivdev,
> >>  	return 0;
> >>  }
> >>
> >> +static const struct attribute_group *select_sysfs_grp(struct
> >intel_pmt_header *hdr)
> >> +{
> >> +	if (hdr->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 control mutex */
> >> @@ -303,11 +545,13 @@ static int pmt_crashlog_header_decode(struct
> >intel_pmt_entry *entry,
> >>  	header->access_type = GET_ACCESS(readl(disc_table));
> >>  	header->guid = readl(disc_table + GUID_OFFSET);
> >>  	header->base_offset = readl(disc_table + BASE_OFFSET);
> >> +	header->type = type;
> >> +	header->version = version;
> >>
> >>  	/* 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(header);
> >>
> >>  	return 0;
> >>  }
> >>
> >
> >--
> > i.
> 

-- 
 i.