[PATCH 0/4] Crashlog Type1 Version2 support

[PATCH 0/4] Crashlog Type1 Version2 support

[Michael J. Ruhl]

The Intel BMG GPU device supports the crashlog feature,
which was exposed in an Xe driver patch
(drm/xe/vsec: Support BMG devices), however the version
of crashlog used by the BMG GPU does not have a supporing
PMT driver.

Update the PMT crashlog driver to support the BMG crashlog
feature.

Michael J. Ruhl (4):
  platform/x86/intel/pmt: crashlog binary file endpoint
  platform/x86/intel/pmt: update to bit access
  platform/x86/intel/pmt: decouple sysfs and namespace
  platform/x86/intel/pmt: support BMG crashlog

 drivers/platform/x86/intel/pmt/class.c    |  12 +-
 drivers/platform/x86/intel/pmt/class.h    |   4 +-
 drivers/platform/x86/intel/pmt/crashlog.c | 429 +++++++++++++++++++---
 3 files changed, 382 insertions(+), 63 deletions(-)

-- 
2.49.0

[PATCH 1/4] platform/x86/intel/pmt: crashlog binary file endpoint

[Michael J. Ruhl]

The export API added a requirement for end point data to be
used by the intel_pmt_read() function to access mmio data.

Without the ep, the call causes a NULL pointer exception.

