Re: [PATCH v6 4/4] hwmon: pmbus: mpq8646: gate the writes

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Debugfs attributes perform I2C accesses without acquiring necessary locks, risking race conditions and bypassing hardware write protection.
- [Medium] Mixing goto-based error handling with guard() declarations in the same function.
- [Medium] Debugfs write functions mask negative error codes by returning 0 (success) on hardware failure.
--

--- Patch [4]: [PATCH v6 4/4] hwmon: pmbus: mpq8646: gate the writes ---
commit ec4599677b214a57f0796000d18d9f3b6e5d4fb5
Author: Vincent Jardin <[email protected]>

hwmon: pmbus: mpq8646: gate the writes

This commit adds unsafe debugfs provisioning and benchmarking helpers
to write specific registers. These operations are restricted due to the
risk of browning out the board or persisting bad setpoints.

> diff --git a/drivers/hwmon/pmbus/mpq8646.c b/drivers/hwmon/pmbus/mpq8646.c
> index 1fd41d89e5bef..0034cf5919345 100644
> --- a/drivers/hwmon/pmbus/mpq8646.c
> +++ b/drivers/hwmon/pmbus/mpq8646.c
[ ... ]
> +static int mpq8646_dbg_clear_protection_last(void *data, u64 val)
> +{
[ ... ]
> +	scoped_guard(mutex, &priv->mps_lock)
> +		rc = i2c_smbus_write_byte(priv->client, MPS_CLEAR_LAST_FAULT);
> +	if (rc < 0)
> +		dev_warn(&priv->client->dev,
> +			 "clear_protection_last: CLEAR_LAST_FAULT write failed (%d)\n",
> +			 rc);
> +	return 0;
> +}

[Severity: Medium]
Should this propagate the negative error code from rc instead of returning 0?
Returning 0 masks hardware faults and might lead userspace provisioning
scripts to assume the write succeeded.

[ ... ]
> +static int mpq8646_dbg_clear_protection_last_force(void *data, u64 val)
> +{
[ ... ]
> +	guard(pmbus_lock)(priv->client);
> +	guard(mutex)(&priv->mps_lock);
> +
> +	wp_orig = i2c_smbus_read_byte_data(priv->client, PMBUS_WRITE_PROTECT);
> +	if (wp_orig < 0) {
> +		dev_warn(&priv->client->dev,
> +			 "clear_protection_last_force: WRITE_PROTECT read failed (%d), aborting\n",
> +			 wp_orig);
> +		return 0;
> +	}

[Severity: Medium]
Like above, should this return the negative error code in wp_orig so the I2C
read failure is reported to the caller?

[ ... ]
> +	rc = i2c_smbus_write_word_data(priv->client, MPS_MFR_CFG_EXT,
> +				       (u16)cfg_orig | MPS_MFR_CFG_EXT_CLR_LAST_EN);
> +	if (rc < 0) {
> +		dev_warn(&priv->client->dev,
> +			 "clear_protection_last_force: gate open failed (%d)\n",
> +			 rc);
> +		goto restore_wp;
> +	}

[Severity: Medium]
Does this mix goto-based error handling with guard() declarations in the
same function?

The cleanup guidelines specify that functions should either use scope-based
cleanup exclusively or not at all, to avoid confusing ownership semantics.

[ ... ]
> +static int mpq8646_dbg_store_all(void *data, u64 val)
> +{
[ ... ]
> +	mpq8646_unsafe_warn_once(priv);
> +	scoped_guard(mutex, &priv->mps_lock)
> +		rc = i2c_smbus_write_byte(priv->client, PMBUS_STORE_USER_ALL);

[Severity: High]
Should this also acquire the global pmbus_lock? 

Issuing STORE_USER_ALL without the pmbus_lock might expose an unprotected
NVM-busy window to PMBus core background polling.

> +	if (rc < 0)
> +		dev_warn(&priv->client->dev,
> +			 "store_all: STORE_USER_ALL (0x15) write failed (%d)\n",
> +			 rc);
> +	return 0;
> +}

[Severity: Medium]
Does this need to return the negative error code in rc instead of 0 on
failure?

[ ... ]
> +static int mpq8646_dbg_restore_all(void *data, u64 val)
> +{
[ ... ]
> +	mpq8646_unsafe_warn_once(priv);
> +	scoped_guard(mutex, &priv->mps_lock)
> +		rc = i2c_smbus_write_byte(priv->client, PMBUS_RESTORE_USER_ALL);

[Severity: High]
As with mpq8646_dbg_store_all(), should this also hold pmbus_lock to
prevent NVM-busy window races?

> +	if (rc < 0)
> +		dev_warn(&priv->client->dev,
> +			 "restore_all: RESTORE_USER_ALL (0x16) write failed (%d)\n",
> +			 rc);
> +	return 0;
> +}

[Severity: Medium]
Should this return the negative error code in rc?

[ ... ]
> +static int mpq8646_dbg_reg_set(void *data, u64 val)
> +{
> +	struct mpq8646_dbg_reg_ctx *ctx = data;
> +	int rc;
> +
> +	mpq8646_unsafe_warn_once(ctx->priv);
> +	if (ctx->desc->is_word)
> +		rc = i2c_smbus_write_word_data(ctx->priv->client,
> +					       ctx->desc->reg, (u16)val);
> +	else
> +		rc = i2c_smbus_write_byte_data(ctx->priv->client,
> +					       ctx->desc->reg, (u8)val);

[Severity: High]
Does this need to acquire pmbus_lock or mps_lock before performing the I2C
write?

If another thread concurrently calls mpq8646_dbg_clear_protection_last_force()
which temporarily lowers the PMBUS_WRITE_PROTECT register, could a direct
write here slip through and bypass the hardware write protection?

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