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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing error check for `pmt_crashlog_complete` in `trigger_store` leads to use of uninitialized variable and dropped error codes.
- [Medium] Sysfs `show` callbacks incorrectly emit string error messages into the output buffer instead of returning negative error codes, breaking the sysfs ABI.
- [Medium] Logic inversion in `enable_show()` causes it to incorrectly return the disabled state instead of the enabled state.
--

commit 812a753233238ae00d9b825c5eb2c3d208aac022
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

platform/x86/intel/pmt: refactor rc with a return value

This commit refactors pmt_crashlog_rw with a return value to handle possible
failures from hardware accesses. It updates the usage of various crashlog
functions to use the new return codes.

> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
[ ... ]
> @@ -236,9 +238,14 @@ 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;
>  
> -	return sysfs_emit(buf, "%d\n", cleared);
> +	ret = pmt_crashlog_cleared(crashlog, &cleared);
> +	if (ret)
> +		return sysfs_emit(buf, "read error: %d\n", ret);

[Severity: Medium]
Does this break the sysfs ABI by emitting an error string into the buffer on
failure instead of returning a negative error code? Userspace applications
might see a successful read containing a string like "read error: -5\n" and
fail to parse it as an integer. This pattern seems to be repeated across
other sysfs show functions modified in this patch, such as consumed_show(),
enable_show(), error_show(), rearm_show(), and trigger_show().

> +	else
> +		return sysfs_emit(buf, "%d\n", cleared);
>  }
[ ... ]
> @@ -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 enabled;
> +	int ret;
>  
> -	return sprintf(buf, "%d\n", enabled);
> +	ret = pmt_crashlog_disabled(crashlog, &enabled);
> +	if (ret)
> +		return sysfs_emit(buf, "error: %d\n", ret);
> +	else
> +		return sysfs_emit(buf, "%d\n", enabled);

[Severity: Medium]
Did the logic for the enable attribute get inverted here? The previous code
negated the result of pmt_crashlog_disabled(), but the new code assigns the
disabled state directly to the enabled variable and emits it. Should enabled
be negated before emitting?

>  }
[ ... ]
> @@ -422,7 +466,8 @@ 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 (complete)

[Severity: High]
Does this code read from the complete variable without checking if the
hardware read failed? If pmt_crashlog_complete() returns an error, complete
remains uninitialized on the stack. The error code in ret is also silently
overwritten by the subsequent pmt_crashlog_set_execute() call. Should there
be an if (ret) return ret; check before evaluating complete?

>  		return -EEXIST;
>  
>  	ret = pmt_crashlog_set_execute(crashlog);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901192736.626777-20-michael.j.ruhl@intel.com?part=3