BUG: kernel NULL pointer dereference, address: 0000000000000000
PGD 0 P4D 0
Oops: Oops: 0000 [#1] SMP NOPTI
CPU: 12 UID: 0 PID: 5721 Comm: cat Tainted: G           OE       6.15.0-rc4+ #3 PREEMPT(voluntary)
Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
Hardware name: ASUS System Product Name/PRIME Z790-P WIFI, BIOS 1641 02/21/2024
RIP: 0010:intel_pmt_read+0x3b/0x70 [pmt_class]
Code:
RSP: 0018:ffffb19981ebba18 EFLAGS: 00010246
RAX: ffffffffc0ef8e08 RBX: 0000000000000800 RCX: 0000000000000800
RDX: ffff99aee03af450 RSI: ffff99ae86552000 RDI: 0000000000000000
RBP: ffffb19981ebba58 R08: 0000000000000000 R09: 0000000000000800
R10: 000000000e2f8200 R11: 0000000000000000 R12: 0000000000000000
R13: ffff99aee03af450 R14: ffff99ae8a4bbc00 R15: ffff99ae86a35a40
FS:  00007f097dd88740(0000) GS:ffff99b62fbe8000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000198860005 CR4: 0000000000f72ef0
PKRU: 55555554
Call Trace:
 <TASK>
 ? sysfs_kf_bin_read+0xc0/0xe0
 kernfs_fop_read_iter+0xac/0x1a0
 vfs_read+0x26d/0x350
 ksys_read+0x6b/0xe0
 __x64_sys_read+0x1d/0x30
 x64_sys_call+0x1bc8/0x1d70
 do_syscall_64+0x6d/0x110
 ? __mod_memcg_lruvec_state+0xe7/0x240
 ? __lruvec_stat_mod_folio+0x8f/0xe0
 ? set_ptes.isra.0+0x3b/0x80
 ? do_anonymous_page+0x101/0x9c0
 ? ___pte_offset_map+0x20/0x180
 ? __handle_mm_fault+0xba3/0x1010
 ? __count_memcg_events+0xca/0x190
 ? count_memcg_events.constprop.0+0x1e/0x40
 ? handle_mm_fault+0x1a8/0x2b0
 ? do_user_addr_fault+0x2f6/0x7b0
 ? irqentry_exit_to_user_mode+0x33/0x170
 ? irqentry_exit+0x3f/0x50
 ? exc_page_fault+0x94/0x1b0
 entry_SYSCALL_64_after_hwframe+0x76/0x7e
RIP: 0033:0x7f097db25701

Add the endpoint information to the crashlog driver to avoid
the NULL pointer exception.

Two minor white space issues are addressed as well.

Fixes: 416eeb2e1fc7 ("platform/x86/intel/pmt: telemetry: Export API to read telemetry")
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/crashlog.c | 38 ++++++++++++++++++++---
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 6a9eb3c4b313..952bfe341f53 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -143,7 +143,7 @@ enable_show(struct device *dev, struct device_attribute *attr, char *buf)
 
 static ssize_t
 enable_store(struct device *dev, struct device_attribute *attr,
-	    const char *buf, size_t count)
+	     const char *buf, size_t count)
 {
 	struct crashlog_entry *entry;
 	bool enabled;
@@ -177,7 +177,7 @@ trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
 
 static ssize_t
 trigger_store(struct device *dev, struct device_attribute *attr,
-	    const char *buf, size_t count)
+	      const char *buf, size_t count)
 {
 	struct crashlog_entry *entry;
 	bool trigger;
@@ -222,6 +222,31 @@ static const struct attribute_group pmt_crashlog_group = {
 	.attrs	= pmt_crashlog_attrs,
 };
 
+static int pmt_crashlog_add_endpoint(struct intel_vsec_device *ivdev,
+				     struct intel_pmt_entry *entry)
+{
+	struct telem_endpoint *ep;
+
+	/* Endpoint lifetimes are managed by kref, not devres */
+	entry->ep = kzalloc(sizeof(*entry->ep), GFP_KERNEL);
+	if (!entry->ep)
+		return -ENOMEM;
+
+	ep = entry->ep;
+	ep->pcidev = ivdev->pcidev;
+	ep->header.access_type = entry->header.access_type;
+	ep->header.guid = entry->header.guid;
+	ep->header.base_offset = entry->header.base_offset;
+	ep->header.size = entry->header.size;
+	ep->base = entry->base;
+	ep->present = true;
+	ep->cb = ivdev->priv_data;
+
+	kref_init(&ep->kref);
+
+	return 0;
+}
+
 static int pmt_crashlog_header_decode(struct intel_pmt_entry *entry,
 				      struct device *dev)
 {
@@ -252,6 +277,7 @@ static struct intel_pmt_namespace pmt_crashlog_ns = {
 	.xa = &crashlog_array,
 	.attr_grp = &pmt_crashlog_group,
 	.pmt_header_decode = pmt_crashlog_header_decode,
+	.pmt_add_endpoint = pmt_crashlog_add_endpoint,
 };
 
 /*
@@ -262,8 +288,12 @@ static void pmt_crashlog_remove(struct auxiliary_device *auxdev)
 	struct pmt_crashlog_priv *priv = auxiliary_get_drvdata(auxdev);
 	int i;
 
-	for (i = 0; i < priv->num_entries; i++)
-		intel_pmt_dev_destroy(&priv->entry[i].entry, &pmt_crashlog_ns);
+	for (i = 0; i < priv->num_entries; i++) {
+		struct intel_pmt_entry *entry = &priv->entry[i].entry;
+
+		kfree(entry->ep);
+		intel_pmt_dev_destroy(entry, &pmt_crashlog_ns);
+	}
 }
 
 static int pmt_crashlog_probe(struct auxiliary_device *auxdev,
-- 
2.49.0

Re: [PATCH 1/4] platform/x86/intel/pmt: crashlog binary file endpoint

[Ilpo Järvinen]

On Fri, 16 May 2025, Michael J. Ruhl wrote:

> The export API added a requirement for end point data to be
> used by the intel_pmt_read() function to access mmio data.

Can you please try to rephrase this, something feels wrong with the 
grammar (but do realize I'm a non-native speaker).

I'm unsure what requires it due to general vagueness feel of the wording.

I guess you wanted to say something along these lines:

PMT export API intel_pmt_read() requires end point data to perform
MMIO access.

> Without the ep, the call causes a NULL pointer exception.
> 
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> PGD 0 P4D 0
> Oops: Oops: 0000 [#1] SMP NOPTI
> CPU: 12 UID: 0 PID: 5721 Comm: cat Tainted: G           OE       6.15.0-rc4+ #3 PREEMPT(voluntary)
> Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
> Hardware name: ASUS System Product Name/PRIME Z790-P WIFI, BIOS 1641 02/21/2024
> RIP: 0010:intel_pmt_read+0x3b/0x70 [pmt_class]
> Code:
> RSP: 0018:ffffb19981ebba18 EFLAGS: 00010246
> RAX: ffffffffc0ef8e08 RBX: 0000000000000800 RCX: 0000000000000800
> RDX: ffff99aee03af450 RSI: ffff99ae86552000 RDI: 0000000000000000
> RBP: ffffb19981ebba58 R08: 0000000000000000 R09: 0000000000000800
> R10: 000000000e2f8200 R11: 0000000000000000 R12: 0000000000000000
> R13: ffff99aee03af450 R14: ffff99ae8a4bbc00 R15: ffff99ae86a35a40
> FS:  00007f097dd88740(0000) GS:ffff99b62fbe8000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 0000000198860005 CR4: 0000000000f72ef0
> PKRU: 55555554
> Call Trace:
>  <TASK>
>  ? sysfs_kf_bin_read+0xc0/0xe0
>  kernfs_fop_read_iter+0xac/0x1a0
>  vfs_read+0x26d/0x350
>  ksys_read+0x6b/0xe0
>  __x64_sys_read+0x1d/0x30
>  x64_sys_call+0x1bc8/0x1d70
>  do_syscall_64+0x6d/0x110
>  ? __mod_memcg_lruvec_state+0xe7/0x240
>  ? __lruvec_stat_mod_folio+0x8f/0xe0
>  ? set_ptes.isra.0+0x3b/0x80
>  ? do_anonymous_page+0x101/0x9c0
>  ? ___pte_offset_map+0x20/0x180
>  ? __handle_mm_fault+0xba3/0x1010
>  ? __count_memcg_events+0xca/0x190
>  ? count_memcg_events.constprop.0+0x1e/0x40
>  ? handle_mm_fault+0x1a8/0x2b0
>  ? do_user_addr_fault+0x2f6/0x7b0
>  ? irqentry_exit_to_user_mode+0x33/0x170
>  ? irqentry_exit+0x3f/0x50
>  ? exc_page_fault+0x94/0x1b0
>  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> RIP: 0033:0x7f097db25701

Please trim this splat to have only what's relevant.

> Add the endpoint information to the crashlog driver to avoid
> the NULL pointer exception.
> 
> Two minor white space issues are addressed as well.
> 
> Fixes: 416eeb2e1fc7 ("platform/x86/intel/pmt: telemetry: Export API to read telemetry")
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 38 ++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index 6a9eb3c4b313..952bfe341f53 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -143,7 +143,7 @@ enable_show(struct device *dev, struct device_attribute *attr, char *buf)
>  
>  static ssize_t
>  enable_store(struct device *dev, struct device_attribute *attr,
> -	    const char *buf, size_t count)
> +	     const char *buf, size_t count)
>  {
>  	struct crashlog_entry *entry;
>  	bool enabled;
> @@ -177,7 +177,7 @@ trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
>  
>  static ssize_t
>  trigger_store(struct device *dev, struct device_attribute *attr,
> -	    const char *buf, size_t count)
> +	      const char *buf, size_t count)
>  {
>  	struct crashlog_entry *entry;
>  	bool trigger;

Please separate these, in general, put whitespace cleanups outside the 
lines you touch due to real code changes always into own patch.

> @@ -222,6 +222,31 @@ static const struct attribute_group pmt_crashlog_group = {
>  	.attrs	= pmt_crashlog_attrs,
>  };
>  
> +static int pmt_crashlog_add_endpoint(struct intel_vsec_device *ivdev,
> +				     struct intel_pmt_entry *entry)
> +{
> +	struct telem_endpoint *ep;
> +
> +	/* Endpoint lifetimes are managed by kref, not devres */
> +	entry->ep = kzalloc(sizeof(*entry->ep), GFP_KERNEL);
> +	if (!entry->ep)
> +		return -ENOMEM;
> +
> +	ep = entry->ep;

Since you have the local var here anyway, I'd kzalloc into ep and only 
assign into entry->ep later (after error checks and filling it).

> +	ep->pcidev = ivdev->pcidev;
> +	ep->header.access_type = entry->header.access_type;
> +	ep->header.guid = entry->header.guid;
> +	ep->header.base_offset = entry->header.base_offset;
> +	ep->header.size = entry->header.size;
> +	ep->base = entry->base;
> +	ep->present = true;
> +	ep->cb = ivdev->priv_data;
> +
> +	kref_init(&ep->kref);
> +
> +	return 0;
> +}

This is 100% copy from telemetry.c, isn't it? The code duplication should 
be avoided.

>  static int pmt_crashlog_header_decode(struct intel_pmt_entry *entry,
>  				      struct device *dev)
>  {
> @@ -252,6 +277,7 @@ static struct intel_pmt_namespace pmt_crashlog_ns = {
>  	.xa = &crashlog_array,
>  	.attr_grp = &pmt_crashlog_group,
>  	.pmt_header_decode = pmt_crashlog_header_decode,
> +	.pmt_add_endpoint = pmt_crashlog_add_endpoint,
>  };
>  
>  /*
> @@ -262,8 +288,12 @@ static void pmt_crashlog_remove(struct auxiliary_device *auxdev)
>  	struct pmt_crashlog_priv *priv = auxiliary_get_drvdata(auxdev);
>  	int i;
>  
> -	for (i = 0; i < priv->num_entries; i++)
> -		intel_pmt_dev_destroy(&priv->entry[i].entry, &pmt_crashlog_ns);
> +	for (i = 0; i < priv->num_entries; i++) {
> +		struct intel_pmt_entry *entry = &priv->entry[i].entry;
> +
> +		kfree(entry->ep);
> +		intel_pmt_dev_destroy(entry, &pmt_crashlog_ns);
> +	}
>  }
>  
>  static int pmt_crashlog_probe(struct auxiliary_device *auxdev,
> 

-- 
 i.

RE: [PATCH 1/4] platform/x86/intel/pmt: crashlog binary file endpoint

[Ruhl, Michael J]

>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>Sent: Monday, May 19, 2025 11:13 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: platform-driver-x86@vger.kernel.org
>Subject: Re: [PATCH 1/4] platform/x86/intel/pmt: crashlog binary file endpoint
>
>On Fri, 16 May 2025, Michael J. Ruhl wrote:
>
>> The export API added a requirement for end point data to be
>> used by the intel_pmt_read() function to access mmio data.
>
>Can you please try to rephrase this, something feels wrong with the
>grammar (but do realize I'm a non-native speaker).
>
>I'm unsure what requires it due to general vagueness feel of the wording.
>
>I guess you wanted to say something along these lines:
>
>PMT export API intel_pmt_read() requires end point data to perform
>MMIO access.

My attempt to be concise did lose some of the context. I will work on the wording...


>> Without the ep, the call causes a NULL pointer exception.
>>
>> BUG: kernel NULL pointer dereference, address: 0000000000000000
>> PGD 0 P4D 0
>> Oops: Oops: 0000 [#1] SMP NOPTI
>> CPU: 12 UID: 0 PID: 5721 Comm: cat Tainted: G           OE       6.15.0-rc4+ #3
>PREEMPT(voluntary)
>> Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
>> Hardware name: ASUS System Product Name/PRIME Z790-P WIFI, BIOS
>1641 02/21/2024
>> RIP: 0010:intel_pmt_read+0x3b/0x70 [pmt_class]
>> Code:
>> RSP: 0018:ffffb19981ebba18 EFLAGS: 00010246
>> RAX: ffffffffc0ef8e08 RBX: 0000000000000800 RCX: 0000000000000800
>> RDX: ffff99aee03af450 RSI: ffff99ae86552000 RDI: 0000000000000000
>> RBP: ffffb19981ebba58 R08: 0000000000000000 R09:
>0000000000000800
>> R10: 000000000e2f8200 R11: 0000000000000000 R12:
>0000000000000000
>> R13: ffff99aee03af450 R14: ffff99ae8a4bbc00 R15: ffff99ae86a35a40
>> FS:  00007f097dd88740(0000) GS:ffff99b62fbe8000(0000)
>knlGS:0000000000000000
>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 0000000000000000 CR3: 0000000198860005 CR4:
>0000000000f72ef0
>> PKRU: 55555554
>> Call Trace:
>>  <TASK>
>>  ? sysfs_kf_bin_read+0xc0/0xe0
>>  kernfs_fop_read_iter+0xac/0x1a0
>>  vfs_read+0x26d/0x350
>>  ksys_read+0x6b/0xe0
>>  __x64_sys_read+0x1d/0x30
>>  x64_sys_call+0x1bc8/0x1d70
>>  do_syscall_64+0x6d/0x110
>>  ? __mod_memcg_lruvec_state+0xe7/0x240
>>  ? __lruvec_stat_mod_folio+0x8f/0xe0
>>  ? set_ptes.isra.0+0x3b/0x80
>>  ? do_anonymous_page+0x101/0x9c0
>>  ? ___pte_offset_map+0x20/0x180
>>  ? __handle_mm_fault+0xba3/0x1010
>>  ? __count_memcg_events+0xca/0x190
>>  ? count_memcg_events.constprop.0+0x1e/0x40
>>  ? handle_mm_fault+0x1a8/0x2b0
>>  ? do_user_addr_fault+0x2f6/0x7b0
>>  ? irqentry_exit_to_user_mode+0x33/0x170
>>  ? irqentry_exit+0x3f/0x50
>>  ? exc_page_fault+0x94/0x1b0
>>  entry_SYSCALL_64_after_hwframe+0x76/0x7e
>> RIP: 0033:0x7f097db25701
>
>Please trim this splat to have only what's relevant.

Will do.

>> Add the endpoint information to the crashlog driver to avoid
>> the NULL pointer exception.
>>
>> Two minor white space issues are addressed as well.
>>
>> Fixes: 416eeb2e1fc7 ("platform/x86/intel/pmt: telemetry: Export API to read
>telemetry")
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
>> ---
>>  drivers/platform/x86/intel/pmt/crashlog.c | 38 ++++++++++++++++++++---
>>  1 file changed, 34 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
>b/drivers/platform/x86/intel/pmt/crashlog.c
>> index 6a9eb3c4b313..952bfe341f53 100644
>> --- a/drivers/platform/x86/intel/pmt/crashlog.c
>> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
>> @@ -143,7 +143,7 @@ enable_show(struct device *dev, struct
>device_attribute *attr, char *buf)
>>
>>  static ssize_t
>>  enable_store(struct device *dev, struct device_attribute *attr,
>> -	    const char *buf, size_t count)
>> +	     const char *buf, size_t count)
>>  {
>>  	struct crashlog_entry *entry;
>>  	bool enabled;
>> @@ -177,7 +177,7 @@ trigger_show(struct device *dev, struct
>device_attribute *attr, char *buf)
>>
>>  static ssize_t
>>  trigger_store(struct device *dev, struct device_attribute *attr,
>> -	    const char *buf, size_t count)
>> +	      const char *buf, size_t count)
>>  {
>>  	struct crashlog_entry *entry;
>>  	bool trigger;
>
>Please separate these, in general, put whitespace cleanups outside the
>lines you touch due to real code changes always into own patch.

Will do.

>> @@ -222,6 +222,31 @@ static const struct attribute_group
>pmt_crashlog_group = {
>>  	.attrs	= pmt_crashlog_attrs,
>>  };
>>
>> +static int pmt_crashlog_add_endpoint(struct intel_vsec_device *ivdev,
>> +				     struct intel_pmt_entry *entry)
>> +{
>> +	struct telem_endpoint *ep;
>> +
>> +	/* Endpoint lifetimes are managed by kref, not devres */
>> +	entry->ep = kzalloc(sizeof(*entry->ep), GFP_KERNEL);
>> +	if (!entry->ep)
>> +		return -ENOMEM;
>> +
>> +	ep = entry->ep;
>
>Since you have the local var here anyway, I'd kzalloc into ep and only
>assign into entry->ep later (after error checks and filling it).

Yeah, I cut and pasted the telemetry code here.  Your comment is how I would
have done it, I will update.

>> +	ep->pcidev = ivdev->pcidev;
>> +	ep->header.access_type = entry->header.access_type;
>> +	ep->header.guid = entry->header.guid;
>> +	ep->header.base_offset = entry->header.base_offset;
>> +	ep->header.size = entry->header.size;
>> +	ep->base = entry->base;
>> +	ep->present = true;
>> +	ep->cb = ivdev->priv_data;
>> +
>> +	kref_init(&ep->kref);
>> +
>> +	return 0;
>> +}
>
>This is 100% copy from telemetry.c, isn't it? The code duplication should
>be avoided.

The telemetry code has more overhead with it...i.e. the kref is actually used.  For the
crashlog code, it is not currently used.   This left me with the allocation part being in
the "common code", but not the kref parts....

I will see if I can make this a bit more clean.

Thank you for your comments!

Mike

>>  static int pmt_crashlog_header_decode(struct intel_pmt_entry *entry,
>>  				      struct device *dev)
>>  {
>> @@ -252,6 +277,7 @@ static struct intel_pmt_namespace pmt_crashlog_ns
>= {
>>  	.xa = &crashlog_array,
>>  	.attr_grp = &pmt_crashlog_group,
>>  	.pmt_header_decode = pmt_crashlog_header_decode,
>> +	.pmt_add_endpoint = pmt_crashlog_add_endpoint,
>>  };
>>
>>  /*
>> @@ -262,8 +288,12 @@ static void pmt_crashlog_remove(struct
>auxiliary_device *auxdev)
>>  	struct pmt_crashlog_priv *priv = auxiliary_get_drvdata(auxdev);
>>  	int i;
>>
>> -	for (i = 0; i < priv->num_entries; i++)
>> -		intel_pmt_dev_destroy(&priv->entry[i].entry,
>&pmt_crashlog_ns);
>> +	for (i = 0; i < priv->num_entries; i++) {
>> +		struct intel_pmt_entry *entry = &priv->entry[i].entry;
>> +
>> +		kfree(entry->ep);
>> +		intel_pmt_dev_destroy(entry, &pmt_crashlog_ns);
>> +	}
>>  }
>>
>>  static int pmt_crashlog_probe(struct auxiliary_device *auxdev,
>>
>
>--
> i.

[PATCH 2/4] platform/x86/intel/pmt: update to bit access

[Michael J. Ruhl]

The current usage of BIT offsets limits adding new
functionality to the crashlog register access.

Update the bit mask #defines to use a bit defined
structure.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/crashlog.c | 116 +++++++++++++++-------
 1 file changed, 79 insertions(+), 37 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 952bfe341f53..dba7e7c1585d 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -22,29 +22,12 @@
 /* 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
 #define BASE_OFFSET		0x8
 #define SIZE_OFFSET		0xC
 #define GET_ACCESS(v)		((v) & GENMASK(3, 0))
-#define GET_TYPE(v)		(((v) & GENMASK(7, 4)) >> 4)
-#define GET_VERSION(v)		(((v) & GENMASK(19, 16)) >> 16)
 /* size is in bytes */
 #define GET_SIZE(v)		((v) * sizeof(u32))
 
@@ -54,6 +37,39 @@ struct crashlog_entry {
 	struct mutex			control_mutex;
 };
 
+struct type1_ver0_base {
+	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 rsvd:		8;  /* ro    20:27 */
+	u32 disable:		1;  /* rw    28: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.
+	 */
+	u32 clear:		1;  /* rw    29:29 */
+	u32 manual:		1;  /* rw/1s 30:30 */
+	u32 complete:		1;  /* ro/v  31:31 */
+};
+
+struct crashlog_status {
+	union {
+		struct type1_ver0_base stat;
+		u32 status;
+	};
+};
+
+struct crashlog_control {
+	union {
+		struct type1_ver0_base ctrl;
+		u32 control;
+	};
+};
+
 struct pmt_crashlog_priv {
 	int			num_entries;
 	struct crashlog_entry	entry[];
@@ -64,27 +80,34 @@ struct pmt_crashlog_priv {
  */
 static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
+	struct crashlog_status status = {
+		.status = readl(entry->disc_table + CONTROL_OFFSET),
+	};
 
 	/* return current value of the crashlog complete flag */
-	return !!(control & CRASHLOG_FLAG_TRIGGER_COMPLETE);
+	return status.stat.complete;
+
 }
 
 static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
+	struct crashlog_status status = {
+		.status = readl(entry->disc_table + CONTROL_OFFSET),
+	};
 
 	/* return current value of the crashlog disabled flag */
-	return !!(control & CRASHLOG_FLAG_DISABLE);
+	return status.stat.disable;
 }
 
 static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
 {
-	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
+	struct crashlog_control discovery_header = {
+		.control = readl(entry->disc_table + CONTROL_OFFSET),
+	};
 	u32 crash_type, version;
 
-	crash_type = GET_TYPE(discovery_header);
-	version = GET_VERSION(discovery_header);
+	crash_type = discovery_header.ctrl.crash_type;
+	version = discovery_header.ctrl.version;
 
 	/*
 	 * Currently we only recognize OOBMSM version 0 devices.
@@ -96,37 +119,53 @@ static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
 static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
 				     bool disable)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
+	struct crashlog_control control = {
+		.control = readl(entry->disc_table + CONTROL_OFFSET),
+	};
 
 	/* clear trigger bits so we are only modifying disable flag */
-	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
+	control.ctrl.clear = 0;
+	control.ctrl.manual = 0;
+	control.ctrl.complete = 0;
 
 	if (disable)
-		control |= CRASHLOG_FLAG_DISABLE;
+		control.ctrl.disable = 1;
 	else
-		control &= ~CRASHLOG_FLAG_DISABLE;
+		control.ctrl.disable = 0;
 
-	writel(control, entry->disc_table + CONTROL_OFFSET);
+	writel(control.control, entry->disc_table + CONTROL_OFFSET);
 }
 
 static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
+	struct crashlog_control control = {
+		.control = readl(entry->disc_table + CONTROL_OFFSET),
+	};
 
-	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
-	control |= CRASHLOG_FLAG_TRIGGER_CLEAR;
+	/* clear trigger bits so we are only modifying disable flag */
+	control.ctrl.disable = 0;
+	control.ctrl.manual = 0;
+	control.ctrl.complete = 0;
 
-	writel(control, entry->disc_table + CONTROL_OFFSET);
+	control.ctrl.clear = 1;
+
+	writel(control.control, entry->disc_table + CONTROL_OFFSET);
 }
 
 static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
+	struct crashlog_control control = {
+		.control = readl(entry->disc_table + CONTROL_OFFSET),
+	};
+
+	/* clear trigger bits so we are only modifying disable flag */
+	control.ctrl.disable = 0;
+	control.ctrl.clear = 0;
+	control.ctrl.complete = 0;
 
-	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
-	control |= CRASHLOG_FLAG_TRIGGER_EXECUTE;
+	control.ctrl.manual = 1;
 
-	writel(control, entry->disc_table + CONTROL_OFFSET);
+	writel(control.control, entry->disc_table + CONTROL_OFFSET);
 }
 
 /*
@@ -304,6 +343,9 @@ static int pmt_crashlog_probe(struct auxiliary_device *auxdev,
 	size_t size;
 	int i, ret;
 
+	BUILD_BUG_ON(sizeof(struct crashlog_control) != sizeof(u32));
+	BUILD_BUG_ON(sizeof(struct crashlog_status) != sizeof(u32));
+
 	size = struct_size(priv, entry, intel_vsec_dev->num_resources);
 	priv = devm_kzalloc(&auxdev->dev, size, GFP_KERNEL);
 	if (!priv)
-- 
2.49.0

Re: [PATCH 2/4] platform/x86/intel/pmt: update to bit access

[Ilpo Järvinen]

On Fri, 16 May 2025, Michael J. Ruhl wrote:

> The current usage of BIT offsets limits adding new

BIT()

> functionality to the crashlog register access.

Please be more precise how it limits, as I don't buy it in the current 
form.

I suspect the limitation is self-imposed by not naming field properly with 
defines but putting GENMASK() inside defines that custom-coded also 
FIELD_GET/PREP() so please check if using FIELD_GET/PREP() within the 
functions (not in the macros) with named defines that only specify field 
masks can overcome the supposed limitations.

> Update the bit mask #defines to use a bit defined
> structure.

These seem quite short lines.

> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 116 +++++++++++++++-------
>  1 file changed, 79 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index 952bfe341f53..dba7e7c1585d 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -22,29 +22,12 @@
>  /* 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
>  #define BASE_OFFSET		0x8
>  #define SIZE_OFFSET		0xC
>  #define GET_ACCESS(v)		((v) & GENMASK(3, 0))
> -#define GET_TYPE(v)		(((v) & GENMASK(7, 4)) >> 4)
> -#define GET_VERSION(v)		(((v) & GENMASK(19, 16)) >> 16)
>  /* size is in bytes */
>  #define GET_SIZE(v)		((v) * sizeof(u32))
>  
> @@ -54,6 +37,39 @@ struct crashlog_entry {
>  	struct mutex			control_mutex;
>  };
>  
> +struct type1_ver0_base {
> +	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 rsvd:		8;  /* ro    20:27 */
> +	u32 disable:		1;  /* rw    28: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.
> +	 */
> +	u32 clear:		1;  /* rw    29:29 */
> +	u32 manual:		1;  /* rw/1s 30:30 */
> +	u32 complete:		1;  /* ro/v  31:31 */
> +};
> +
> +struct crashlog_status {
> +	union {
> +		struct type1_ver0_base stat;
> +		u32 status;
> +	};
> +};
> +
> +struct crashlog_control {
> +	union {
> +		struct type1_ver0_base ctrl;
> +		u32 control;
> +	};
> +};
> +
>  struct pmt_crashlog_priv {
>  	int			num_entries;
>  	struct crashlog_entry	entry[];
> @@ -64,27 +80,34 @@ struct pmt_crashlog_priv {
>   */
>  static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +	struct crashlog_status status = {
> +		.status = readl(entry->disc_table + CONTROL_OFFSET),
> +	};
>  
>  	/* return current value of the crashlog complete flag */
> -	return !!(control & CRASHLOG_FLAG_TRIGGER_COMPLETE);
> +	return status.stat.complete;
> +
>  }
>  
>  static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +	struct crashlog_status status = {
> +		.status = readl(entry->disc_table + CONTROL_OFFSET),
> +	};
>  
>  	/* return current value of the crashlog disabled flag */
> -	return !!(control & CRASHLOG_FLAG_DISABLE);
> +	return status.stat.disable;
>  }
>  
>  static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
>  {
> -	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
> +	struct crashlog_control discovery_header = {
> +		.control = readl(entry->disc_table + CONTROL_OFFSET),
> +	};
>  	u32 crash_type, version;
>  
> -	crash_type = GET_TYPE(discovery_header);
> -	version = GET_VERSION(discovery_header);
> +	crash_type = discovery_header.ctrl.crash_type;
> +	version = discovery_header.ctrl.version;
>  
>  	/*
>  	 * Currently we only recognize OOBMSM version 0 devices.
> @@ -96,37 +119,53 @@ static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
>  static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
>  				     bool disable)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +	struct crashlog_control control = {
> +		.control = readl(entry->disc_table + CONTROL_OFFSET),
> +	};
>  
>  	/* clear trigger bits so we are only modifying disable flag */
> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> +	control.ctrl.clear = 0;
> +	control.ctrl.manual = 0;
> +	control.ctrl.complete = 0;
>  
>  	if (disable)
> -		control |= CRASHLOG_FLAG_DISABLE;
> +		control.ctrl.disable = 1;
>  	else
> -		control &= ~CRASHLOG_FLAG_DISABLE;
> +		control.ctrl.disable = 0;
>  
> -	writel(control, entry->disc_table + CONTROL_OFFSET);
> +	writel(control.control, entry->disc_table + CONTROL_OFFSET);
>  }
>  
>  static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +	struct crashlog_control control = {
> +		.control = readl(entry->disc_table + CONTROL_OFFSET),
> +	};
>  
> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> -	control |= CRASHLOG_FLAG_TRIGGER_CLEAR;
> +	/* clear trigger bits so we are only modifying disable flag */
> +	control.ctrl.disable = 0;
> +	control.ctrl.manual = 0;
> +	control.ctrl.complete = 0;
>  
> -	writel(control, entry->disc_table + CONTROL_OFFSET);
> +	control.ctrl.clear = 1;
> +
> +	writel(control.control, entry->disc_table + CONTROL_OFFSET);
>  }
>  
>  static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +	struct crashlog_control control = {
> +		.control = readl(entry->disc_table + CONTROL_OFFSET),
> +	};
> +
> +	/* clear trigger bits so we are only modifying disable flag */
> +	control.ctrl.disable = 0;
> +	control.ctrl.clear = 0;
> +	control.ctrl.complete = 0;
>  
> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> -	control |= CRASHLOG_FLAG_TRIGGER_EXECUTE;
> +	control.ctrl.manual = 1;
>  
> -	writel(control, entry->disc_table + CONTROL_OFFSET);
> +	writel(control.control, entry->disc_table + CONTROL_OFFSET);
>  }
>  
>  /*
> @@ -304,6 +343,9 @@ static int pmt_crashlog_probe(struct auxiliary_device *auxdev,
>  	size_t size;
>  	int i, ret;
>  
> +	BUILD_BUG_ON(sizeof(struct crashlog_control) != sizeof(u32));
> +	BUILD_BUG_ON(sizeof(struct crashlog_status) != sizeof(u32));
> +
>  	size = struct_size(priv, entry, intel_vsec_dev->num_resources);
>  	priv = devm_kzalloc(&auxdev->dev, size, GFP_KERNEL);
>  	if (!priv)
> 

-- 
 i.

RE: [PATCH 2/4] platform/x86/intel/pmt: update to bit access

[Ruhl, Michael J]

>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>Sent: Monday, May 19, 2025 11:19 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: platform-driver-x86@vger.kernel.org
>Subject: Re: [PATCH 2/4] platform/x86/intel/pmt: update to bit access
>
>On Fri, 16 May 2025, Michael J. Ruhl wrote:
>
>> The current usage of BIT offsets limits adding new
>
>BIT()
>
>> functionality to the crashlog register access.
>
>Please be more precise how it limits, as I don't buy it in the current
>form.
>
>I suspect the limitation is self-imposed by not naming field properly with
>defines but putting GENMASK() inside defines that custom-coded also
>FIELD_GET/PREP() so please check if using FIELD_GET/PREP() within the
>functions (not in the macros) with named defines that only specify field
>masks can overcome the supposed limitations.

The big issue that I was trying to address is that depending on the crashlog version,
the control and status bits can be in the same space, or separated.

For crashlog version 0 the status and control are all in the one long word.  For version
2, status is in the first word, and control in a separate word.

Also the bit definitions are slightly different for each and the bit definitions felt awkward..

I will revisit this and see if I can use the appropriate defines.

Thanks!

M

>> Update the bit mask #defines to use a bit defined
>> structure.
>
>These seem quite short lines.
>
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
>> ---
>>  drivers/platform/x86/intel/pmt/crashlog.c | 116 +++++++++++++++-------
>>  1 file changed, 79 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
>b/drivers/platform/x86/intel/pmt/crashlog.c
>> index 952bfe341f53..dba7e7c1585d 100644
>> --- a/drivers/platform/x86/intel/pmt/crashlog.c
>> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
>> @@ -22,29 +22,12 @@
>>  /* 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
>>  #define BASE_OFFSET		0x8
>>  #define SIZE_OFFSET		0xC
>>  #define GET_ACCESS(v)		((v) & GENMASK(3, 0))
>> -#define GET_TYPE(v)		(((v) & GENMASK(7, 4)) >> 4)
>> -#define GET_VERSION(v)		(((v) & GENMASK(19, 16)) >> 16)
>>  /* size is in bytes */
>>  #define GET_SIZE(v)		((v) * sizeof(u32))
>>
>> @@ -54,6 +37,39 @@ struct crashlog_entry {
>>  	struct mutex			control_mutex;
>>  };
>>
>> +struct type1_ver0_base {
>> +	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 rsvd:		8;  /* ro    20:27 */
>> +	u32 disable:		1;  /* rw    28: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.
>> +	 */
>> +	u32 clear:		1;  /* rw    29:29 */
>> +	u32 manual:		1;  /* rw/1s 30:30 */
>> +	u32 complete:		1;  /* ro/v  31:31 */
>> +};
>> +
>> +struct crashlog_status {
>> +	union {
>> +		struct type1_ver0_base stat;
>> +		u32 status;
>> +	};
>> +};
>> +
>> +struct crashlog_control {
>> +	union {
>> +		struct type1_ver0_base ctrl;
>> +		u32 control;
>> +	};
>> +};
>> +
>>  struct pmt_crashlog_priv {
>>  	int			num_entries;
>>  	struct crashlog_entry	entry[];
>> @@ -64,27 +80,34 @@ struct pmt_crashlog_priv {
>>   */
>>  static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
>>  {
>> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
>> +	struct crashlog_status status = {
>> +		.status = readl(entry->disc_table + CONTROL_OFFSET),
>> +	};
>>
>>  	/* return current value of the crashlog complete flag */
>> -	return !!(control & CRASHLOG_FLAG_TRIGGER_COMPLETE);
>> +	return status.stat.complete;
>> +
>>  }
>>
>>  static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry)
>>  {
>> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
>> +	struct crashlog_status status = {
>> +		.status = readl(entry->disc_table + CONTROL_OFFSET),
>> +	};
>>
>>  	/* return current value of the crashlog disabled flag */
>> -	return !!(control & CRASHLOG_FLAG_DISABLE);
>> +	return status.stat.disable;
>>  }
>>
>>  static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
>>  {
>> -	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
>> +	struct crashlog_control discovery_header = {
>> +		.control = readl(entry->disc_table + CONTROL_OFFSET),
>> +	};
>>  	u32 crash_type, version;
>>
>> -	crash_type = GET_TYPE(discovery_header);
>> -	version = GET_VERSION(discovery_header);
>> +	crash_type = discovery_header.ctrl.crash_type;
>> +	version = discovery_header.ctrl.version;
>>
>>  	/*
>>  	 * Currently we only recognize OOBMSM version 0 devices.
>> @@ -96,37 +119,53 @@ static bool pmt_crashlog_supported(struct
>intel_pmt_entry *entry)
>>  static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
>>  				     bool disable)
>>  {
>> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
>> +	struct crashlog_control control = {
>> +		.control = readl(entry->disc_table + CONTROL_OFFSET),
>> +	};
>>
>>  	/* clear trigger bits so we are only modifying disable flag */
>> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
>> +	control.ctrl.clear = 0;
>> +	control.ctrl.manual = 0;
>> +	control.ctrl.complete = 0;
>>
>>  	if (disable)
>> -		control |= CRASHLOG_FLAG_DISABLE;
>> +		control.ctrl.disable = 1;
>>  	else
>> -		control &= ~CRASHLOG_FLAG_DISABLE;
>> +		control.ctrl.disable = 0;
>>
>> -	writel(control, entry->disc_table + CONTROL_OFFSET);
>> +	writel(control.control, entry->disc_table + CONTROL_OFFSET);
>>  }
>>
>>  static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
>>  {
>> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
>> +	struct crashlog_control control = {
>> +		.control = readl(entry->disc_table + CONTROL_OFFSET),
>> +	};
>>
>> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
>> -	control |= CRASHLOG_FLAG_TRIGGER_CLEAR;
>> +	/* clear trigger bits so we are only modifying disable flag */
>> +	control.ctrl.disable = 0;
>> +	control.ctrl.manual = 0;
>> +	control.ctrl.complete = 0;
>>
>> -	writel(control, entry->disc_table + CONTROL_OFFSET);
>> +	control.ctrl.clear = 1;
>> +
>> +	writel(control.control, entry->disc_table + CONTROL_OFFSET);
>>  }
>>
>>  static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
>>  {
>> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
>> +	struct crashlog_control control = {
>> +		.control = readl(entry->disc_table + CONTROL_OFFSET),
>> +	};
>> +
>> +	/* clear trigger bits so we are only modifying disable flag */
>> +	control.ctrl.disable = 0;
>> +	control.ctrl.clear = 0;
>> +	control.ctrl.complete = 0;
>>
>> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
>> -	control |= CRASHLOG_FLAG_TRIGGER_EXECUTE;
>> +	control.ctrl.manual = 1;
>>
>> -	writel(control, entry->disc_table + CONTROL_OFFSET);
>> +	writel(control.control, entry->disc_table + CONTROL_OFFSET);
>>  }
>>
>>  /*
>> @@ -304,6 +343,9 @@ static int pmt_crashlog_probe(struct auxiliary_device
>*auxdev,
>>  	size_t size;
>>  	int i, ret;
>>
>> +	BUILD_BUG_ON(sizeof(struct crashlog_control) != sizeof(u32));
>> +	BUILD_BUG_ON(sizeof(struct crashlog_status) != sizeof(u32));
>> +
>>  	size = struct_size(priv, entry, intel_vsec_dev->num_resources);
>>  	priv = devm_kzalloc(&auxdev->dev, size, GFP_KERNEL);
>>  	if (!priv)
>>
>
>--
> i.

[PATCH 3/4] platform/x86/intel/pmt: decouple sysfs and namespace

[Michael J. Ruhl]

The PMT namespace includes the crashlog sysfs attribute
information.  Other crashlog version/types may need
different sysfs attributes.  Coupling the attributes with
the namespace blocks this usage.

Decouple sysfs attributes from the name space and add them
to the specific entry.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/class.c    | 12 ++++++------
 drivers/platform/x86/intel/pmt/class.h    |  2 +-
 drivers/platform/x86/intel/pmt/crashlog.c |  3 ++-
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
index 7233b654bbad..7404807c3943 100644
--- a/drivers/platform/x86/intel/pmt/class.c
+++ b/drivers/platform/x86/intel/pmt/class.c
@@ -284,8 +284,8 @@ static int intel_pmt_dev_register(struct intel_pmt_entry *entry,
 
 	entry->kobj = &dev->kobj;
 
-	if (ns->attr_grp) {
-		ret = sysfs_create_group(entry->kobj, ns->attr_grp);
+	if (entry->attr_grp) {
+		ret = sysfs_create_group(entry->kobj, entry->attr_grp);
 		if (ret)
 			goto fail_sysfs_create_group;
 	}
@@ -326,8 +326,8 @@ static int intel_pmt_dev_register(struct intel_pmt_entry *entry,
 fail_add_endpoint:
 	sysfs_remove_bin_file(entry->kobj, &entry->pmt_bin_attr);
 fail_ioremap:
-	if (ns->attr_grp)
-		sysfs_remove_group(entry->kobj, ns->attr_grp);
+	if (entry->attr_grp)
+		sysfs_remove_group(entry->kobj, entry->attr_grp);
 fail_sysfs_create_group:
 	device_unregister(dev);
 fail_dev_create:
@@ -369,8 +369,8 @@ void intel_pmt_dev_destroy(struct intel_pmt_entry *entry,
 	if (entry->size)
 		sysfs_remove_bin_file(entry->kobj, &entry->pmt_bin_attr);
 
-	if (ns->attr_grp)
-		sysfs_remove_group(entry->kobj, ns->attr_grp);
+	if (entry->attr_grp)
+		sysfs_remove_group(entry->kobj, entry->attr_grp);
 
 	device_unregister(dev);
 	xa_erase(ns->xa, entry->devid);
diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h
index b2006d57779d..6b3455a86471 100644
--- a/drivers/platform/x86/intel/pmt/class.h
+++ b/drivers/platform/x86/intel/pmt/class.h
@@ -41,6 +41,7 @@ struct intel_pmt_entry {
 	struct telem_endpoint	*ep;
 	struct intel_pmt_header	header;
 	struct bin_attribute	pmt_bin_attr;
+	const struct attribute_group *attr_grp;
 	struct kobject		*kobj;
 	void __iomem		*disc_table;
 	void __iomem		*base;
@@ -54,7 +55,6 @@ struct intel_pmt_entry {
 struct intel_pmt_namespace {
 	const char *name;
 	struct xarray *xa;
-	const struct attribute_group *attr_grp;
 	int (*pmt_header_decode)(struct intel_pmt_entry *entry,
 				 struct device *dev);
 	int (*pmt_add_endpoint)(struct intel_vsec_device *ivdev,
diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index dba7e7c1585d..c9bfe1c26311 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -307,6 +307,8 @@ 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;
+
 	return 0;
 }
 
@@ -314,7 +316,6 @@ static DEFINE_XARRAY_ALLOC(crashlog_array);
 static struct intel_pmt_namespace pmt_crashlog_ns = {
 	.name = "crashlog",
 	.xa = &crashlog_array,
-	.attr_grp = &pmt_crashlog_group,
 	.pmt_header_decode = pmt_crashlog_header_decode,
 	.pmt_add_endpoint = pmt_crashlog_add_endpoint,
 };
-- 
2.49.0

Re: [PATCH 3/4] platform/x86/intel/pmt: decouple sysfs and namespace

[Ilpo Järvinen]

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

On Fri, 16 May 2025, Michael J. Ruhl wrote:

> The PMT namespace includes the crashlog sysfs attribute
> information.  Other crashlog version/types may need

One space is enough after . (it might be your editor auto-adds the second 
space, if that's the case, look into the editors settings).

> different sysfs attributes.  Coupling the attributes with
> the namespace blocks this usage.
> 
> Decouple sysfs attributes from the name space and add them
> to the specific entry.

Too short lines here as well, please reflow the paragraphs.

> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/class.c    | 12 ++++++------
>  drivers/platform/x86/intel/pmt/class.h    |  2 +-
>  drivers/platform/x86/intel/pmt/crashlog.c |  3 ++-
>  3 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
> index 7233b654bbad..7404807c3943 100644
> --- a/drivers/platform/x86/intel/pmt/class.c
> +++ b/drivers/platform/x86/intel/pmt/class.c
> @@ -284,8 +284,8 @@ static int intel_pmt_dev_register(struct intel_pmt_entry *entry,
>  
>  	entry->kobj = &dev->kobj;
>  
> -	if (ns->attr_grp) {
> -		ret = sysfs_create_group(entry->kobj, ns->attr_grp);
> +	if (entry->attr_grp) {
> +		ret = sysfs_create_group(entry->kobj, entry->attr_grp);
>  		if (ret)
>  			goto fail_sysfs_create_group;
>  	}
> @@ -326,8 +326,8 @@ static int intel_pmt_dev_register(struct intel_pmt_entry *entry,
>  fail_add_endpoint:
>  	sysfs_remove_bin_file(entry->kobj, &entry->pmt_bin_attr);
>  fail_ioremap:
> -	if (ns->attr_grp)
> -		sysfs_remove_group(entry->kobj, ns->attr_grp);
> +	if (entry->attr_grp)
> +		sysfs_remove_group(entry->kobj, entry->attr_grp);
>  fail_sysfs_create_group:
>  	device_unregister(dev);
>  fail_dev_create:
> @@ -369,8 +369,8 @@ void intel_pmt_dev_destroy(struct intel_pmt_entry *entry,
>  	if (entry->size)
>  		sysfs_remove_bin_file(entry->kobj, &entry->pmt_bin_attr);
>  
> -	if (ns->attr_grp)
> -		sysfs_remove_group(entry->kobj, ns->attr_grp);
> +	if (entry->attr_grp)
> +		sysfs_remove_group(entry->kobj, entry->attr_grp);
>  
>  	device_unregister(dev);
>  	xa_erase(ns->xa, entry->devid);
> diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h
> index b2006d57779d..6b3455a86471 100644
> --- a/drivers/platform/x86/intel/pmt/class.h
> +++ b/drivers/platform/x86/intel/pmt/class.h
> @@ -41,6 +41,7 @@ struct intel_pmt_entry {
>  	struct telem_endpoint	*ep;
>  	struct intel_pmt_header	header;
>  	struct bin_attribute	pmt_bin_attr;
> +	const struct attribute_group *attr_grp;
>  	struct kobject		*kobj;
>  	void __iomem		*disc_table;
>  	void __iomem		*base;
> @@ -54,7 +55,6 @@ struct intel_pmt_entry {
>  struct intel_pmt_namespace {
>  	const char *name;
>  	struct xarray *xa;
> -	const struct attribute_group *attr_grp;
>  	int (*pmt_header_decode)(struct intel_pmt_entry *entry,
>  				 struct device *dev);
>  	int (*pmt_add_endpoint)(struct intel_vsec_device *ivdev,
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index dba7e7c1585d..c9bfe1c26311 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -307,6 +307,8 @@ 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;
> +
>  	return 0;
>  }
>  
> @@ -314,7 +316,6 @@ static DEFINE_XARRAY_ALLOC(crashlog_array);
>  static struct intel_pmt_namespace pmt_crashlog_ns = {
>  	.name = "crashlog",
>  	.xa = &crashlog_array,
> -	.attr_grp = &pmt_crashlog_group,
>  	.pmt_header_decode = pmt_crashlog_header_decode,
>  	.pmt_add_endpoint = pmt_crashlog_add_endpoint,
>  };
> 

With minor changelog related issues fixed,

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


BTW, now that I remember, you should always include the correct entries 
from MAINTAINERS file as receipients. It's not enough to send only to
platform-driver-x86@vger.kernel.org.

-- 
 i.

RE: [PATCH 3/4] platform/x86/intel/pmt: decouple sysfs and namespace

[Ruhl, Michael J]

>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>Sent: Monday, May 19, 2025 11:24 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: platform-driver-x86@vger.kernel.org
>Subject: Re: [PATCH 3/4] platform/x86/intel/pmt: decouple sysfs and
>namespace
>
>On Fri, 16 May 2025, Michael J. Ruhl wrote:
>
>> The PMT namespace includes the crashlog sysfs attribute
>> information.  Other crashlog version/types may need
>
>One space is enough after . (it might be your editor auto-adds the second
>space, if that's the case, look into the editors settings).

Learned this on a manual typewriter...has been built in for a long time. 😊

>> different sysfs attributes.  Coupling the attributes with
>> the namespace blocks this usage.
>>
>> Decouple sysfs attributes from the name space and add them
>> to the specific entry.
>
>Too short lines here as well, please reflow the paragraphs.
>
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
>> ---
>>  drivers/platform/x86/intel/pmt/class.c    | 12 ++++++------
>>  drivers/platform/x86/intel/pmt/class.h    |  2 +-
>>  drivers/platform/x86/intel/pmt/crashlog.c |  3 ++-
>>  3 files changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmt/class.c
>b/drivers/platform/x86/intel/pmt/class.c
>> index 7233b654bbad..7404807c3943 100644
>> --- a/drivers/platform/x86/intel/pmt/class.c
>> +++ b/drivers/platform/x86/intel/pmt/class.c
>> @@ -284,8 +284,8 @@ static int intel_pmt_dev_register(struct
>intel_pmt_entry *entry,
>>
>>  	entry->kobj = &dev->kobj;
>>
>> -	if (ns->attr_grp) {
>> -		ret = sysfs_create_group(entry->kobj, ns->attr_grp);
>> +	if (entry->attr_grp) {
>> +		ret = sysfs_create_group(entry->kobj, entry->attr_grp);
>>  		if (ret)
>>  			goto fail_sysfs_create_group;
>>  	}
>> @@ -326,8 +326,8 @@ static int intel_pmt_dev_register(struct
>intel_pmt_entry *entry,
>>  fail_add_endpoint:
>>  	sysfs_remove_bin_file(entry->kobj, &entry->pmt_bin_attr);
>>  fail_ioremap:
>> -	if (ns->attr_grp)
>> -		sysfs_remove_group(entry->kobj, ns->attr_grp);
>> +	if (entry->attr_grp)
>> +		sysfs_remove_group(entry->kobj, entry->attr_grp);
>>  fail_sysfs_create_group:
>>  	device_unregister(dev);
>>  fail_dev_create:
>> @@ -369,8 +369,8 @@ void intel_pmt_dev_destroy(struct intel_pmt_entry
>*entry,
>>  	if (entry->size)
>>  		sysfs_remove_bin_file(entry->kobj, &entry->pmt_bin_attr);
>>
>> -	if (ns->attr_grp)
>> -		sysfs_remove_group(entry->kobj, ns->attr_grp);
>> +	if (entry->attr_grp)
>> +		sysfs_remove_group(entry->kobj, entry->attr_grp);
>>
>>  	device_unregister(dev);
>>  	xa_erase(ns->xa, entry->devid);
>> diff --git a/drivers/platform/x86/intel/pmt/class.h
>b/drivers/platform/x86/intel/pmt/class.h
>> index b2006d57779d..6b3455a86471 100644
>> --- a/drivers/platform/x86/intel/pmt/class.h
>> +++ b/drivers/platform/x86/intel/pmt/class.h
>> @@ -41,6 +41,7 @@ struct intel_pmt_entry {
>>  	struct telem_endpoint	*ep;
>>  	struct intel_pmt_header	header;
>>  	struct bin_attribute	pmt_bin_attr;
>> +	const struct attribute_group *attr_grp;
>>  	struct kobject		*kobj;
>>  	void __iomem		*disc_table;
>>  	void __iomem		*base;
>> @@ -54,7 +55,6 @@ struct intel_pmt_entry {
>>  struct intel_pmt_namespace {
>>  	const char *name;
>>  	struct xarray *xa;
>> -	const struct attribute_group *attr_grp;
>>  	int (*pmt_header_decode)(struct intel_pmt_entry *entry,
>>  				 struct device *dev);
>>  	int (*pmt_add_endpoint)(struct intel_vsec_device *ivdev,
>> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
>b/drivers/platform/x86/intel/pmt/crashlog.c
>> index dba7e7c1585d..c9bfe1c26311 100644
>> --- a/drivers/platform/x86/intel/pmt/crashlog.c
>> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
>> @@ -307,6 +307,8 @@ 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;
>> +
>>  	return 0;
>>  }
>>
>> @@ -314,7 +316,6 @@ static DEFINE_XARRAY_ALLOC(crashlog_array);
>>  static struct intel_pmt_namespace pmt_crashlog_ns = {
>>  	.name = "crashlog",
>>  	.xa = &crashlog_array,
>> -	.attr_grp = &pmt_crashlog_group,
>>  	.pmt_header_decode = pmt_crashlog_header_decode,
>>  	.pmt_add_endpoint = pmt_crashlog_add_endpoint,
>>  };
>>
>
>With minor changelog related issues fixed,
>
>Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>
>
>BTW, now that I remember, you should always include the correct entries
>from MAINTAINERS file as receipients. It's not enough to send only to
>platform-driver-x86@vger.kernel.org.

Yup.  Sorry about that, I forgot about this, and will correct it in my updates.

Mike

>--
> i.

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

[Michael J. Ruhl]

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/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
+
+#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 */
+};
+
+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 */
+	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 */
+};
+
 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/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);
+
+	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
+	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;
 }
-- 
2.49.0

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

[Ilpo Järvinen]

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?

> +#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?

> +	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. :-(

> +};
> +
>  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.

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.

> +
> +	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.

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

[Ruhl, Michael J]

>-----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...

>> +};
>> +
>>  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;
} offset;

if (version == 0) {
	offset.disabled(28);
	offset.set_disabled = BIT(28);
}
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.

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

[Ilpo Järvinen]

[-- 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.

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

[Ruhl, Michael J]

>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>Sent: Wednesday, May 21, 2025 9:17 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 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.

Ok, that makes sense.

I am working on the test infrastructure, once that is complete, I will revisit these
patches and get them updated.

Thanks!

Mike

>> }
>> 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.