Re: [PATCH v2 3/3] hwmon: pmbus: add MPQ8646 driver

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Stack-allocated buffers are passed to `i2c_transfer()` in `mpq8646_raw_xfer_rword()`, violating DMA mapping rules.
- [Medium] Writing to `alarm_poll_interval_ms` via debugfs fails to restart polling if it was previously disabled.
- [Medium] The driver bypasses the PMBus core lock (`pmbus_lock()`) during concurrent hardware accesses from delayed workers, debugfs, and NVMEM reads.
- [Low] `mpq8646_write_byte` incorrectly hooks the Send Byte callback to intercept `PMBUS_PAGE` writes, making the swallow logic dead code.
- [Low] Multi-variable data race in debugfs probe handlers leads to torn diagnostic data being exposed to userspace.
- [Low] Direct hardware page writes in debugfs bypass PMBus core, permanently desynchronizing the cached page state.
--

--- Patch [3]: [PATCH v2 3/3] hwmon: pmbus: add MPQ8646 driver ---
commit 8f8537703d7daf4f173be90253175f7bc734cf66
Author: Vincent Jardin <[email protected]>
Subject: hwmon: pmbus: add MPQ8646 driver

This commit adds a new PMBus driver for the MPS MPQ8646 synchronous buck
converter. It provides basic PMBus telemetry along with MPS-specific
extensions for alarm polling, STATUS_WORD decoding, and NVM recovery.
The driver also integrates with the regulator framework and provides debugfs
entries for hardware introspection.

