RE: [PATCH v3 02/10] platform/x86/intel/pmt: Add register access callbacks

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

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

On Wed, 26 Aug 2026, Ruhl, Michael J wrote:

> >-----Original Message-----
> >From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >Sent: Tuesday, August 25, 2026 5:35 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 <hansg@kernel.org>; Brost, Matthew <matthew.brost@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; Vijay,
> >Anoop C <anoop.c.vijay@intel.com>; Nilawar, Badal
> ><badal.nilawar@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>;
> >Ausmus, James <james.ausmus@intel.com>; Poosa, Karthik
> ><karthik.poosa@intel.com>
> >Subject: Re: [PATCH v3 02/10] platform/x86/intel/pmt: Add register access
> >callbacks
> >
> >On Mon, 24 Aug 2026, Michael J. Ruhl wrote:
> >
> >> Some HW does not have direct MMIO access to PMT control and data
> >> features.
> >>
> >> Augment the current callback infrastructure (data access) to allow
> >> a registered driver to customize read/write access to the control
> >> paths for PMT usage.
> >>
> >> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> >> ---
> >>  drivers/platform/x86/intel/pmt/crashlog.c | 39 +++++++++++++++++++++-
> >-
> >>  include/linux/intel_vsec.h                | 14 +++++++-
> >>  2 files changed, 49 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
> >b/drivers/platform/x86/intel/pmt/crashlog.c
> >> index f936daf99e4d..5923ad7abbd9 100644
> >> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> >> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> >> @@ -129,7 +129,19 @@ static void pmt_crashlog_rmw(struct
> >crashlog_entry *crashlog, u32 bit, bool set)
> >>  {
> >>  	const struct crashlog_control *control = &crashlog->info->control;
> >>  	struct intel_pmt_entry *entry = &crashlog->entry;
> >> -	u32 reg = readl(entry->disc_table + control->offset);
> >> +	u32 guid = entry->header.guid;
> >> +	u32 reg;
> >> +	int err;
> >> +
> >> +	if (entry->cb && entry->cb->read_reg) {
> >> +		err = entry->cb->read_reg(entry->dev, guid, &reg, control-
> >>offset);
> >> +		if (err) {
> >> +			pr_err("%s: failed to read reg: %d\n", __func__, err);
> >
> >Never print __func__ in any user consumable message (level > debug) but
> >write the message in plain English.
> >
> >Also, this needs include.
> >
> >> +			return;
> >> +		}
> >> +	} else {
> >> +		reg = readl(entry->disc_table + control->offset);
> >
> >Add include.
> 
> I have added the linux/printk.h include... I am not clear on the correct readl inclued.
> 
> Should this be <linux/io.h> or <include/asm-generic/io.h>?

Usually it's better to use linux/ one, except in headers that are used 
in many .c files where it may be helpful to use as precise include as 
possible to limit the number of things one include ends up pulling in.

> >> +	}
> >>
> >>  	reg &= ~control->trigger_mask;
> >>
> >> @@ -138,14 +150,35 @@ static void pmt_crashlog_rmw(struct
> >crashlog_entry *crashlog, u32 bit, bool set)
> >>  	else
> >>  		reg &= ~bit;
> >>
> >> -	writel(reg, entry->disc_table + control->offset);
> >> +	if (entry->cb && entry->cb->write_reg) {
> >> +		err = entry->cb->write_reg(entry->dev, guid, reg, control-
> >>offset);
> >> +		if (err) {
> >> +			pr_err("%s: failed to write reg: %d\n", __func__, err);
> >
> >Rephrase without using __func__.
> 
> This was for my internal debugging and should have been cleaned up.  Removed.
> 
> >> +			return;
> >> +		}
> >> +	} else {
> >> +		writel(reg, entry->disc_table + control->offset);
> >> +	}
> >>  }
> >>
> >>  /* Read the status register and see if the specified @bit is set */
> >>  static bool pmt_crashlog_rc(struct crashlog_entry *crashlog, u32 bit)
> >>  {
> >>  	const struct crashlog_status *status = &crashlog->info->status;
> >> -	u32 reg = readl(crashlog->entry.disc_table + status->offset);
> >> +	struct intel_pmt_entry *entry = &crashlog->entry;
> >> +	u32 guid = entry->header.guid;
> >> +	u32 reg;
> >> +	int err;
> >> +
> >> +	if (entry->cb && entry->cb->read_reg) {
> >> +		err = entry->cb->read_reg(entry->dev, guid, &reg, status-
> >>offset);
> >> +		if (err) {
> >> +			pr_err("%s: failed to read reg: %d\n", __func__, err);
> >> +			return false;
> >
> >It seems you need a lerger rework here to properly return error codes.
> 
> Yes, there would be a significant rework (almost all internal functions 
> will need to be updated) to return the error codes.  This seems out of 
> scope for these updates.

Your series adds a call that can fail (before there wasn't one) so it 
should be part of this effort. bool -> int conversion should be mostly 
simple (though admittedly a bit tedious) as it mainly just adds the 
error handling ifs to the callchains.

> Does this need to be addressed to make progress on this patch set?  Or can we work on that
> in the future?

I think it should be part of this effort because of the forementioned 
reason.


-- 
 i.