Re: [PATCH v8 03/20] platform/x86/intel/pmt: refactor rc with a return value

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Sep 11, 2026 at 01:11:51PM -0700, Michael J. Ruhl wrote:
> Upcoming changes will include possible failures from HW accesses.
> 
> Refactor pmt_crashlog_rw() with a return value.
> 
> Update all necessary usage to use the return code.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 101 ++++++++++++++++------
>  1 file changed, 74 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index 831cff743803..b6540f3a9a17 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -144,27 +144,29 @@ static int pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
>  }
>  
>  /* Read the status register and see if the specified @bit is set */
> -static bool pmt_crashlog_rc(struct crashlog_entry *crashlog, u32 bit)
> +static int pmt_crashlog_rc(struct crashlog_entry *crashlog, u32 bit, bool *state)
>  {
>  	const struct crashlog_status *status = &crashlog->info->status;
>  	u32 reg = readl(crashlog->entry.disc_table + status->offset);
>  
> -	return !!(reg & bit);
> +	*state = !!(reg & bit);
> +
> +	return 0;
>  }
>  
> -static bool pmt_crashlog_complete(struct crashlog_entry *crashlog)
> +static int pmt_crashlog_complete(struct crashlog_entry *crashlog, bool *state)
>  {
>  	/* return current value of the crashlog complete flag */
> -	return pmt_crashlog_rc(crashlog, crashlog->info->status.complete);
> +	return pmt_crashlog_rc(crashlog, crashlog->info->status.complete, state);
>  }
>  
> -static bool pmt_crashlog_disabled(struct crashlog_entry *crashlog)
> +static int pmt_crashlog_disabled(struct crashlog_entry *crashlog, bool *state)
>  {
>  	/* return current value of the crashlog disabled flag */
> -	return pmt_crashlog_rc(crashlog, crashlog->info->status.disabled);
> +	return pmt_crashlog_rc(crashlog, crashlog->info->status.disabled, state);
>  }
>  
> -static bool pmt_crashlog_supported(struct intel_pmt_entry *entry, u32 *crash_type, u32 *version)
> +static int pmt_crashlog_supported(struct intel_pmt_entry *entry, u32 *crash_type, u32 *version)
>  {
>  	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
>  
> @@ -199,14 +201,14 @@ static int pmt_crashlog_set_execute(struct crashlog_entry *crashlog)
>  	return pmt_crashlog_rmw(crashlog, crashlog->info->control.manual, true);
>  }
>  
> -static bool pmt_crashlog_cleared(struct crashlog_entry *crashlog)
> +static int pmt_crashlog_cleared(struct crashlog_entry *crashlog, bool *state)
>  {
> -	return pmt_crashlog_rc(crashlog, crashlog->info->status.cleared);
> +	return pmt_crashlog_rc(crashlog, crashlog->info->status.cleared, state);
>  }
>  
> -static bool pmt_crashlog_consumed(struct crashlog_entry *crashlog)
> +static int pmt_crashlog_consumed(struct crashlog_entry *crashlog, bool *state)
>  {
> -	return pmt_crashlog_rc(crashlog, crashlog->info->status.consumed);
> +	return pmt_crashlog_rc(crashlog, crashlog->info->status.consumed, state);
>  }
>  
>  static int pmt_crashlog_set_consumed(struct crashlog_entry *crashlog)
> @@ -214,14 +216,14 @@ static int pmt_crashlog_set_consumed(struct crashlog_entry *crashlog)
>  	return pmt_crashlog_rmw(crashlog, crashlog->info->control.consume, true);
>  }
>  
> -static bool pmt_crashlog_error(struct crashlog_entry *crashlog)
> +static int pmt_crashlog_error(struct crashlog_entry *crashlog, bool *state)
>  {
> -	return pmt_crashlog_rc(crashlog, crashlog->info->status.error);
> +	return pmt_crashlog_rc(crashlog, crashlog->info->status.error, state);
>  }
>  
> -static bool pmt_crashlog_rearm(struct crashlog_entry *crashlog)
> +static int pmt_crashlog_rearm(struct crashlog_entry *crashlog, bool *state)
>  {
> -	return pmt_crashlog_rc(crashlog, crashlog->info->status.rearmed);
> +	return pmt_crashlog_rc(crashlog, crashlog->info->status.rearmed, state);
>  }
>  
>  static int pmt_crashlog_set_rearm(struct crashlog_entry *crashlog)
> @@ -236,7 +238,12 @@ 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);
> +	bool cleared;
> +	int ret;
> +
> +	ret = pmt_crashlog_cleared(crashlog, &cleared);
> +	if (ret)
> +		return ret;
>  
>  	return sysfs_emit(buf, "%d\n", cleared);
>  }
> @@ -271,7 +278,12 @@ 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);
> +	bool consumed;
> +	int ret;
> +
> +	ret = pmt_crashlog_consumed(crashlog, &consumed);
> +	if (ret)
> +		return ret;
>  
>  	return sysfs_emit(buf, "%d\n", consumed);
>  }
> @@ -281,7 +293,9 @@ consumed_store(struct device *dev, struct device_attribute *attr, const char *bu
>  	       size_t count)
>  {
>  	struct crashlog_entry *crashlog;
> +	bool complete;
>  	bool consumed;
> +	bool disabled;
>  	int ret;
>  
>  	crashlog = dev_get_drvdata(dev);
> @@ -296,10 +310,16 @@ consumed_store(struct device *dev, struct device_attribute *attr, const char *bu
>  
>  	guard(mutex)(&crashlog->control_mutex);
>  
> -	if (pmt_crashlog_disabled(crashlog))
> +	ret = pmt_crashlog_disabled(crashlog, &disabled);
> +	if (ret)
> +		return ret;
> +	if (disabled)
>  		return -EBUSY;
>  
> -	if (!pmt_crashlog_complete(crashlog))
> +	ret = pmt_crashlog_complete(crashlog, &complete);
> +	if (ret)
> +		return ret;
> +	if (!complete)
>  		return -EEXIST;
>  
>  	ret = pmt_crashlog_set_consumed(crashlog);
> @@ -312,9 +332,14 @@ static ssize_t
>  enable_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> -	bool enabled = !pmt_crashlog_disabled(crashlog);
> +	bool disabled;
> +	int ret;
>  
> -	return sprintf(buf, "%d\n", enabled);
> +	ret = pmt_crashlog_disabled(crashlog, &disabled);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%d\n", !disabled);
>  }
>  
>  static ssize_t
> @@ -343,7 +368,12 @@ 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);
> +	bool error;
> +	int ret;
> +
> +	ret = pmt_crashlog_error(crashlog, &error);
> +	if (ret)
> +		return ret;
>  
>  	return sysfs_emit(buf, "%d\n", error);
>  }
> @@ -353,7 +383,12 @@ 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);
> +	bool rearmed;
> +	int ret;
> +
> +	ret = pmt_crashlog_rearm(crashlog, &rearmed);
> +	if (ret)
> +		return ret;
>  
>  	return sysfs_emit(buf, "%d\n", rearmed);
>  }
> @@ -389,11 +424,15 @@ trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	struct crashlog_entry *crashlog;
>  	bool trigger;
> +	int ret;
>  
>  	crashlog = dev_get_drvdata(dev);
> -	trigger = pmt_crashlog_complete(crashlog);
>  
> -	return sprintf(buf, "%d\n", trigger);
> +	ret = pmt_crashlog_complete(crashlog, &trigger);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%d\n", trigger);
>  }
>  
>  static ssize_t
> @@ -401,6 +440,8 @@ trigger_store(struct device *dev, struct device_attribute *attr,
>  	      const char *buf, size_t count)
>  {
>  	struct crashlog_entry *crashlog;
> +	bool complete;
> +	bool disabled;
>  	bool trigger;
>  	int ret;
>  
> @@ -413,7 +454,10 @@ trigger_store(struct device *dev, struct device_attribute *attr,
>  	guard(mutex)(&crashlog->control_mutex);
>  
>  	/* if device is currently disabled, return busy */
> -	if (pmt_crashlog_disabled(crashlog))
> +	ret = pmt_crashlog_disabled(crashlog, &disabled);
> +	if (ret)
> +		return ret;
> +	if (disabled)
>  		return -EBUSY;
>  
>  	if (!trigger) {
> @@ -422,7 +466,10 @@ trigger_store(struct device *dev, struct device_attribute *attr,
>  	}
>  
>  	/* we cannot trigger a new crash if one is still pending */
> -	if (pmt_crashlog_complete(crashlog))
> +	ret = pmt_crashlog_complete(crashlog, &complete);
> +	if (ret)
> +		return ret;
> +	if (complete)
>  		return -EEXIST;
>  
>  	ret = pmt_crashlog_set_execute(crashlog);
> -- 
> 2.43.0
>