> diff --git a/drivers/hwmon/pmbus/mpq8646.c b/drivers/hwmon/pmbus/mpq8646.c
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/mpq8646.c
[ ... ]
> +static int mpq8646_raw_xfer_rword(struct i2c_client *client, u8 reg)
> +{
> +	u8 cmd = reg;
> +	__le16 data = 0;
> +	struct i2c_msg msg[] = {
> +		{
> +			.addr = client->addr,
> +			.flags = 0,
> +			.len = sizeof(cmd),
> +			.buf = &cmd,
> +		},
> +		{
> +			.addr = client->addr,
> +			.flags = I2C_M_RD,
> +			.len = sizeof(data),
> +			.buf = (u8 *)&data,
> +		},
> +	};

[Severity: High]
Is it safe to pass stack-allocated buffers to i2c_transfer() here?
If the underlying I2C adapter driver uses DMA for transfers, dma_map_single()
will attempt to map these stack addresses. On systems with virtually mapped
stacks, this violates DMA mapping rules and could lead to memory corruption.

[ ... ]
> +static int mpq8646_write_byte(struct i2c_client *client, int page, u8 value)
> +{
> +	struct mpq8646_priv *priv = mpq8646_priv_from_client(client);
> +
> +	atomic_inc(&priv->write_byte_calls);
> +	MPQ8646_TRACE("write_byte page=%d value=0x%02x\n", page, value);
> +
> +	if (priv->debug_delay_us)
> +		udelay(priv->debug_delay_us);
> +
> +	if (value == PMBUS_PAGE) {
> +		atomic_inc(&priv->page_writes_swallowed);
> +		return 0;
> +	}

[Severity: Low]
Does this hook effectively intercept page writes?
The write_byte callback is used for the PMBus Send Byte protocol, but PMBus
page changes use the Write Byte Data protocol via i2c_smbus_write_byte_data().
It appears page changes will completely bypass this check.

[ ... ]
> +static int mpq8646_nvmem_read(void *data, unsigned int offset, void *val,
> +			      size_t bytes)
> +{
> +	struct mpq8646_priv *priv = data;
> +	u8 *out = val;
> +	size_t i;
> +
> +	if (offset >= MPQ8646_NVMEM_SIZE)
> +		return -EINVAL;
> +	if (offset + bytes > MPQ8646_NVMEM_SIZE)
> +		bytes = MPQ8646_NVMEM_SIZE - offset;
> +
> +	memset(out, 0, bytes);
> +
> +	mutex_lock(&priv->mps_lock);
> +	for (i = 0; i < ARRAY_SIZE(mpq8646_nvmem_map); i++) {

[Severity: Medium]
Does this sequence bypass the PMBus core serialization?
By using the private mps_lock instead of pmbus_lock(), the PMBus core could
concurrently issue sysfs reads during these NVMEM reads.

[ ... ]
> +static int mpq8646_dbg_probe_smbus_rword(void *data, u64 val)
> +{
> +	struct mpq8646_priv *priv = data;
> +	int rc;
> +
> +	rc = i2c_smbus_read_word_data(priv->client, (u8)val);
> +	priv->last_probe_rc = rc;
> +	priv->last_probe_data = (rc < 0) ? 0 : (u16)rc;
> +	return 0;
> +}

[Severity: Low]
Could concurrent debugfs writes cause torn diagnostic state here?
The last_probe_rc and last_probe_data variables are updated sequentially
without synchronization. This might expose mismatched values to userspace
readers in mpq8646_dbg_last_probe_show().

[ ... ]
> +static int mpq8646_dbg_probe_page_write(void *data, u64 val)
> +{
> +	struct mpq8646_priv *priv = data;
> +	int rc;
> +
> +	rc = i2c_smbus_write_byte_data(priv->client, PMBUS_PAGE, (u8)val);

[Severity: Low]
Could this direct hardware page write desynchronize the PMBus core page cache?
Since the core's software page cache is not updated, and the driver declares
pages = 1, the core assumes page switches are unnecessary and will not restore
it. This could cause sysfs telemetry to return incorrect data until reload.

[ ... ]
> +static int mpq8646_dbg_clear_protection_last_force(void *data, u64 val)
> +{
> +	struct mpq8646_priv *priv = data;
> +	int rc, last_rc;
> +	int wp_orig, cfg_orig;
> +
> +	if (!val)
> +		return 0;
> +
> +	mutex_lock(&priv->mps_lock);
> +
> +	wp_orig = i2c_smbus_read_byte_data(priv->client, PMBUS_WRITE_PROTECT);

[Severity: Medium]
Does this multi-step unlock sequence bypass pmbus_lock()?
The PMBus core could issue interleaved sysfs reads while the chip is unlocked
or busy writing to NVM, potentially leading to I2C NACKs.

[ ... ]
> +static void mpq8646_debugfs_register(struct mpq8646_priv *priv)
> +{
[ ... ]
> +	debugfs_create_file_unsafe("restore_all", 0200, root, priv,
> +				   &mpq8646_dbg_restore_all_fops);
> +	debugfs_create_u32("alarm_poll_interval_ms", 0600, root,
> +			   &priv->alarm_poll_interval_ms);

[Severity: Medium]
How is the worker restarted if a user writes to this debugfs file after
disabling polling?
Because the variable is exported directly without a setter function, writing
a non-zero value updates the integer but fails to call schedule_delayed_work()
to re-arm the worker.

[ ... ]
> +static void mpq8646_alarm_poll_work(struct work_struct *work)
> +{
> +	struct mpq8646_priv *priv = container_of(to_delayed_work(work),
> +						 struct mpq8646_priv,
> +						 alarm_poll_work);
> +	int rc;
> +	u16 cur, newly_set;
> +	size_t i;
> +
> +	if (priv->client->irq)
> +		return;	/* SMBALERT# wired; polling not needed */
> +
> +	if (!priv->alarm_poll_interval_ms)
> +		return;	/* polling disabled; don't re-arm */
> +
> +	if (!priv->hwmon_dev)
> +		goto rearm;	/* hwmon not ready yet; try again next tick */
> +
> +	rc = i2c_smbus_read_word_data(priv->client, PMBUS_STATUS_WORD);

[Severity: Medium]
Does this direct hardware telemetry access also bypass pmbus_lock()?
Without the core lock, concurrent accesses from this background worker
could conflict with other sysfs polling.

[ ... ]

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