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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Missing debugfs unregistration on early probe error paths leads to dangling debugfs files and a Use-After-Free vulnerability.
- [High] Stack-allocated buffers are passed to `i2c_transfer()`, violating DMA safety rules.
- [High] Incorrect PMBus Direct format coefficients for VOUT lead to significantly wrong voltage scaling.
- [High] The shim of PMBUS_WRITE_PROTECT to 0 prevents the core from unlocking the chip, causing silent write failures.
- [Medium] The alarm polling worker unconditionally rearms itself even when an IRQ is configured, causing unnecessary CPU wakeups.
- [Medium] Missing `dput()` after `debugfs_lookup()` causes a dentry reference leak.
- [Medium] `mpq8646_nvmem_read` accesses NVM registers without holding `mps_lock`, leading to data corruption during chip busy windows.
--

--- Patch [3]: [PATCH 3/3] hwmon: pmbus: add MPQ8646 driver ---
commit 55c0a49021c892a77923b9ad41cc1d13d1b685b0
Author: Vincent Jardin <vjardin@free.fr>
Subject: hwmon: pmbus: add MPQ8646 driver

This commit adds a new single-chip PMBus driver for the MPS MPQ8646,
including extended observability and alarm polling fallbacks.

> diff --git a/drivers/hwmon/pmbus/mpq8646.c b/drivers/hwmon/pmbus/mpq8646.c
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/mpq8646.c
> @@ -0,0 +1,1214 @@

[ ... ]

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

[Severity: High]
Does this pass stack-allocated buffers to i2c_transfer()? Since many I2C
host controllers use DMA, mapping stack memory for DMA can cause a kernel
panic if CONFIG_VMAP_STACK is enabled. Should these buffers be dynamically
allocated?

[ ... ]

> +static int mpq8646_identify(struct i2c_client *client,
> +			    struct pmbus_driver_info *info)
> +{
> +	int vout_mode;

[ ... ]

> +	case PB_VOUT_MODE_VID >> PB_VOUT_MODE_MODE_SHIFT:
> +	case PB_VOUT_MODE_DIRECT >> PB_VOUT_MODE_MODE_SHIFT:
> +		info->format[PSC_VOLTAGE_OUT] = direct;
> +		info->m[PSC_VOLTAGE_OUT] = MPQ8646_VID_M;
> +		info->b[PSC_VOLTAGE_OUT] = MPQ8646_VID_B;
> +		info->R[PSC_VOLTAGE_OUT] = MPQ8646_VID_R;
> +		break;

[Severity: High]
Will these coefficients result in the correct voltage scaling? The Direct
format equation converts register Y to physical X using
X = (Y * 10^-R - b) / m. With m = 64, b = 0, and R = 1, the effective LSB is
10^-1 / 64 = 1.5625 mV/LSB.

To match the 6.4 mV/LSB mentioned in the macro comments, would the correct
coefficients be m = 15625 and R = -2?

[ ... ]

> +static int mpq8646_read_byte_data(struct i2c_client *client, int page, int reg)
> +{
> +	struct mpq8646_priv *priv = mpq8646_priv_from_client(client);
> +	int ret;

[ ... ]

> +	case PMBUS_WRITE_PROTECT:
> +		MPQ8646_TRACE("  WRITE_PROTECT shimmed to 0 (framework path)\n");
> +		return 0;

[Severity: High]
Does returning 0 here prevent the PMBus core from automatically unlocking the
chip? The core initialization code checks PMBUS_WRITE_PROTECT and writes 0 to
unlock it only if it appears enabled. By faking an unlocked state, the core
skips the unlock command, which might cause the hardware to silently ignore
sysfs configuration writes.

[ ... ]

> +#if IS_ENABLED(CONFIG_NVMEM)

[ ... ]

> +static int mpq8646_nvmem_read(void *data, unsigned int offset, void *val,
> +			      size_t bytes)
> +{
> +	struct mpq8646_priv *priv = data;

[ ... ]

> +		if (e->is_word)
> +			rc = i2c_smbus_read_word_data(priv->client, e->reg);
> +		else
> +			rc = i2c_smbus_read_byte_data(priv->client, e->reg);
> +		if (rc < 0)
> +			continue;	/* leave the zero-fill in place */

[Severity: Medium]
Does this sequence need to hold mps_lock during the NVM register reads? If
an nvmem read races with a force clear or an NVM store/restore operation, it
might hit an undocumented NVM-busy window, resulting in NACKs and silently
returning zero-filled snapshot data.

[ ... ]

> +#ifdef CONFIG_DEBUG_FS

[ ... ]

> +static void mpq8646_debugfs_register(struct mpq8646_priv *priv)
> +{
> +	struct dentry *root, *parent;
> +	char name[MPQ8646_DEBUGFS_DIRNAME_MAX];
> +	size_t i;
> +
> +	parent = debugfs_lookup(MPQ8646_DEBUGFS_ROOT_NAME, NULL);
> +	if (!parent)
> +		parent = debugfs_create_dir(MPQ8646_DEBUGFS_ROOT_NAME, NULL);

[Severity: Medium]
Does this code leak the dentry reference returned by debugfs_lookup()? If the
directory already exists, debugfs_lookup() returns it with an incremented
reference count, which requires a matching dput() to prevent a memory leak.

[ ... ]

> +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)
> +		goto rearm;

[ ... ]

> +rearm:
> +	if (priv->alarm_poll_interval_ms)
> +		schedule_delayed_work(&priv->alarm_poll_work,
> +				      msecs_to_jiffies(priv->alarm_poll_interval_ms));
> +}

[Severity: Medium]
Does this unconditionally reschedule the worker even when an IRQ is already
configured? If client->irq is present, taking the goto rearm path will cause
the worker to perpetually wake up and reschedule itself without performing
any checks.

[ ... ]

> +static int mpq8646_probe(struct i2c_client *client)
> +{

[ ... ]

> +	mpq8646_debugfs_register(priv);
> +
> +	if (!device_property_read_u32(dev, "mps,vout-fb-divider-ratio-permille",
> +				      &voltage_scale)) {
> +		if (voltage_scale > MPQ8646_VOUT_SCALE_LOOP_MAX)
> +			return -EINVAL;

[Severity: High]
Can this early return cause a use-after-free in debugfs?

Since mpq8646_debugfs_register() has already created files referencing the
devm_kzalloc-allocated priv struct, returning -EINVAL here leaves dangling
dentry files. If these files are read later, they will dereference the freed
priv structure. Should mpq8646_debugfs_unregister() be called before
returning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-mpq8646_v0-v1-0-14363c75916f@free.fr?part=3