[PATCH 07/10] platform/x86/intel/pmt: use a version struct
Ilpo Järvinen
ilpo.jarvinen at linux.intel.com
Tue Jun 3 07:06:28 UTC 2025
On Mon, 2 Jun 2025, Ruhl, Michael J wrote:
> >-----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.
This look okay to me.
--
i.
> >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