[PATCH v4 00/10] Crashlog Type1 Version2 support

[PATCH v4 00/10] 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.

v2:
 - fix a misconfig for the crashlog DVSEC info in the xe driver
 - address review comments
v3:
 - re-order bug fix patches for stable
 - added re-order trigger logic patch
 - added helper patch to address repeated code patterns
 - address review comments
v4:
 - added pcidev to intel_pmt_entry to address null issue
 - dropped endpoint update patches
 - patch cleanup and address review comments

Michael J. Ruhl (10):
  platform/x86/intel/pmt: fix a crashlog NULL pointer access
  drm/xe: Correct BMG VSEC header sizing
  platform/x86/intel/pmt: white space cleanup
  platform/x86/intel/pmt: use guard(mutex)
  platform/x86/intel/pmt: re-order trigger logic
  platform/x86/intel/pmt: correct types
  platform/x86/intel/pmt: decouple sysfs and namespace
  platform/x86/intel/pmt: add register access helpers
  platform/x86/intel/pmt: use a version struct
  platform/x86/intel/pmt: support BMG crashlog

 drivers/gpu/drm/xe/xe_vsec.c              |  20 +-
 drivers/platform/x86/intel/pmt/class.c    |  15 +-
 drivers/platform/x86/intel/pmt/class.h    |   3 +-
 drivers/platform/x86/intel/pmt/crashlog.c | 469 +++++++++++++++++-----
 4 files changed, 392 insertions(+), 115 deletions(-)

-- 
2.49.0

[PATCH v4 01/10] platform/x86/intel/pmt: fix a crashlog NULL pointer access

[Michael J. Ruhl]

Usage of the intel_pmt_read() for binary sysfs, requires a pcidev.  The
current use of the endpoint value is only valid for telemetry endpoint
usage.

Without the ep, the crashlog usage causes the following NULL pointer
exception:

