Re: [PATCH v2 02/10] platform/x86/intel/pmt: Add register access callbacks

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing error propagation in pmt_crashlog_rmw and pmt_crashlog_rc when register callbacks fail.
--

--- Patch [2]: [PATCH v2 02/10] platform/x86/intel/pmt: Add register access callbacks ---
commit 63beb7f9cb719c6439cef978b14d03ae4aa7a626
Author: Michael J. Ruhl <[email protected]>

platform/x86/intel/pmt: Add register access callbacks

This commit augments the PMT callback infrastructure to allow registered
drivers to customize read and write access to the control paths.

> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index f936daf99e4d2..5923ad7abbd98 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);
> +			return;

[Severity: Medium]
Should pmt_crashlog_rmw() be updated to return an int so that this error code
can be propagated to the caller? 

Since it currently returns void, any sysfs write handlers that trigger this
path will silently swallow the error and falsely report success to userspace if
the underlying hardware access fails.

> +		}
> +	} else {
> +		reg = readl(entry->disc_table + control->offset);
> +	}

[ ... ]

> -	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);
> +			return;

[Severity: Medium]
Similarly, should the error code be propagated here rather than returning
without notifying the caller of the failure?

> +		}
> +	} 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;

[Severity: Medium]
Is returning false the correct behavior when the read callback fails? 

This masks the error code from the caller and treats the read failure as if
the bit is unset. This could lead to incorrect evaluations, for example causing
sysfs read handlers to return an inaccurate hardware state. Should we propagate
the error instead?

> +		}
> +	} else {
> +		reg = readl(crashlog->entry.disc_table + status->offset);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2