Re: [PATCH v6 10/12] platform/x86/intel/pmt: refactor base parameter

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

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

On Thu, 3 Jul 2025, Michael J. Ruhl wrote:

> For the crashlog driver, struct crashlog_entry is the parent of
> struct intel_pmt_entry. To support multiple crashlog versions, most

To support an upcoming change ...

> accesses will be to the struct crashlog_entry.

I don't understand the meaning of the part after the comma as "most 
accesses" is extremely vague, could you please rephrase it.

Jumping directly to the itemized list at this point feels a bit clumsy as 
they don't really connect well.

> - Refactor to use struct crashlog_entry in place of
>   struct intel_pmt_entry
> - Rename some usages (auto-variables) from entry to crashlog
>
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 59 ++++++++++++-----------
>  1 file changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index 888946a8ba46..8cca520c5a1c 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -68,8 +68,9 @@ struct pmt_crashlog_priv {
>  #define CRASHLOG_CLEAR_BIT	false
>  
>  /* read/modify/write */
> -static void pmt_crashlog_rmw(struct intel_pmt_entry *entry, u32 bit, bool set)
> +static void pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
>  {
> +	struct intel_pmt_entry *entry = &crashlog->entry;
>  	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
>  
>  	reg &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> @@ -83,23 +84,24 @@ static void pmt_crashlog_rmw(struct intel_pmt_entry *entry, u32 bit, bool set)
>  }
>  
>  /* read/check */
> -static bool pmt_crashlog_rc(struct intel_pmt_entry *entry, u32 bit)
> +static bool pmt_crashlog_rc(struct crashlog_entry *crashlog, u32 bit)
>  {
> +	struct intel_pmt_entry *entry = &crashlog->entry;
>  	u32 reg = readl(entry->disc_table + CONTROL_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 pmt_crashlog_rc(entry, CRASHLOG_FLAG_TRIGGER_COMPLETE);
> +	return pmt_crashlog_rc(crashlog, CRASHLOG_FLAG_TRIGGER_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 pmt_crashlog_rc(entry, CRASHLOG_FLAG_DISABLE);
> +	return pmt_crashlog_rc(crashlog, CRASHLOG_FLAG_DISABLE);
>  }
>  
>  static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
> @@ -117,20 +119,19 @@ static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
>  	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
>  }
>  
> -static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
> -				     bool disable)
> +static void pmt_crashlog_set_disable(struct crashlog_entry *crashlog, bool disable)
>  {
> -	pmt_crashlog_rmw(entry, CRASHLOG_FLAG_DISABLE, disable);
> +	pmt_crashlog_rmw(crashlog, CRASHLOG_FLAG_DISABLE, disable);
>  }
>  
> -static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
> +static void pmt_crashlog_set_clear(struct crashlog_entry *crashlog)
>  {
> -	pmt_crashlog_rmw(entry, CRASHLOG_FLAG_TRIGGER_CLEAR, CRASHLOG_SET_BIT);
> +	pmt_crashlog_rmw(crashlog, CRASHLOG_FLAG_TRIGGER_CLEAR, CRASHLOG_SET_BIT);
>  }
>  
> -static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
> +static void pmt_crashlog_set_execute(struct crashlog_entry *crashlog)
>  {
> -	pmt_crashlog_rmw(entry, CRASHLOG_FLAG_TRIGGER_EXECUTE, CRASHLOG_SET_BIT);
> +	pmt_crashlog_rmw(crashlog, CRASHLOG_FLAG_TRIGGER_EXECUTE, CRASHLOG_SET_BIT);
>  }
>  
>  /*
> @@ -139,8 +140,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);
>  }
> @@ -149,19 +150,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;
>  }
> @@ -170,11 +171,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);
>  }
> @@ -183,32 +184,32 @@ static ssize_t
>  trigger_store(struct device *dev, struct device_attribute *attr,
>  	      const char *buf, size_t count)
>  {
> -	struct crashlog_entry *entry;
> +	struct crashlog_entry *crashlog;
>  	bool trigger;
>  	int result;
>  
> -	entry = dev_get_drvdata(dev);
> +	crashlog = dev_get_drvdata(dev);
>  
>  	result = kstrtobool(buf, &trigger);
>  	if (result)
>  		return result;
>  
> -	guard(mutex)(&entry->control_mutex);
> +	guard(mutex)(&crashlog->control_mutex);
>  
>  	/* if 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;
>  }
> 

Once you've improved the changelog, feel free to add:

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

-- 
 i.