On Wed, 21 May 2025, Ruhl, Michael J wrote: > >-----Original Message----- > >From: Ilpo Järvinen > >Sent: Monday, May 19, 2025 11:52 AM > >To: Ruhl, Michael J > >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 > >> --- > >> 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.