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

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

On Fri, Sep 11, 2026 at 01:11:50PM -0700, Michael J. Ruhl wrote:
> Upcoming changes will include possible failures from HW accesses.
> 
> Refactor pmt_crashlog_rwm() 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 | 90 ++++++++++++-----------
>  1 file changed, 46 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index f936daf99e4d..831cff743803 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -125,7 +125,7 @@ struct pmt_crashlog_priv {
>   */
>  
>  /* Read, modify, write the control register, setting or clearing @bit based on @set */
> -static void pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
> +static int 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;
> @@ -139,6 +139,8 @@ static void pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
>  		reg &= ~bit;
>  
>  	writel(reg, entry->disc_table + control->offset);
> +
> +	return 0;
>  }
>  
>  /* Read the status register and see if the specified @bit is set */
> @@ -181,20 +183,20 @@ static bool pmt_crashlog_supported(struct intel_pmt_entry *entry, u32 *crash_typ
>  	return false;
>  }
>  
> -static void pmt_crashlog_set_disable(struct crashlog_entry *crashlog,
> -				     bool disable)
> +static int pmt_crashlog_set_disable(struct crashlog_entry *crashlog,
> +				    bool disable)
>  {
> -	pmt_crashlog_rmw(crashlog, crashlog->info->control.disable, disable);
> +	return pmt_crashlog_rmw(crashlog, crashlog->info->control.disable, disable);
>  }
>  
> -static void pmt_crashlog_set_clear(struct crashlog_entry *crashlog)
> +static int pmt_crashlog_set_clear(struct crashlog_entry *crashlog)
>  {
> -	pmt_crashlog_rmw(crashlog, crashlog->info->control.clear, true);
> +	return pmt_crashlog_rmw(crashlog, crashlog->info->control.clear, true);
>  }
>  
> -static void pmt_crashlog_set_execute(struct crashlog_entry *crashlog)
> +static int pmt_crashlog_set_execute(struct crashlog_entry *crashlog)
>  {
> -	pmt_crashlog_rmw(crashlog, crashlog->info->control.manual, true);
> +	return pmt_crashlog_rmw(crashlog, crashlog->info->control.manual, true);
>  }
>  
>  static bool pmt_crashlog_cleared(struct crashlog_entry *crashlog)
> @@ -207,9 +209,9 @@ static bool pmt_crashlog_consumed(struct crashlog_entry *crashlog)
>  	return pmt_crashlog_rc(crashlog, crashlog->info->status.consumed);
>  }
>  
> -static void pmt_crashlog_set_consumed(struct crashlog_entry *crashlog)
> +static int pmt_crashlog_set_consumed(struct crashlog_entry *crashlog)
>  {
> -	pmt_crashlog_rmw(crashlog, crashlog->info->control.consume, true);
> +	return pmt_crashlog_rmw(crashlog, crashlog->info->control.consume, true);
>  }
>  
>  static bool pmt_crashlog_error(struct crashlog_entry *crashlog)
> @@ -222,9 +224,9 @@ static bool pmt_crashlog_rearm(struct crashlog_entry *crashlog)
>  	return pmt_crashlog_rc(crashlog, crashlog->info->status.rearmed);
>  }
>  
> -static void pmt_crashlog_set_rearm(struct crashlog_entry *crashlog)
> +static int pmt_crashlog_set_rearm(struct crashlog_entry *crashlog)
>  {
> -	pmt_crashlog_rmw(crashlog, crashlog->info->control.rearm, true);
> +	return pmt_crashlog_rmw(crashlog, crashlog->info->control.rearm, true);
>  }
>  
>  /*
> @@ -245,13 +247,13 @@ clear_store(struct device *dev, struct device_attribute *attr,
>  {
>  	struct crashlog_entry *crashlog;
>  	bool clear;
> -	int result;
> +	int ret;
>  
>  	crashlog = dev_get_drvdata(dev);
>  
> -	result = kstrtobool(buf, &clear);
> -	if (result)
> -		return result;
> +	ret = kstrtobool(buf, &clear);
> +	if (ret)
> +		return ret;
>  
>  	/* set bit only */
>  	if (!clear)
> @@ -259,9 +261,9 @@ clear_store(struct device *dev, struct device_attribute *attr,
>  
>  	guard(mutex)(&crashlog->control_mutex);
>  
> -	pmt_crashlog_set_clear(crashlog);
> +	ret = pmt_crashlog_set_clear(crashlog);
>  
> -	return count;
> +	return ret ?: count;
>  }
>  static DEVICE_ATTR_RW(clear);
>  
> @@ -280,13 +282,13 @@ consumed_store(struct device *dev, struct device_attribute *attr, const char *bu
>  {
>  	struct crashlog_entry *crashlog;
>  	bool consumed;
> -	int result;
> +	int ret;
>  
>  	crashlog = dev_get_drvdata(dev);
>  
> -	result = kstrtobool(buf, &consumed);
> -	if (result)
> -		return result;
> +	ret = kstrtobool(buf, &consumed);
> +	if (ret)
> +		return ret;
>  
>  	/* set bit only */
>  	if (!consumed)
> @@ -300,9 +302,9 @@ consumed_store(struct device *dev, struct device_attribute *attr, const char *bu
>  	if (!pmt_crashlog_complete(crashlog))
>  		return -EEXIST;
>  
> -	pmt_crashlog_set_consumed(crashlog);
> +	ret = pmt_crashlog_set_consumed(crashlog);
>  
> -	return count;
> +	return ret ?: count;
>  }
>  static DEVICE_ATTR_RW(consumed);
>  
> @@ -321,19 +323,19 @@ enable_store(struct device *dev, struct device_attribute *attr,
>  {
>  	struct crashlog_entry *crashlog;
>  	bool enabled;
> -	int result;
> +	int ret;
>  
>  	crashlog = dev_get_drvdata(dev);
>  
> -	result = kstrtobool(buf, &enabled);
> -	if (result)
> -		return result;
> +	ret = kstrtobool(buf, &enabled);
> +	if (ret)
> +		return ret;
>  
>  	guard(mutex)(&crashlog->control_mutex);
>  
> -	pmt_crashlog_set_disable(crashlog, !enabled);
> +	ret = pmt_crashlog_set_disable(crashlog, !enabled);
>  
> -	return count;
> +	return ret ?: count;
>  }
>  static DEVICE_ATTR_RW(enable);
>  
> @@ -362,13 +364,13 @@ rearm_store(struct device *dev, struct device_attribute *attr, const char *buf,
>  {
>  	struct crashlog_entry *crashlog;
>  	bool rearm;
> -	int result;
> +	int ret;
>  
>  	crashlog = dev_get_drvdata(dev);
>  
> -	result = kstrtobool(buf, &rearm);
> -	if (result)
> -		return result;
> +	ret = kstrtobool(buf, &rearm);
> +	if (ret)
> +		return ret;
>  
>  	/* set only */
>  	if (!rearm)
> @@ -376,9 +378,9 @@ rearm_store(struct device *dev, struct device_attribute *attr, const char *buf,
>  
>  	guard(mutex)(&crashlog->control_mutex);
>  
> -	pmt_crashlog_set_rearm(crashlog);
> +	ret = pmt_crashlog_set_rearm(crashlog);
>  
> -	return count;
> +	return ret ?: count;
>  }
>  static DEVICE_ATTR_RW(rearm);
>  
> @@ -400,13 +402,13 @@ trigger_store(struct device *dev, struct device_attribute *attr,
>  {
>  	struct crashlog_entry *crashlog;
>  	bool trigger;
> -	int result;
> +	int ret;
>  
>  	crashlog = dev_get_drvdata(dev);
>  
> -	result = kstrtobool(buf, &trigger);
> -	if (result)
> -		return result;
> +	ret = kstrtobool(buf, &trigger);
> +	if (ret)
> +		return ret;
>  
>  	guard(mutex)(&crashlog->control_mutex);
>  
> @@ -415,17 +417,17 @@ trigger_store(struct device *dev, struct device_attribute *attr,
>  		return -EBUSY;
>  
>  	if (!trigger) {
> -		pmt_crashlog_set_clear(crashlog);
> -		return count;
> +		ret = pmt_crashlog_set_clear(crashlog);
> +		return ret ?: count;
>  	}
>  
>  	/* we cannot trigger a new crash if one is still pending */
>  	if (pmt_crashlog_complete(crashlog))
>  		return -EEXIST;
>  
> -	pmt_crashlog_set_execute(crashlog);
> +	ret = pmt_crashlog_set_execute(crashlog);
>  
> -	return count;
> +	return ret ?: count;
>  }
>  static DEVICE_ATTR_RW(trigger);
>  
> -- 
> 2.43.0
>