BUG: kernel NULL pointer dereference, address: 0000000000000000
Oops: Oops: 0000 [#1] SMP NOPTI
RIP: 0010:intel_pmt_read+0x3b/0x70 [pmt_class]
Code:
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

Augment the inte_pmt_entry to include the pcidev to allow for access to
the pcidev and avoid the NULL pointer exception.

Fixes: 416eeb2e1fc7 ("platform/x86/intel/pmt: telemetry: Export API to read telemetry")
Cc: <stable@vger.kernel.org>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/class.c | 3 ++-
 drivers/platform/x86/intel/pmt/class.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
index 7233b654bbad..d046e8752173 100644
--- a/drivers/platform/x86/intel/pmt/class.c
+++ b/drivers/platform/x86/intel/pmt/class.c
@@ -97,7 +97,7 @@ intel_pmt_read(struct file *filp, struct kobject *kobj,
 	if (count > entry->size - off)
 		count = entry->size - off;
 
-	count = pmt_telem_read_mmio(entry->ep->pcidev, entry->cb, entry->header.guid, buf,
+	count = pmt_telem_read_mmio(entry->pcidev, entry->cb, entry->header.guid, buf,
 				    entry->base, off, count);
 
 	return count;
@@ -252,6 +252,7 @@ static int intel_pmt_populate_entry(struct intel_pmt_entry *entry,
 		return -EINVAL;
 	}
 
+	entry->pcidev = pci_dev;
 	entry->guid = header->guid;
 	entry->size = header->size;
 	entry->cb = ivdev->priv_data;
diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h
index b2006d57779d..f6ce80c4e051 100644
--- a/drivers/platform/x86/intel/pmt/class.h
+++ b/drivers/platform/x86/intel/pmt/class.h
@@ -39,6 +39,7 @@ struct intel_pmt_header {
 
 struct intel_pmt_entry {
 	struct telem_endpoint	*ep;
+	struct pci_dev		*pcidev;
 	struct intel_pmt_header	header;
 	struct bin_attribute	pmt_bin_attr;
 	struct kobject		*kobj;
-- 
2.49.0

RE: [PATCH v4 01/10] platform/x86/intel/pmt: fix a crashlog NULL pointer access

[Upadhyay, Tejas]

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Michael
> J. Ruhl
> Sent: 11 June 2025 02:42
> To: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org;
> hdegoede@redhat.com; ilpo.jarvinen@linux.intel.com; De Marchi, Lucas
> <lucas.demarchi@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
> thomas.hellstrom@linux.intel.com; airlied@gmail.com; simona@ffwll.ch;
> david.e.box@linux.intel.com
> Cc: Ruhl, Michael J <michael.j.ruhl@intel.com>; stable@vger.kernel.org
> Subject: [PATCH v4 01/10] platform/x86/intel/pmt: fix a crashlog NULL pointer
> access
> 
> Usage of the intel_pmt_read() for binary sysfs, requires a pcidev.  The current
> use of the endpoint value is only valid for telemetry endpoint usage.
> 
> Without the ep, the crashlog usage causes the following NULL pointer
> exception:
> 
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> Oops: Oops: 0000 [#1] SMP NOPTI
> RIP: 0010:intel_pmt_read+0x3b/0x70 [pmt_class]
> Code:
> 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
> 
> Augment the inte_pmt_entry to include the pcidev to allow for access to the
> pcidev and avoid the NULL pointer exception.
> 
> Fixes: 416eeb2e1fc7 ("platform/x86/intel/pmt: telemetry: Export API to read
> telemetry")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/class.c | 3 ++-
> drivers/platform/x86/intel/pmt/class.h | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/class.c
> b/drivers/platform/x86/intel/pmt/class.c
> index 7233b654bbad..d046e8752173 100644
> --- a/drivers/platform/x86/intel/pmt/class.c
> +++ b/drivers/platform/x86/intel/pmt/class.c
> @@ -97,7 +97,7 @@ intel_pmt_read(struct file *filp, struct kobject *kobj,
>  	if (count > entry->size - off)
>  		count = entry->size - off;
> 
> -	count = pmt_telem_read_mmio(entry->ep->pcidev, entry->cb, entry-
> >header.guid, buf,
> +	count = pmt_telem_read_mmio(entry->pcidev, entry->cb,
> +entry->header.guid, buf,
>  				    entry->base, off, count);
> 
>  	return count;
> @@ -252,6 +252,7 @@ static int intel_pmt_populate_entry(struct
> intel_pmt_entry *entry,
>  		return -EINVAL;
>  	}
> 
> +	entry->pcidev = pci_dev;

Change looks correct to me, 
Reviewed-by: Tejas Upadhyay <tejas.upadhyay@intel.com>

Tejas
>  	entry->guid = header->guid;
>  	entry->size = header->size;
>  	entry->cb = ivdev->priv_data;
> diff --git a/drivers/platform/x86/intel/pmt/class.h
> b/drivers/platform/x86/intel/pmt/class.h
> index b2006d57779d..f6ce80c4e051 100644
> --- a/drivers/platform/x86/intel/pmt/class.h
> +++ b/drivers/platform/x86/intel/pmt/class.h
> @@ -39,6 +39,7 @@ struct intel_pmt_header {
> 
>  struct intel_pmt_entry {
>  	struct telem_endpoint	*ep;
> +	struct pci_dev		*pcidev;
>  	struct intel_pmt_header	header;
>  	struct bin_attribute	pmt_bin_attr;
>  	struct kobject		*kobj;
> --
> 2.49.0

Re: [PATCH v4 01/10] platform/x86/intel/pmt: fix a crashlog NULL pointer access

[Ilpo Järvinen]

On Tue, 10 Jun 2025, Michael J. Ruhl wrote:

> Usage of the intel_pmt_read() for binary sysfs, requires a pcidev.  The
> current use of the endpoint value is only valid for telemetry endpoint
> usage.
> 
> Without the ep, the crashlog usage causes the following NULL pointer
> exception:
> 
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> Oops: Oops: 0000 [#1] SMP NOPTI
> RIP: 0010:intel_pmt_read+0x3b/0x70 [pmt_class]
> Code:
> 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
> 
> Augment the inte_pmt_entry to include the pcidev to allow for access to

intel_pmt_entry

> the pcidev and avoid the NULL pointer exception.
> 
> Fixes: 416eeb2e1fc7 ("platform/x86/intel/pmt: telemetry: Export API to read telemetry")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/class.c | 3 ++-
>  drivers/platform/x86/intel/pmt/class.h | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
> index 7233b654bbad..d046e8752173 100644
> --- a/drivers/platform/x86/intel/pmt/class.c
> +++ b/drivers/platform/x86/intel/pmt/class.c
> @@ -97,7 +97,7 @@ intel_pmt_read(struct file *filp, struct kobject *kobj,
>  	if (count > entry->size - off)
>  		count = entry->size - off;
>  
> -	count = pmt_telem_read_mmio(entry->ep->pcidev, entry->cb, entry->header.guid, buf,
> +	count = pmt_telem_read_mmio(entry->pcidev, entry->cb, entry->header.guid, buf,
>  				    entry->base, off, count);
>  
>  	return count;
> @@ -252,6 +252,7 @@ static int intel_pmt_populate_entry(struct intel_pmt_entry *entry,
>  		return -EINVAL;
>  	}
>  
> +	entry->pcidev = pci_dev;
>  	entry->guid = header->guid;
>  	entry->size = header->size;
>  	entry->cb = ivdev->priv_data;
> diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h
> index b2006d57779d..f6ce80c4e051 100644
> --- a/drivers/platform/x86/intel/pmt/class.h
> +++ b/drivers/platform/x86/intel/pmt/class.h
> @@ -39,6 +39,7 @@ struct intel_pmt_header {
>  
>  struct intel_pmt_entry {
>  	struct telem_endpoint	*ep;
> +	struct pci_dev		*pcidev;
>  	struct intel_pmt_header	header;
>  	struct bin_attribute	pmt_bin_attr;
>  	struct kobject		*kobj;
> 

-- 
 i.

RE: [PATCH v4 01/10] platform/x86/intel/pmt: fix a crashlog NULL pointer access

[Ruhl, Michael J]

>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>Sent: Wednesday, June 11, 2025 6:42 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org; Hans
>de Goede <hdegoede@redhat.com>; De Marchi, Lucas
><lucas.demarchi@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
>thomas.hellstrom@linux.intel.com; airlied@gmail.com; simona@ffwll.ch;
>david.e.box@linux.intel.com; stable@vger.kernel.org
>Subject: Re: [PATCH v4 01/10] platform/x86/intel/pmt: fix a crashlog NULL
>pointer access
>
>On Tue, 10 Jun 2025, Michael J. Ruhl wrote:
>
>> Usage of the intel_pmt_read() for binary sysfs, requires a pcidev.  The
>> current use of the endpoint value is only valid for telemetry endpoint
>> usage.
>>
>> Without the ep, the crashlog usage causes the following NULL pointer
>> exception:
>>
>> BUG: kernel NULL pointer dereference, address: 0000000000000000
>> Oops: Oops: 0000 [#1] SMP NOPTI
>> RIP: 0010:intel_pmt_read+0x3b/0x70 [pmt_class]
>> Code:
>> 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
>>
>> Augment the inte_pmt_entry to include the pcidev to allow for access to
>
>intel_pmt_entry

I have also been told that should be "intel_pmt_entry()"....  when I redo, is that
more correct?

Thanks,

M

>> the pcidev and avoid the NULL pointer exception.
>>
>> Fixes: 416eeb2e1fc7 ("platform/x86/intel/pmt: telemetry: Export API to read
>telemetry")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
>> ---
>>  drivers/platform/x86/intel/pmt/class.c | 3 ++-
>>  drivers/platform/x86/intel/pmt/class.h | 1 +
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmt/class.c
>b/drivers/platform/x86/intel/pmt/class.c
>> index 7233b654bbad..d046e8752173 100644
>> --- a/drivers/platform/x86/intel/pmt/class.c
>> +++ b/drivers/platform/x86/intel/pmt/class.c
>> @@ -97,7 +97,7 @@ intel_pmt_read(struct file *filp, struct kobject *kobj,
>>  	if (count > entry->size - off)
>>  		count = entry->size - off;
>>
>> -	count = pmt_telem_read_mmio(entry->ep->pcidev, entry->cb, entry-
>>header.guid, buf,
>> +	count = pmt_telem_read_mmio(entry->pcidev, entry->cb, entry-
>>header.guid, buf,
>>  				    entry->base, off, count);
>>
>>  	return count;
>> @@ -252,6 +252,7 @@ static int intel_pmt_populate_entry(struct
>intel_pmt_entry *entry,
>>  		return -EINVAL;
>>  	}
>>
>> +	entry->pcidev = pci_dev;
>>  	entry->guid = header->guid;
>>  	entry->size = header->size;
>>  	entry->cb = ivdev->priv_data;
>> diff --git a/drivers/platform/x86/intel/pmt/class.h
>b/drivers/platform/x86/intel/pmt/class.h
>> index b2006d57779d..f6ce80c4e051 100644
>> --- a/drivers/platform/x86/intel/pmt/class.h
>> +++ b/drivers/platform/x86/intel/pmt/class.h
>> @@ -39,6 +39,7 @@ struct intel_pmt_header {
>>
>>  struct intel_pmt_entry {
>>  	struct telem_endpoint	*ep;
>> +	struct pci_dev		*pcidev;
>>  	struct intel_pmt_header	header;
>>  	struct bin_attribute	pmt_bin_attr;
>>  	struct kobject		*kobj;
>>
>
>--
> i.

RE: [PATCH v4 01/10] platform/x86/intel/pmt: fix a crashlog NULL pointer access

[Ilpo Järvinen]

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

On Wed, 11 Jun 2025, Ruhl, Michael J wrote:

> >-----Original Message-----
> >From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >Sent: Wednesday, June 11, 2025 6:42 AM
> >To: Ruhl, Michael J <michael.j.ruhl@intel.com>
> >Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org; Hans
> >de Goede <hdegoede@redhat.com>; De Marchi, Lucas
> ><lucas.demarchi@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
> >thomas.hellstrom@linux.intel.com; airlied@gmail.com; simona@ffwll.ch;
> >david.e.box@linux.intel.com; stable@vger.kernel.org
> >Subject: Re: [PATCH v4 01/10] platform/x86/intel/pmt: fix a crashlog NULL
> >pointer access
> >
> >On Tue, 10 Jun 2025, Michael J. Ruhl wrote:
> >
> >> Usage of the intel_pmt_read() for binary sysfs, requires a pcidev.  The
> >> current use of the endpoint value is only valid for telemetry endpoint
> >> usage.
> >>
> >> Without the ep, the crashlog usage causes the following NULL pointer
> >> exception:
> >>
> >> BUG: kernel NULL pointer dereference, address: 0000000000000000
> >> Oops: Oops: 0000 [#1] SMP NOPTI
> >> RIP: 0010:intel_pmt_read+0x3b/0x70 [pmt_class]
> >> Code:
> >> 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
> >>
> >> Augment the inte_pmt_entry to include the pcidev to allow for access to
> >
> >intel_pmt_entry
> 
> I have also been told that should be "intel_pmt_entry()"....  when I redo, is that
> more correct?

?? For structs, don't use (). Use () after any name that refers to a 
C function or a function like macro.

You could also say the struct intel_pmt_entry to indicate unambiguously to 
the reader what kind of object sits behind the name.

> Thanks,
> 
> M
> 
> >> the pcidev and avoid the NULL pointer exception.
> >>
> >> Fixes: 416eeb2e1fc7 ("platform/x86/intel/pmt: telemetry: Export API to read
> >telemetry")
> >> Cc: <stable@vger.kernel.org>
> >> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> >> ---
> >>  drivers/platform/x86/intel/pmt/class.c | 3 ++-
> >>  drivers/platform/x86/intel/pmt/class.h | 1 +
> >>  2 files changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/platform/x86/intel/pmt/class.c
> >b/drivers/platform/x86/intel/pmt/class.c
> >> index 7233b654bbad..d046e8752173 100644
> >> --- a/drivers/platform/x86/intel/pmt/class.c
> >> +++ b/drivers/platform/x86/intel/pmt/class.c
> >> @@ -97,7 +97,7 @@ intel_pmt_read(struct file *filp, struct kobject *kobj,
> >>  	if (count > entry->size - off)
> >>  		count = entry->size - off;
> >>
> >> -	count = pmt_telem_read_mmio(entry->ep->pcidev, entry->cb, entry-
> >>header.guid, buf,
> >> +	count = pmt_telem_read_mmio(entry->pcidev, entry->cb, entry-
> >>header.guid, buf,
> >>  				    entry->base, off, count);
> >>
> >>  	return count;
> >> @@ -252,6 +252,7 @@ static int intel_pmt_populate_entry(struct
> >intel_pmt_entry *entry,
> >>  		return -EINVAL;
> >>  	}
> >>
> >> +	entry->pcidev = pci_dev;
> >>  	entry->guid = header->guid;
> >>  	entry->size = header->size;
> >>  	entry->cb = ivdev->priv_data;
> >> diff --git a/drivers/platform/x86/intel/pmt/class.h
> >b/drivers/platform/x86/intel/pmt/class.h
> >> index b2006d57779d..f6ce80c4e051 100644
> >> --- a/drivers/platform/x86/intel/pmt/class.h
> >> +++ b/drivers/platform/x86/intel/pmt/class.h
> >> @@ -39,6 +39,7 @@ struct intel_pmt_header {
> >>
> >>  struct intel_pmt_entry {
> >>  	struct telem_endpoint	*ep;
> >> +	struct pci_dev		*pcidev;
> >>  	struct intel_pmt_header	header;
> >>  	struct bin_attribute	pmt_bin_attr;
> >>  	struct kobject		*kobj;
> >>
> >
> >--
> > i.
> 

-- 
 i.

[PATCH v4 02/10] drm/xe: Correct BMG VSEC header sizing

[Michael J. Ruhl]

The intel_vsec_header information for the crashlog feature is
incorrect.

Update the VSEC header with correct sizing and count.

Since the crashlog entries are "merged" (num_entries = 2), the
separate capabilities entries must be merged as well.

Fixes: 0c45e76fcc62 ("drm/xe/vsec: Support BMG devices")
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/xe/xe_vsec.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index 3e573b0b7ebd..67238fc57a4d 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -32,28 +32,18 @@ static struct intel_vsec_header bmg_telemetry = {
 	.offset = BMG_DISCOVERY_OFFSET,
 };
 
-static struct intel_vsec_header bmg_punit_crashlog = {
-	.length = 0x10,
+static struct intel_vsec_header bmg_crashlog = {
+	.length = 0x18,
 	.id = VSEC_ID_CRASHLOG,
-	.num_entries = 1,
-	.entry_size = 4,
+	.num_entries = 2,
+	.entry_size = 6,
 	.tbir = 0,
 	.offset = BMG_DISCOVERY_OFFSET + 0x60,
 };
 
-static struct intel_vsec_header bmg_oobmsm_crashlog = {
-	.length = 0x10,
-	.id = VSEC_ID_CRASHLOG,
-	.num_entries = 1,
-	.entry_size = 4,
-	.tbir = 0,
-	.offset = BMG_DISCOVERY_OFFSET + 0x78,
-};
-
 static struct intel_vsec_header *bmg_capabilities[] = {
 	&bmg_telemetry,
-	&bmg_punit_crashlog,
-	&bmg_oobmsm_crashlog,
+	&bmg_crashlog,
 	NULL
 };
 
-- 
2.49.0

[PATCH v4 03/10] platform/x86/intel/pmt: white space cleanup

[Michael J. Ruhl]

Noticed two white space issues; cleaned them.

Reviewed-by: David E. Box <david.e.box@linux.intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/crashlog.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 6a9eb3c4b313..d40c8e212733 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;
-- 
2.49.0

[PATCH v4 04/10] platform/x86/intel/pmt: use guard(mutex)

[Michael J. Ruhl]

Update the mutex paths to use the new guard() mechanism.

With the removal of goto, do some minor cleanup of the current logic
path.

Reviewed-by: David E. Box <david.e.box@linux.intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/crashlog.c | 33 +++++++++++------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index d40c8e212733..1525e702e993 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/auxiliary_bus.h>
+#include <linux/cleanup.h>
 #include <linux/intel_vsec.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -155,9 +156,9 @@ enable_store(struct device *dev, struct device_attribute *attr,
 	if (result)
 		return result;
 
-	mutex_lock(&entry->control_mutex);
+	guard(mutex)(&entry->control_mutex);
+
 	pmt_crashlog_set_disable(&entry->entry, !enabled);
-	mutex_unlock(&entry->control_mutex);
 
 	return count;
 }
@@ -189,26 +190,24 @@ trigger_store(struct device *dev, struct device_attribute *attr,
 	if (result)
 		return result;
 
-	mutex_lock(&entry->control_mutex);
+	guard(mutex)(&entry->control_mutex);
 
 	if (!trigger) {
 		pmt_crashlog_set_clear(&entry->entry);
-	} else if (pmt_crashlog_complete(&entry->entry)) {
-		/* we cannot trigger a new crash if one is still pending */
-		result = -EEXIST;
-		goto err;
-	} else if (pmt_crashlog_disabled(&entry->entry)) {
-		/* if device is currently disabled, return busy */
-		result = -EBUSY;
-		goto err;
-	} else {
-		pmt_crashlog_set_execute(&entry->entry);
+		return count;
 	}
 
-	result = count;
-err:
-	mutex_unlock(&entry->control_mutex);
-	return result;
+	/* we cannot trigger a new crash if one is still pending */
+	if (pmt_crashlog_complete(&entry->entry))
+		return -EEXIST;
+
+	/* if device is currently disabled, return busy */
+	if (pmt_crashlog_disabled(&entry->entry))
+		return -EBUSY;
+
+	pmt_crashlog_set_execute(&entry->entry);
+
+	return count;
 }
 static DEVICE_ATTR_RW(trigger);
 
-- 
2.49.0

[PATCH v4 05/10] platform/x86/intel/pmt: re-order trigger logic

[Michael J. Ruhl]

Setting the clear bit or checking the complete bit before checking to
see if crashlog is disabled seems incorrect.

Check disable before accessing any other bits.

Reviewed-by: David E. Box <david.e.box@linux.intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/crashlog.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 1525e702e993..3d3b72451c01 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -192,6 +192,10 @@ trigger_store(struct device *dev, struct device_attribute *attr,
 
 	guard(mutex)(&entry->control_mutex);
 
+	/* if device is currently disabled, return busy */
+	if (pmt_crashlog_disabled(&entry->entry))
+		return -EBUSY;
+
 	if (!trigger) {
 		pmt_crashlog_set_clear(&entry->entry);
 		return count;
@@ -201,10 +205,6 @@ trigger_store(struct device *dev, struct device_attribute *attr,
 	if (pmt_crashlog_complete(&entry->entry))
 		return -EEXIST;
 
-	/* if device is currently disabled, return busy */
-	if (pmt_crashlog_disabled(&entry->entry))
-		return -EBUSY;
-
 	pmt_crashlog_set_execute(&entry->entry);
 
 	return count;
-- 
2.49.0

Re: [PATCH v4 05/10] platform/x86/intel/pmt: re-order trigger logic

[Ilpo Järvinen]

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

On Tue, 10 Jun 2025, Michael J. Ruhl wrote:

> Setting the clear bit or checking the complete bit before checking to
> see if crashlog is disabled seems incorrect.
> 
> Check disable before accessing any other bits.
> 
> Reviewed-by: David E. Box <david.e.box@linux.intel.com>
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index 1525e702e993..3d3b72451c01 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -192,6 +192,10 @@ trigger_store(struct device *dev, struct device_attribute *attr,
>  
>  	guard(mutex)(&entry->control_mutex);
>  
> +	/* if device is currently disabled, return busy */
> +	if (pmt_crashlog_disabled(&entry->entry))
> +		return -EBUSY;
> +
>  	if (!trigger) {
>  		pmt_crashlog_set_clear(&entry->entry);
>  		return count;
> @@ -201,10 +205,6 @@ trigger_store(struct device *dev, struct device_attribute *attr,
>  	if (pmt_crashlog_complete(&entry->entry))
>  		return -EEXIST;
>  
> -	/* if device is currently disabled, return busy */
> -	if (pmt_crashlog_disabled(&entry->entry))
> -		return -EBUSY;
> -
>  	pmt_crashlog_set_execute(&entry->entry);
>  
>  	return count;
> 

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

-- 
 i.

[PATCH v4 06/10] platform/x86/intel/pmt: correct types

[Michael J. Ruhl]

A couple of auto variables do not match the return types of some of
the functions.

Update the mismatched types to match.

Reviewed-by: David E. Box <david.e.box@linux.intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/crashlog.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 3d3b72451c01..ddb05d069fc6 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -137,7 +137,7 @@ static ssize_t
 enable_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct intel_pmt_entry *entry = dev_get_drvdata(dev);
-	int enabled = !pmt_crashlog_disabled(entry);
+	bool enabled = !pmt_crashlog_disabled(entry);
 
 	return sprintf(buf, "%d\n", enabled);
 }
@@ -168,7 +168,7 @@ static ssize_t
 trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct intel_pmt_entry *entry;
-	int trigger;
+	bool trigger;
 
 	entry = dev_get_drvdata(dev);
 	trigger = pmt_crashlog_complete(entry);
-- 
2.49.0

Re: [PATCH v4 06/10] platform/x86/intel/pmt: correct types

[Ilpo Järvinen]

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

On Tue, 10 Jun 2025, Michael J. Ruhl wrote:

> A couple of auto variables do not match the return types of some of
> the functions.
> 
> Update the mismatched types to match.
> 
> Reviewed-by: David E. Box <david.e.box@linux.intel.com>
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index 3d3b72451c01..ddb05d069fc6 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -137,7 +137,7 @@ static ssize_t
>  enable_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	struct intel_pmt_entry *entry = dev_get_drvdata(dev);
> -	int enabled = !pmt_crashlog_disabled(entry);
> +	bool enabled = !pmt_crashlog_disabled(entry);
>  
>  	return sprintf(buf, "%d\n", enabled);
>  }
> @@ -168,7 +168,7 @@ static ssize_t
>  trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	struct intel_pmt_entry *entry;
> -	int trigger;
> +	bool trigger;
>  
>  	entry = dev_get_drvdata(dev);
>  	trigger = pmt_crashlog_complete(entry);
> 

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

-- 
 i.

[PATCH v4 07/10] 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.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
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 d046e8752173..3b6bf2f14dcb 100644
--- a/drivers/platform/x86/intel/pmt/class.c
+++ b/drivers/platform/x86/intel/pmt/class.c
@@ -285,8 +285,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;
 	}
@@ -327,8 +327,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:
@@ -370,8 +370,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 f6ce80c4e051..d5d86b8a2d15 100644
--- a/drivers/platform/x86/intel/pmt/class.h
+++ b/drivers/platform/x86/intel/pmt/class.h
@@ -42,6 +42,7 @@ struct intel_pmt_entry {
 	struct pci_dev		*pcidev;
 	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;
@@ -55,7 +56,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 ddb05d069fc6..99f0e85f2de6 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -242,6 +242,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;
 }
 
@@ -249,7 +251,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,
 };
 
-- 
2.49.0

[PATCH v4 08/10] platform/x86/intel/pmt: add register access helpers

[Michael J. Ruhl]

The control register is used in a read/modify/write pattern.
The status register is used in a read/check bit pattern.

Add helpers to eliminate common code.

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

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 99f0e85f2de6..e11865686f2a 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -63,20 +63,40 @@ struct pmt_crashlog_priv {
 /*
  * I/O
  */
-static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
+#define SET	true
+#define CLEAR	false
+
+static void read_modify_write(struct intel_pmt_entry *entry, u32 bit, bool set)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
+	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
+
+	reg &= ~CRASHLOG_FLAG_TRIGGER_MASK;
+
+	if (set)
+		reg |= bit;
+	else
+		reg &= bit;
+
+	writel(reg, entry->disc_table + CONTROL_OFFSET);
+}
+
+static bool read_check(struct intel_pmt_entry *entry, u32 bit)
+{
+	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
+
+	return !!(reg & bit);
+}
 
+static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
+{
 	/* return current value of the crashlog complete flag */
-	return !!(control & CRASHLOG_FLAG_TRIGGER_COMPLETE);
+	return read_check(entry, CRASHLOG_FLAG_TRIGGER_COMPLETE);
 }
 
 static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
-
 	/* return current value of the crashlog disabled flag */
-	return !!(control & CRASHLOG_FLAG_DISABLE);
+	return read_check(entry, CRASHLOG_FLAG_DISABLE);
 }
 
 static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
@@ -97,37 +117,17 @@ 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);
-
-	/* clear trigger bits so we are only modifying disable flag */
-	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
-
-	if (disable)
-		control |= CRASHLOG_FLAG_DISABLE;
-	else
-		control &= ~CRASHLOG_FLAG_DISABLE;
-
-	writel(control, entry->disc_table + CONTROL_OFFSET);
+	read_modify_write(entry, CRASHLOG_FLAG_DISABLE, disable);
 }
 
 static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
-
-	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
-	control |= CRASHLOG_FLAG_TRIGGER_CLEAR;
-
-	writel(control, entry->disc_table + CONTROL_OFFSET);
+	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_CLEAR, SET);
 }
 
 static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
-
-	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
-	control |= CRASHLOG_FLAG_TRIGGER_EXECUTE;
-
-	writel(control, entry->disc_table + CONTROL_OFFSET);
+	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_EXECUTE, SET);
 }
 
 /*
-- 
2.49.0

Re: [PATCH v4 08/10] platform/x86/intel/pmt: add register access helpers

[Ilpo Järvinen]

On Tue, 10 Jun 2025, Michael J. Ruhl wrote:

> The control register is used in a read/modify/write pattern.
> The status register is used in a read/check bit pattern.
> 
> Add helpers to eliminate common code.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 58 +++++++++++------------
>  1 file changed, 29 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index 99f0e85f2de6..e11865686f2a 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -63,20 +63,40 @@ struct pmt_crashlog_priv {
>  /*
>   * I/O
>   */
> -static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
> +#define SET	true
> +#define CLEAR	false

There's a risk of namespace collisions if using too generic names.

> +static void read_modify_write(struct intel_pmt_entry *entry, u32 bit, bool set)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
> +
> +	reg &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> +
> +	if (set)
> +		reg |= bit;
> +	else
> +		reg &= bit;
> +
> +	writel(reg, entry->disc_table + CONTROL_OFFSET);
> +}
> +
> +static bool read_check(struct intel_pmt_entry *entry, u32 bit)

Despite being static, I'd prefer these to have prefixes. With the prefixes 
reading the calling code, it's trivial to discern it's an driver internal 
function whereas generic names likes will not convey the scope 
information.

> +{
> +	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
> +
> +	return !!(reg & bit);
> +}
>  
> +static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
> +{
>  	/* return current value of the crashlog complete flag */
> -	return !!(control & CRASHLOG_FLAG_TRIGGER_COMPLETE);
> +	return read_check(entry, CRASHLOG_FLAG_TRIGGER_COMPLETE);
>  }
>  
>  static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> -
>  	/* return current value of the crashlog disabled flag */
> -	return !!(control & CRASHLOG_FLAG_DISABLE);
> +	return read_check(entry, CRASHLOG_FLAG_DISABLE);
>  }
>  
>  static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
> @@ -97,37 +117,17 @@ 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);
> -
> -	/* clear trigger bits so we are only modifying disable flag */
> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> -
> -	if (disable)
> -		control |= CRASHLOG_FLAG_DISABLE;
> -	else
> -		control &= ~CRASHLOG_FLAG_DISABLE;
> -
> -	writel(control, entry->disc_table + CONTROL_OFFSET);
> +	read_modify_write(entry, CRASHLOG_FLAG_DISABLE, disable);
>  }
>  
>  static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> -
> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> -	control |= CRASHLOG_FLAG_TRIGGER_CLEAR;
> -
> -	writel(control, entry->disc_table + CONTROL_OFFSET);
> +	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_CLEAR, SET);
>  }
>  
>  static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> -
> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> -	control |= CRASHLOG_FLAG_TRIGGER_EXECUTE;
> -
> -	writel(control, entry->disc_table + CONTROL_OFFSET);
> +	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_EXECUTE, SET);
>  }
>  
>  /*
> 

-- 
 i.

[PATCH v4 09/10] platform/x86/intel/pmt: use a version struct

[Michael J. Ruhl]

In preparation for supporting multiple crashlog versions, use a struct
to keep bit offset info for the status and control bits.

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

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index e11865686f2a..7c259b1cf95b 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -23,21 +23,6 @@
 /* Crashlog discovery header types */
 #define CRASH_TYPE_OOBMSM	1
 
-/* Control Flags */
-#define CRASHLOG_FLAG_DISABLE		BIT(28)
-
-/*
- * Bits 29 and 30 control the state of bit 31.
- *
- * Bit 29 will clear bit 31, if set, allowing a new crashlog to be captured.
- * Bit 30 will immediately trigger a crashlog to be generated, setting bit 31.
- * Bit 31 is the read-only status with a 1 indicating log is complete.
- */
-#define CRASHLOG_FLAG_TRIGGER_CLEAR	BIT(29)
-#define CRASHLOG_FLAG_TRIGGER_EXECUTE	BIT(30)
-#define CRASHLOG_FLAG_TRIGGER_COMPLETE	BIT(31)
-#define CRASHLOG_FLAG_TRIGGER_MASK	GENMASK(31, 28)
-
 /* Crashlog Discovery Header */
 #define CONTROL_OFFSET		0x0
 #define GUID_OFFSET		0x4
@@ -49,10 +34,63 @@
 /* size is in bytes */
 #define GET_SIZE(v)		((v) * sizeof(u32))
 
+/*
+ * Type 1 Version 0
+ * status and control registers are combined.
+ *
+ * Bits 29 and 30 control the state of bit 31.
+ * Bit 29 will clear bit 31, if set, allowing a new crashlog to be captured.
+ * Bit 30 will immediately trigger a crashlog to be generated, setting bit 31.
+ * Bit 31 is the read-only status with a 1 indicating log is complete.
+ */
+#define TYPE1_VER0_STATUS_OFFSET	0x00
+#define TYPE1_VER0_CONTROL_OFFSET	0x00
+
+#define TYPE1_VER0_DISABLE		BIT(28)
+#define TYPE1_VER0_CLEAR		BIT(29)
+#define TYPE1_VER0_EXECUTE		BIT(30)
+#define TYPE1_VER0_COMPLETE		BIT(31)
+#define TYPE1_VER0_TRIGGER_MASK		GENMASK(31, 28)
+
+/* After offset, order alphabetically, not bit ordered */
+struct crashlog_status {
+	u32 offset;
+	u32 cleared;
+	u32 complete;
+	u32 disabled;
+};
+
+struct crashlog_control {
+	u32 offset;
+	u32 trigger_mask;
+	u32 clear;
+	u32 disable;
+	u32 manual;
+};
+
+struct crashlog_info {
+	struct crashlog_status status;
+	struct crashlog_control control;
+};
+
+static const struct crashlog_info crashlog_type1_ver0 = {
+	.status.offset = TYPE1_VER0_STATUS_OFFSET,
+	.status.cleared = TYPE1_VER0_CLEAR,
+	.status.complete = TYPE1_VER0_COMPLETE,
+	.status.disabled = TYPE1_VER0_DISABLE,
+
+	.control.offset = TYPE1_VER0_CONTROL_OFFSET,
+	.control.trigger_mask = TYPE1_VER0_TRIGGER_MASK,
+	.control.clear = TYPE1_VER0_CLEAR,
+	.control.disable = TYPE1_VER0_DISABLE,
+	.control.manual = TYPE1_VER0_EXECUTE,
+};
+
 struct crashlog_entry {
 	/* entry must be first member of struct */
 	struct intel_pmt_entry		entry;
 	struct mutex			control_mutex;
+	const struct crashlog_info	*info;
 };
 
 struct pmt_crashlog_priv {
@@ -60,74 +98,76 @@ struct pmt_crashlog_priv {
 	struct crashlog_entry	entry[];
 };
 
+static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
+{
+	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
+	u32 crash_type, version;
+
+	crash_type = GET_TYPE(discovery_header);
+	version = GET_VERSION(discovery_header);
+
+	/*
+	 * Currently we only recognize OOBMSM version 0 devices.
+	 * We can ignore all other crashlog devices in the system.
+	 */
+	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
+}
+
 /*
  * I/O
  */
-#define SET	true
-#define CLEAR	false
+#define SET     true
+#define CLEAR   false
 
-static void read_modify_write(struct intel_pmt_entry *entry, u32 bit, bool set)
+static void read_modify_write(struct crashlog_entry *crashlog, u32 bit, bool set)
 {
-	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
+	const struct crashlog_control *control = &crashlog->info->control;
+	struct intel_pmt_entry *entry = &crashlog->entry;
+	u32 reg = readl(entry->disc_table + control->offset);
 
-	reg &= ~CRASHLOG_FLAG_TRIGGER_MASK;
+	reg &= ~control->trigger_mask;
 
 	if (set)
 		reg |= bit;
 	else
 		reg &= bit;
 
-	writel(reg, entry->disc_table + CONTROL_OFFSET);
+	writel(reg, entry->disc_table + control->offset);
 }
 
-static bool read_check(struct intel_pmt_entry *entry, u32 bit)
+static bool read_check(struct crashlog_entry *crashlog, u32 bit)
 {
-	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
+	const struct crashlog_status *status = &crashlog->info->status;
+	u32 reg = readl(crashlog->entry.disc_table + status->offset);
 
 	return !!(reg & bit);
 }
 
-static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
+static bool pmt_crashlog_complete(struct crashlog_entry *crashlog)
 {
 	/* return current value of the crashlog complete flag */
-	return read_check(entry, CRASHLOG_FLAG_TRIGGER_COMPLETE);
+	return read_check(crashlog, crashlog->info->status.complete);
 }
 
-static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry)
+static bool pmt_crashlog_disabled(struct crashlog_entry *crashlog)
 {
 	/* return current value of the crashlog disabled flag */
-	return read_check(entry, CRASHLOG_FLAG_DISABLE);
+	return read_check(crashlog, crashlog->info->status.disabled);
 }
 
-static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
+static void pmt_crashlog_set_disable(struct crashlog_entry *crashlog, bool disable)
 {
-	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
-	u32 crash_type, version;
-
-	crash_type = GET_TYPE(discovery_header);
-	version = GET_VERSION(discovery_header);
-
-	/*
-	 * Currently we only recognize OOBMSM version 0 devices.
-	 * We can ignore all other crashlog devices in the system.
-	 */
-	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
+	read_modify_write(crashlog, crashlog->info->control.disable, disable);
 }
 
-static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
-				     bool disable)
+static void pmt_crashlog_set_clear(struct crashlog_entry *crashlog)
 {
-	read_modify_write(entry, CRASHLOG_FLAG_DISABLE, disable);
+	read_modify_write(crashlog, crashlog->info->control.clear, SET);
 }
 
-static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
+static void pmt_crashlog_set_execute(struct crashlog_entry *crashlog)
 {
-	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_CLEAR, SET);
-}
-
-static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
-{
-	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_EXECUTE, SET);
+	read_modify_write(crashlog, crashlog->info->control.manual, SET);
 }
 
 /*
@@ -136,8 +176,8 @@ static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
 static ssize_t
 enable_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-	struct intel_pmt_entry *entry = dev_get_drvdata(dev);
-	bool enabled = !pmt_crashlog_disabled(entry);
+	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
+	bool enabled = !pmt_crashlog_disabled(crashlog);
 
 	return sprintf(buf, "%d\n", enabled);
 }
@@ -146,19 +186,19 @@ static ssize_t
 enable_store(struct device *dev, struct device_attribute *attr,
 	     const char *buf, size_t count)
 {
-	struct crashlog_entry *entry;
+	struct crashlog_entry *crashlog;
 	bool enabled;
 	int result;
 
-	entry = dev_get_drvdata(dev);
+	crashlog = dev_get_drvdata(dev);
 
 	result = kstrtobool(buf, &enabled);
 	if (result)
 		return result;
 
-	guard(mutex)(&entry->control_mutex);
+	guard(mutex)(&crashlog->control_mutex);
 
-	pmt_crashlog_set_disable(&entry->entry, !enabled);
+	pmt_crashlog_set_disable(crashlog, !enabled);
 
 	return count;
 }
@@ -167,11 +207,11 @@ static DEVICE_ATTR_RW(enable);
 static ssize_t
 trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-	struct intel_pmt_entry *entry;
+	struct crashlog_entry *crashlog;
 	bool trigger;
 
-	entry = dev_get_drvdata(dev);
-	trigger = pmt_crashlog_complete(entry);
+	crashlog = dev_get_drvdata(dev);
+	trigger = pmt_crashlog_complete(crashlog);
 
 	return sprintf(buf, "%d\n", trigger);
 }
@@ -180,32 +220,33 @@ static ssize_t
 trigger_store(struct device *dev, struct device_attribute *attr,
 	      const char *buf, size_t count)
 {
-	struct crashlog_entry *entry;
+	struct crashlog_entry *crashlog;
 	bool trigger;
 	int result;
 
-	entry = dev_get_drvdata(dev);
+	crashlog = dev_get_drvdata(dev);
 
 	result = kstrtobool(buf, &trigger);
 	if (result)
 		return result;
 
-	guard(mutex)(&entry->control_mutex);
+	guard(mutex)(&crashlog->control_mutex);
 
 	/* if device is currently disabled, return busy */
-	if (pmt_crashlog_disabled(&entry->entry))
+	if (pmt_crashlog_disabled(crashlog))
 		return -EBUSY;
 
 	if (!trigger) {
-		pmt_crashlog_set_clear(&entry->entry);
+		pmt_crashlog_set_clear(crashlog);
 		return count;
 	}
 
 	/* we cannot trigger a new crash if one is still pending */
-	if (pmt_crashlog_complete(&entry->entry))
+	if (pmt_crashlog_complete(crashlog))
 		return -EEXIST;
 
-	pmt_crashlog_set_execute(&entry->entry);
+	pmt_crashlog_set_execute(crashlog);
+
 
 	return count;
 }
@@ -231,9 +272,10 @@ static int pmt_crashlog_header_decode(struct intel_pmt_entry *entry,
 	if (!pmt_crashlog_supported(entry))
 		return 1;
 
-	/* initialize control mutex */
+	/* initialize the crashlog struct */
 	crashlog = container_of(entry, struct crashlog_entry, entry);
 	mutex_init(&crashlog->control_mutex);
+	crashlog->info = &crashlog_type1_ver0;
 
 	header->access_type = GET_ACCESS(readl(disc_table));
 	header->guid = readl(disc_table + GUID_OFFSET);
-- 
2.49.0

Re: [PATCH v4 09/10] platform/x86/intel/pmt: use a version struct

[Ilpo Järvinen]

On Tue, 10 Jun 2025, Michael J. Ruhl wrote:

> In preparation for supporting multiple crashlog versions, use a struct
> to keep bit offset info for the status and control bits.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 174 ++++++++++++++--------
>  1 file changed, 108 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index e11865686f2a..7c259b1cf95b 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -23,21 +23,6 @@
>  /* Crashlog discovery header types */
>  #define CRASH_TYPE_OOBMSM	1
>  
> -/* Control Flags */
> -#define CRASHLOG_FLAG_DISABLE		BIT(28)
> -
> -/*
> - * Bits 29 and 30 control the state of bit 31.
> - *
> - * Bit 29 will clear bit 31, if set, allowing a new crashlog to be captured.
> - * Bit 30 will immediately trigger a crashlog to be generated, setting bit 31.
> - * Bit 31 is the read-only status with a 1 indicating log is complete.
> - */
> -#define CRASHLOG_FLAG_TRIGGER_CLEAR	BIT(29)
> -#define CRASHLOG_FLAG_TRIGGER_EXECUTE	BIT(30)
> -#define CRASHLOG_FLAG_TRIGGER_COMPLETE	BIT(31)
> -#define CRASHLOG_FLAG_TRIGGER_MASK	GENMASK(31, 28)
> -
>  /* Crashlog Discovery Header */
>  #define CONTROL_OFFSET		0x0
>  #define GUID_OFFSET		0x4
> @@ -49,10 +34,63 @@
>  /* size is in bytes */
>  #define GET_SIZE(v)		((v) * sizeof(u32))
>  
> +/*
> + * Type 1 Version 0
> + * status and control registers are combined.
> + *
> + * Bits 29 and 30 control the state of bit 31.
> + * Bit 29 will clear bit 31, if set, allowing a new crashlog to be captured.
> + * Bit 30 will immediately trigger a crashlog to be generated, setting bit 31.
> + * Bit 31 is the read-only status with a 1 indicating log is complete.
> + */
> +#define TYPE1_VER0_STATUS_OFFSET	0x00
> +#define TYPE1_VER0_CONTROL_OFFSET	0x00
> +
> +#define TYPE1_VER0_DISABLE		BIT(28)
> +#define TYPE1_VER0_CLEAR		BIT(29)
> +#define TYPE1_VER0_EXECUTE		BIT(30)
> +#define TYPE1_VER0_COMPLETE		BIT(31)
> +#define TYPE1_VER0_TRIGGER_MASK		GENMASK(31, 28)
> +
> +/* After offset, order alphabetically, not bit ordered */
> +struct crashlog_status {
> +	u32 offset;
> +	u32 cleared;
> +	u32 complete;
> +	u32 disabled;
> +};
> +
> +struct crashlog_control {
> +	u32 offset;
> +	u32 trigger_mask;
> +	u32 clear;
> +	u32 disable;
> +	u32 manual;
> +};
> +
> +struct crashlog_info {
> +	struct crashlog_status status;
> +	struct crashlog_control control;
> +};
> +
> +static const struct crashlog_info crashlog_type1_ver0 = {
> +	.status.offset = TYPE1_VER0_STATUS_OFFSET,
> +	.status.cleared = TYPE1_VER0_CLEAR,
> +	.status.complete = TYPE1_VER0_COMPLETE,
> +	.status.disabled = TYPE1_VER0_DISABLE,
> +
> +	.control.offset = TYPE1_VER0_CONTROL_OFFSET,
> +	.control.trigger_mask = TYPE1_VER0_TRIGGER_MASK,
> +	.control.clear = TYPE1_VER0_CLEAR,
> +	.control.disable = TYPE1_VER0_DISABLE,
> +	.control.manual = TYPE1_VER0_EXECUTE,
> +};
> +
>  struct crashlog_entry {
>  	/* entry must be first member of struct */
>  	struct intel_pmt_entry		entry;
>  	struct mutex			control_mutex;
> +	const struct crashlog_info	*info;
>  };
>  
>  struct pmt_crashlog_priv {
> @@ -60,74 +98,76 @@ struct pmt_crashlog_priv {
>  	struct crashlog_entry	entry[];
>  };
>  
> +static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
> +{
> +	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
> +	u32 crash_type, version;
> +
> +	crash_type = GET_TYPE(discovery_header);
> +	version = GET_VERSION(discovery_header);
> +
> +	/*
> +	 * Currently we only recognize OOBMSM version 0 devices.
> +	 * We can ignore all other crashlog devices in the system.
> +	 */
> +	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
> +}
> +
>  /*
>   * I/O
>   */
> -#define SET	true
> -#define CLEAR	false
> +#define SET     true
> +#define CLEAR   false

Unrelated space change, and the pre-spacing with tabs is better.

> -static void read_modify_write(struct intel_pmt_entry *entry, u32 bit, bool set)
> +static void read_modify_write(struct crashlog_entry *crashlog, u32 bit, bool set)
>  {
> -	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
> +	const struct crashlog_control *control = &crashlog->info->control;
> +	struct intel_pmt_entry *entry = &crashlog->entry;
> +	u32 reg = readl(entry->disc_table + control->offset);
>  
> -	reg &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> +	reg &= ~control->trigger_mask;
>  
>  	if (set)
>  		reg |= bit;
>  	else
>  		reg &= bit;
>  
> -	writel(reg, entry->disc_table + CONTROL_OFFSET);
> +	writel(reg, entry->disc_table + control->offset);
>  }
>  
> -static bool read_check(struct intel_pmt_entry *entry, u32 bit)
> +static bool read_check(struct crashlog_entry *crashlog, u32 bit)
>  {
> -	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
> +	const struct crashlog_status *status = &crashlog->info->status;
> +	u32 reg = readl(crashlog->entry.disc_table + status->offset);
>  
>  	return !!(reg & bit);
>  }
>  
> -static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
> +static bool pmt_crashlog_complete(struct crashlog_entry *crashlog)
>  {
>  	/* return current value of the crashlog complete flag */
> -	return read_check(entry, CRASHLOG_FLAG_TRIGGER_COMPLETE);
> +	return read_check(crashlog, crashlog->info->status.complete);
>  }
>  
> -static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry)
> +static bool pmt_crashlog_disabled(struct crashlog_entry *crashlog)
>  {
>  	/* return current value of the crashlog disabled flag */
> -	return read_check(entry, CRASHLOG_FLAG_DISABLE);
> +	return read_check(crashlog, crashlog->info->status.disabled);
>  }
>  
> -static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
> +static void pmt_crashlog_set_disable(struct crashlog_entry *crashlog, bool disable)
>  {
> -	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
> -	u32 crash_type, version;
> -
> -	crash_type = GET_TYPE(discovery_header);
> -	version = GET_VERSION(discovery_header);
> -
> -	/*
> -	 * Currently we only recognize OOBMSM version 0 devices.
> -	 * We can ignore all other crashlog devices in the system.
> -	 */
> -	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
> +	read_modify_write(crashlog, crashlog->info->control.disable, disable);
>  }
>  
> -static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
> -				     bool disable)
> +static void pmt_crashlog_set_clear(struct crashlog_entry *crashlog)
>  {
> -	read_modify_write(entry, CRASHLOG_FLAG_DISABLE, disable);
> +	read_modify_write(crashlog, crashlog->info->control.clear, SET);
>  }
>  
> -static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
> +static void pmt_crashlog_set_execute(struct crashlog_entry *crashlog)
>  {
> -	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_CLEAR, SET);
> -}
> -
> -static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
> -{
> -	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_EXECUTE, SET);
> +	read_modify_write(crashlog, crashlog->info->control.manual, SET);
>  }
>  
>  /*
> @@ -136,8 +176,8 @@ static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
>  static ssize_t
>  enable_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> -	struct intel_pmt_entry *entry = dev_get_drvdata(dev);
> -	bool enabled = !pmt_crashlog_disabled(entry);
> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> +	bool enabled = !pmt_crashlog_disabled(crashlog);
>  
>  	return sprintf(buf, "%d\n", enabled);
>  }
> @@ -146,19 +186,19 @@ static ssize_t
>  enable_store(struct device *dev, struct device_attribute *attr,
>  	     const char *buf, size_t count)
>  {
> -	struct crashlog_entry *entry;
> +	struct crashlog_entry *crashlog;
>  	bool enabled;
>  	int result;
>  
> -	entry = dev_get_drvdata(dev);
> +	crashlog = dev_get_drvdata(dev);
>  
>  	result = kstrtobool(buf, &enabled);
>  	if (result)
>  		return result;
>  
> -	guard(mutex)(&entry->control_mutex);
> +	guard(mutex)(&crashlog->control_mutex);
>  
> -	pmt_crashlog_set_disable(&entry->entry, !enabled);
> +	pmt_crashlog_set_disable(crashlog, !enabled);
>  
>  	return count;
>  }
> @@ -167,11 +207,11 @@ static DEVICE_ATTR_RW(enable);
>  static ssize_t
>  trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> -	struct intel_pmt_entry *entry;
> +	struct crashlog_entry *crashlog;
>  	bool trigger;
>  
> -	entry = dev_get_drvdata(dev);
> -	trigger = pmt_crashlog_complete(entry);
> +	crashlog = dev_get_drvdata(dev);
> +	trigger = pmt_crashlog_complete(crashlog);
>  
>  	return sprintf(buf, "%d\n", trigger);
>  }
> @@ -180,32 +220,33 @@ static ssize_t
>  trigger_store(struct device *dev, struct device_attribute *attr,
>  	      const char *buf, size_t count)
>  {
> -	struct crashlog_entry *entry;
> +	struct crashlog_entry *crashlog;
>  	bool trigger;
>  	int result;
>  
> -	entry = dev_get_drvdata(dev);
> +	crashlog = dev_get_drvdata(dev);
>  
>  	result = kstrtobool(buf, &trigger);
>  	if (result)
>  		return result;
>  
> -	guard(mutex)(&entry->control_mutex);
> +	guard(mutex)(&crashlog->control_mutex);

Could you please do the entry -> crashlog variable rename first in a 
separate patch for the cases where it's already struct crashlog_entry to 
keep this patch focused on real change.
 
>  	/* if device is currently disabled, return busy */
> -	if (pmt_crashlog_disabled(&entry->entry))
> +	if (pmt_crashlog_disabled(crashlog))
>  		return -EBUSY;
>  
>  	if (!trigger) {
> -		pmt_crashlog_set_clear(&entry->entry);
> +		pmt_crashlog_set_clear(crashlog);
>  		return count;
>  	}
>  
>  	/* we cannot trigger a new crash if one is still pending */
> -	if (pmt_crashlog_complete(&entry->entry))
> +	if (pmt_crashlog_complete(crashlog))
>  		return -EEXIST;
>  
> -	pmt_crashlog_set_execute(&entry->entry);
> +	pmt_crashlog_set_execute(crashlog);
> +
>  
>  	return count;
>  }
> @@ -231,9 +272,10 @@ static int pmt_crashlog_header_decode(struct intel_pmt_entry *entry,
>  	if (!pmt_crashlog_supported(entry))
>  		return 1;
>  
> -	/* initialize control mutex */
> +	/* initialize the crashlog struct */
>  	crashlog = container_of(entry, struct crashlog_entry, entry);
>  	mutex_init(&crashlog->control_mutex);

Unrelated to this patch, there seems to be no mutex_destroy() done for 
this mutex anywhere.

> +	crashlog->info = &crashlog_type1_ver0;
>  
>  	header->access_type = GET_ACCESS(readl(disc_table));
>  	header->guid = readl(disc_table + GUID_OFFSET);
> 

-- 
 i.

[PATCH v4 10/10] 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/crashlog.c | 269 ++++++++++++++++++++--
 1 file changed, 256 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 7c259b1cf95b..17a49c08e911 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -52,20 +52,53 @@
 #define TYPE1_VER0_COMPLETE		BIT(31)
 #define TYPE1_VER0_TRIGGER_MASK		GENMASK(31, 28)
 
+/*
+ * Type 1 Version 2
+ * status and control are two different registers
+ */
+#define TYPE1_VER2_STATUS_OFFSET	0x00
+#define TYPE1_VER2_CONTROL_OFFSET	0x14
+
+/* status register */
+#define TYPE1_VER2_CLEAR_SUPPORT	BIT(20)
+#define TYPE1_VER2_REARMED		BIT(25)
+#define TYPE1_VER2_ERROR		BIT(26)
+#define TYPE1_VER2_CONSUMED		BIT(27)
+#define TYPE1_VER2_DISABLED		BIT(28)
+#define TYPE1_VER2_CLEARED		BIT(29)
+#define TYPE1_VER2_IN_PROGRESS		BIT(30)
+#define TYPE1_VER2_COMPLETE		BIT(31)
+
+/* control register */
+#define TYPE1_VER2_CONSUME		BIT(25)
+#define TYPE1_VER2_REARM		BIT(28)
+#define TYPE1_VER2_EXECUTE		BIT(29)
+#define TYPE1_VER2_CLEAR		BIT(30)
+#define TYPE1_VER2_DISABLE		BIT(31)
+#define TYPE1_VER2_TRIGGER_MASK		(TYPE1_VER2_CONSUME | TYPE1_VER2_EXECUTE | \
+					 TYPE1_VER2_CLEAR | TYPE1_VER2_DISABLE)
+
 /* After offset, order alphabetically, not bit ordered */
 struct crashlog_status {
 	u32 offset;
+	u32 clear_supported;
 	u32 cleared;
 	u32 complete;
+	u32 consumed;
 	u32 disabled;
+	u32 error;
+	u32 in_progress;
+	u32 rearmed;
 };
 
 struct crashlog_control {
 	u32 offset;
 	u32 trigger_mask;
 	u32 clear;
+	u32 consume;
 	u32 disable;
 	u32 manual;
+	u32 rearm;
 };
 
 struct crashlog_info {
@@ -86,6 +119,26 @@ static const struct crashlog_info crashlog_type1_ver0 = {
 	.control.manual = TYPE1_VER0_EXECUTE,
 };
 
+const struct crashlog_info crashlog_type1_ver2 = {
+	.status.offset = TYPE1_VER2_STATUS_OFFSET,
+	.status.clear_supported = TYPE1_VER2_CLEAR_SUPPORT,
+	.status.cleared = TYPE1_VER2_CLEARED,
+	.status.complete = TYPE1_VER2_COMPLETE,
+	.status.consumed = TYPE1_VER2_CONSUMED,
+	.status.disabled = TYPE1_VER2_DISABLED,
+	.status.error = TYPE1_VER2_ERROR,
+	.status.in_progress = TYPE1_VER2_IN_PROGRESS,
+	.status.rearmed = TYPE1_VER2_REARMED,
+
+	.control.offset = TYPE1_VER2_CONTROL_OFFSET,
+	.control.trigger_mask = TYPE1_VER2_TRIGGER_MASK,
+	.control.clear = TYPE1_VER2_CLEAR,
+	.control.consume = TYPE1_VER2_CONSUME,
+	.control.disable = TYPE1_VER2_DISABLE,
+	.control.manual = TYPE1_VER2_EXECUTE,
+	.control.rearm = TYPE1_VER2_REARM,
+};
+
 struct crashlog_entry {
 	/* entry must be first member of struct */
 	struct intel_pmt_entry		entry;
@@ -98,19 +151,23 @@ struct pmt_crashlog_priv {
 	struct crashlog_entry	entry[];
 };
 
-static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
+static bool pmt_crashlog_supported(struct intel_pmt_entry *entry, u32 *crash_type, u32 *version)
 {
 	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
-	u32 crash_type, version;
 
-	crash_type = GET_TYPE(discovery_header);
-	version = GET_VERSION(discovery_header);
+	*crash_type = GET_TYPE(discovery_header);
+	*version = GET_VERSION(discovery_header);
 
 	/*
-	 * Currently we only recognize OOBMSM version 0 devices.
-	 * We can ignore all other crashlog devices in the system.
+	 * Currently we only recognize OOBMSM (type 1) and version 0 or 2
+	 * devices.
+	 *
+	 * Ignore all other crashlog devices in the system.
 	 */
-	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
+	if (*crash_type == CRASH_TYPE_OOBMSM && (*version == 0 || *version == 2))
+		return true;
+
+	return false;
 }
 
 /*
@@ -170,9 +227,118 @@ static void pmt_crashlog_set_execute(struct crashlog_entry *crashlog)
 	read_modify_write(crashlog, crashlog->info->control.manual, SET);
 }
 
+static bool pmt_crashlog_cleared(struct crashlog_entry *crashlog)
+{
+	/* return current value of the crashlog cleared flag */
+	return read_check(crashlog, crashlog->info->status.cleared);
+}
+
+static bool pmt_crashlog_consumed(struct crashlog_entry *crashlog)
+{
+	/* return current value of the crashlog consumedflag */
+	return read_check(crashlog, crashlog->info->status.consumed);
+}
+
+static void pmt_crashlog_set_consumed(struct crashlog_entry *crashlog)
+{
+	read_modify_write(crashlog, crashlog->info->control.consume, SET);
+}
+
+static bool pmt_crashlog_error(struct crashlog_entry *crashlog)
+{
+	/* return current value of the crashlog error flag */
+	return read_check(crashlog, crashlog->info->status.error);
+}
+
+static bool pmt_crashlog_rearm(struct crashlog_entry *crashlog)
+{
+	/* return current value of the crashlog reamed flag */
+	return read_check(crashlog, crashlog->info->status.rearmed);
+}
+
+static void pmt_crashlog_set_rearm(struct crashlog_entry *crashlog)
+{
+	read_modify_write(crashlog, crashlog->info->control.rearm, SET);
+}
+
 /*
  * sysfs
  */
+static ssize_t
+clear_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
+	bool cleared = pmt_crashlog_cleared(crashlog);
+
+	return sysfs_emit(buf, "%d\n", cleared);
+}
+
+static ssize_t
+clear_store(struct device *dev, struct device_attribute *attr,
+	    const char *buf, size_t count)
+{
+	struct crashlog_entry *crashlog;
+	bool clear;
+	int result;
+
+	crashlog = dev_get_drvdata(dev);
+
+	result = kstrtobool(buf, &clear);
+	if (result)
+		return result;
+
+	/* set bit only */
+	if (!clear)
+		return -EINVAL;
+
+	guard(mutex)(&crashlog->control_mutex);
+
+	pmt_crashlog_set_clear(crashlog);
+
+	return count;
+}
+static DEVICE_ATTR_RW(clear);
+
+static ssize_t
+consumed_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
+	bool consumed = pmt_crashlog_consumed(crashlog);
+
+	return sysfs_emit(buf, "%d\n", consumed);
+}
+
+static ssize_t consumed_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct crashlog_entry *crashlog;
+	bool consumed;
+	int result;
+
+	crashlog = dev_get_drvdata(dev);
+
+	result = kstrtobool(buf, &consumed);
+	if (result)
+		return result;
+
+	/* set bit only */
+	if (!consumed)
+		return -EINVAL;
+
+	guard(mutex)(&crashlog->control_mutex);
+
+	if (pmt_crashlog_disabled(crashlog))
+		return -EBUSY;
+
+	if (!pmt_crashlog_complete(crashlog))
+		return -EEXIST;
+
+	pmt_crashlog_set_consumed(crashlog);
+
+	return count;
+}
+static DEVICE_ATTR_RW(consumed);
+
 static ssize_t
 enable_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -204,6 +370,50 @@ enable_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(enable);
 
+static ssize_t
+error_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
+	bool error = pmt_crashlog_error(crashlog);
+
+	return sysfs_emit(buf, "%d\n", error);
+}
+static DEVICE_ATTR_RO(error);
+
+static ssize_t
+rearm_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
+	int rearmed = pmt_crashlog_rearm(crashlog);
+
+	return sysfs_emit(buf, "%d\n", rearmed);
+}
+
+static ssize_t rearm_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct crashlog_entry *crashlog;
+	bool rearm;
+	int result;
+
+	crashlog = dev_get_drvdata(dev);
+
+	result = kstrtobool(buf, &rearm);
+	if (result)
+		return result;
+
+	/* set only */
+	if (!rearm)
+		return -EINVAL;
+
+	guard(mutex)(&crashlog->control_mutex);
+
+	pmt_crashlog_set_rearm(crashlog);
+
+	return count;
+}
+static DEVICE_ATTR_RW(rearm);
+
 static ssize_t
 trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -252,30 +462,63 @@ trigger_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(trigger);
 
-static struct attribute *pmt_crashlog_attrs[] = {
+static struct attribute *pmt_crashlog_type1_ver0_attrs[] = {
 	&dev_attr_enable.attr,
 	&dev_attr_trigger.attr,
 	NULL
 };
 
-static const struct attribute_group pmt_crashlog_group = {
-	.attrs	= pmt_crashlog_attrs,
+static struct attribute *pmt_crashlog_type1_ver2_attrs[] = {
+	&dev_attr_clear.attr,
+	&dev_attr_consumed.attr,
+	&dev_attr_enable.attr,
+	&dev_attr_error.attr,
+	&dev_attr_rearm.attr,
+	&dev_attr_trigger.attr,
+	NULL
+};
+
+static const struct attribute_group pmt_crashlog_type1_ver0_group = {
+	.attrs	= pmt_crashlog_type1_ver0_attrs,
 };
 
+static const struct attribute_group pmt_crashlog_type1_ver2_group = {
+	.attrs = pmt_crashlog_type1_ver2_attrs,
+};
+
+static const struct crashlog_info *select_crashlog_info(u32 type, u32 version)
+{
+	if (version == 0)
+		return &crashlog_type1_ver0;
+
+	return &crashlog_type1_ver2;
+}
+
+static const struct attribute_group *select_sysfs_grp(u32 type, u32 version)
+{
+	if (version == 0)
+		return &pmt_crashlog_type1_ver2_group;
+
+	return &pmt_crashlog_type1_ver2_group;
+}
+
 static int pmt_crashlog_header_decode(struct intel_pmt_entry *entry,
 				      struct device *dev)
 {
 	void __iomem *disc_table = entry->disc_table;
 	struct intel_pmt_header *header = &entry->header;
 	struct crashlog_entry *crashlog;
+	u32 version;
+	u32 type;
 
-	if (!pmt_crashlog_supported(entry))
+	if (!pmt_crashlog_supported(entry, &type, &version))
 		return 1;
 
 	/* initialize the crashlog struct */
 	crashlog = container_of(entry, struct crashlog_entry, entry);
 	mutex_init(&crashlog->control_mutex);
-	crashlog->info = &crashlog_type1_ver0;
+
+	crashlog->info = select_crashlog_info(type, version);
 
 	header->access_type = GET_ACCESS(readl(disc_table));
 	header->guid = readl(disc_table + GUID_OFFSET);
@@ -284,7 +527,7 @@ static int pmt_crashlog_header_decode(struct intel_pmt_entry *entry,
 	/* Size is measured in DWORDS, but accessor returns bytes */
 	header->size = GET_SIZE(readl(disc_table + SIZE_OFFSET));
 
-	entry->attr_grp = &pmt_crashlog_group;
+	entry->attr_grp = select_sysfs_grp(type, version);
 
 	return 0;
 }
-- 
2.49.0