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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Stack-allocated variables are passed to `i2c_transfer()`, violating DMA safety constraints.
- [Medium] Incorrect channel indices in mpq8646_alarm_map cause off-by-one uevent generation for voltage alarms.
- [Low] The shared debugfs root directory 'mpq8646' is leaked upon module unload.
- [Low] The debugfs entry `probe_page_write` is documented but its implementation is missing.
- [Low] Spelling mistake in the documentation.
--

--- Patch [3]: [PATCH v4 3/3] hwmon: pmbus: add MPQ8646 driver ---
commit f8de07401397e8a8db3fd6012638f3548f012fb7
Author: Vincent Jardin <[email protected]>
Date:   Thu Jul 23 21:38:46 2026 +0000

    hwmon: pmbus: add MPQ8646 driver
    
    Add a new single-chip driver for the MPS MPQ8646 that is
    a PMBus device.
    
    Beyond basic PMBus telemetry, the driver adds:
      - MFR_CFG_EXT gate-close retry after CLEAR_LAST_FAULT.
      - alarm acknowledge via inX_reset_history.
      - STATUS_WORD MPS-extended bit decode + post-mortem clear.
      - In-driver alarm-poll fallback work item (thanks lm90) for
        boards without SMBALERT
      - on_off_config/vout_margin/mfr_pmbus_lock sysfs.

diff --git a/Documentation/hwmon/mpq8646.rst b/Documentation/hwmon/mpq8646.rst
new file mode 100644
index 0000000000000..61ca2a5387daa
--- /dev/null
+++ b/Documentation/hwmon/mpq8646.rst
@@ -0,0 +1,314 @@
[ ... ]
> +clear_protection_last_force performs the unlock with the following
> +six-step dansing:

[Severity: Low]
This isn't a bug, but there is a typo here (dansing instead of dancing).

[ ... ]
> +==========================  ==================================================
> +File                        Action
> +==========================  ==================================================
> +``probe_smbus_rword <reg>``  i2c_smbus_read_word_data on <reg>
> +``probe_smbus_rbyte <reg>``  i2c_smbus_read_byte_data on <reg>
> +``probe_raw_xfer <reg>``     raw i2c_transfer read-word on <reg>
> +``probe_page_write <pg>``    write PMBUS_PAGE = <pg>

[Severity: Low]
The documentation explicitly describes the probe_page_write debugfs entry,
but its implementation appears to be missing from the driver in
mpq8646_debugfs_register().

[ ... ]
diff --git a/drivers/hwmon/pmbus/mpq8646.c b/drivers/hwmon/pmbus/mpq8646.c
new file mode 100644
index 0000000000000..95abd347e3e9b
--- /dev/null
+++ b/drivers/hwmon/pmbus/mpq8646.c
@@ -0,0 +1,1237 @@
[ ... ]
> +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]
Are these stack-allocated variables safe for DMA operations?

Passing stack addresses like &cmd and &data in i2c_msg.buf to i2c_transfer()
violates the DMA API when the kernel is compiled with CONFIG_VMAP_STACK=y.
This could lead to a system crash if the underlying I2C controller driver
relies on DMA for transfers.

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

[Severity: Low]
Does this leave a stale directory in debugfs upon module unload?

The shared mpq8646 root directory is created here, but there does not seem
to be a module_exit function handling global teardown to remove it. The
driver relies on module_i2c_driver(), and mpq8646_debugfs_unregister()
only removes the per-device subdirectories, which might leak this root
directory when the module is removed.

[ ... ]
> +static const struct {
> +	u16 mask;
> +	enum hwmon_sensor_types type;
> +	u32 attr;
> +	int channel;
> +} mpq8646_alarm_map[] = {
> +	{ PB_STATUS_INPUT,       hwmon_in,   hwmon_in_alarm,   0 },	/* in1_alarm  (VIN)  */
> +	{ PB_STATUS_VOUT,        hwmon_in,   hwmon_in_alarm,   1 },	/* in2_alarm  (VOUT) */

[Severity: Medium]
Will this correctly attribute the uevents to the intended channels?

Since hwmon_attr_base(hwmon_in) returns 0, hwmon_notify_event() requires
channel 1 for in1_alarm and channel 2 for in2_alarm.

Using 0 and 1 here in mpq8646_alarm_map would misattribute VOUT faults to
in1_alarm, and incorrectly attempt to generate events for in0_alarm which
does not exist.

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