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

On 7/29/26 15:07, [email protected] wrote:
> 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.
> --
> 

Feedback seems valid. Please address.

Thanks,
Guenter

> --- 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?
>