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

From: Guenter Roeck

Date: Thu Jul 30 2026 - 00:28:46 EST

On 7/29/26 14:51, Vincent Jardin via B4 Relay wrote:

From: Vincent Jardin <vjardin@xxxxxxx>

The write cases of debugfs entries are provisioning and bench
helpers. By designs the MPQ8646 powers the CPU cores rail,
so a wrong write can brown out the board or persist a bad
setpoint into finite-cycle NVM.

Signed-off-by: Vincent Jardin <vjardin@xxxxxxx>
---
Documentation/hwmon/mpq8646.rst | 37 ++++++
drivers/hwmon/pmbus/Kconfig | 17 +++
drivers/hwmon/pmbus/mpq8646.c | 269 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 323 insertions(+)

diff --git a/Documentation/hwmon/mpq8646.rst b/Documentation/hwmon/mpq8646.rst
index d3ede656ea15..6207cfd89f7f 100644
--- a/Documentation/hwmon/mpq8646.rst
+++ b/Documentation/hwmon/mpq8646.rst
@@ -246,6 +246,43 @@ The only writable entry is ``alarm_poll_interval_ms`` (the alarm-poll
worker cadence, see above), it is driver-local and never touches the
chip.
+Unsafe provisioning
+-------------------
+
+Available only when ``CONFIG_SENSORS_MPQ8646_DEBUG_UNSAFE=y``
+(``default n``, it is for boards bring up only). They WRITE to the regulator.
+On many designs the MPQ8646 powers the main CPU core rail, so a wrong write can
+brown out the board or persist a bad setpoint into finite-cycle NVM. The first
+gated write logs a one-shot warning.
+

This and the explanation in Kconfig need to be much stronger,
along the line of

"WARNING: Wrong register writes can and likely will physically damage
or destroy the chip and/or the board".

+=============================== =============== =========================================================
+File PMBus / MFR cmd Description
+=============================== =============== =========================================================
+``store_all`` 0x15 STORE_USER_ALL Send-Byte (commit RAM config to NVM)
+``restore_all`` 0x16 RESTORE_USER_ALL Send-Byte (revert RAM to last-NVM image)
+``clear_protection_last`` 0x08 CLEAR_LAST_FAULT Send-Byte
+``clear_protection_last_force`` -- gated CLEAR_LAST_FAULT, see below
+``on_off_config`` 0x02 (byte) PMBus vs CTRL-pin on/off source + active polarity
+``vout_margin_high`` 0x25 (word) margin-high VOUT setpoint
+``vout_margin_low`` 0x26 (word) margin-low VOUT setpoint
+``mfr_pmbus_lock`` 0xEE (word) programmable PMBus write-lock
+``mfr_product_rev_user`` 0xC2 (word) user-programmable product revision
+=============================== =============== =========================================================
+
+``clear_protection_last`` writes ``CLEAR_LAST_FAULT`` (0x08) Send-Byte.
+The chip silently no-ops unless ``MFR_CFG_EXT`` (0xF5) bit[6] is set.
+
+``clear_protection_last_force`` performs the unlock with the following
+six-step dancing:
+
+1. read ``WRITE_PROTECT`` (0x10) and ``MFR_CFG_EXT`` (0xF5) for restore
+2. clear ``WRITE_PROTECT`` if set
+3. set ``MFR_CFG_EXT`` bit[6] = 1, preserving other bits
+4. send ``CLEAR_LAST_FAULT`` (0x08)
+5. restore ``MFR_CFG_EXT`` (with retry to handle the chip's
+ undocumented post-NVM-write busy window)
+6. restore ``WRITE_PROTECT``
+
Devicetree
----------
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 9f44e76b0b64..9c6d3de61a75 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -627,6 +627,23 @@ config SENSORS_MPQ8646
This driver can also be built as a module. If so, the module
will be called mpq8646.
+config SENSORS_MPQ8646_DEBUG_UNSAFE
+ bool "MPQ8646 unsafe write/provisioning debugfs (DANGEROUS)"
+ depends on SENSORS_MPQ8646 && DEBUG_FS
+ default n
+ help
+ Expose additional WRITE-able debugfs files in the client's
+ pmbus debugfs directory,
+ /sys/kernel/debug/i2c/i2c-<bus>/<bus>-<addr>/
+
+ These are provisioning and bring-up aids. On many designs the
+ MPQ8646 powers the SoC core rail, so a wrong write can brown out
+ or permanently mis-provision the board.
+
+ It shall never be set in a production, shipping, or default configuration.
+
+ If unsure, say N.
+
config SENSORS_PIM4328
tristate "Flex PIM4328 and compatibles"
help
diff --git a/drivers/hwmon/pmbus/mpq8646.c b/drivers/hwmon/pmbus/mpq8646.c
index 1fd41d89e5be..0034cf591934 100644
--- a/drivers/hwmon/pmbus/mpq8646.c
+++ b/drivers/hwmon/pmbus/mpq8646.c
@@ -7,6 +7,7 @@
#include <linux/bitops.h>
#include <linux/debugfs.h>
+#include <linux/delay.h>
#include <linux/i2c.h>
#include <linux/module.h>
#include <linux/mutex.h>
@@ -23,8 +24,18 @@
#define MPQ8646_ALARM_POLL_MS_DEFAULT 1000
/* MPS vendor-extended command codes (NOT in PMBus 1.3 Part II) */
+#define MPS_CLEAR_LAST_FAULT 0x08
+#define MPS_MFR_CFG_EXT 0xF5
+#define MPS_MFR_CFG_EXT_CLR_LAST_EN BIT(6)
#define MPS_PROTECTION_LAST 0xFB
+/*
+ * PMBus 1.3 NVM commit / revert commands. MPS equivalent of
+ * STORE_ALL (15h) and RESTORE_ALL (16h).
+ */
+#define PMBUS_STORE_USER_ALL 0x15
+#define PMBUS_RESTORE_USER_ALL 0x16
+
/* PMBus 1.3 timing / UVLO command codes */
#define PMBUS_VIN_ON 0x35
#define PMBUS_VIN_OFF 0x36
@@ -36,10 +47,32 @@
/* MPS vendor-extended observability / identity registers */
#define MPS_MFR_CONFIG_ID 0xC0
#define MPS_MFR_CONFIG_CODE_REV 0xC1
+#define MPS_MFR_PRODUCT_REV_USER 0xC2
#define MPS_MFR_SILICON_REV 0xC3
#define MPS_MFR_RETRY_TIMES 0xF4
#define MPS_MFR_VBOOT_CFG 0xFC
+/*
+ * MPS_MFR_PMBUS_LOCK (EEh): 16-bit WORD whose low two bits gate
+ * subsequent PMBus writes
+ * bits[1:0] = 00 -- unlocked (POR default)
+ * 01 -- lock all writes EXCEPT VOUT_COMMAND (0x21)
+ * so the operator can still DVFS the rail
+ * 11 -- lock all writes
+ * A negative-going PG edge resets these bits to 00, the lock
+ * is operationally reversible without a full chip POR.
+ */
+#define MPS_MFR_PMBUS_LOCK 0xEE
+
+/*
+ * Retry parameters for the MFR_CFG_EXT gate-close write after
+ * CLEAR_LAST_FAULT. Bench-observed NVM-busy NACK window on this
+ * silicon is about 1 ms; the datasheet does not have information.
+ */
+#define MPQ8646_NVM_RETRY_MAX 5
+#define MPQ8646_NVM_RETRY_DELAY_US_MIN 2000
+#define MPQ8646_NVM_RETRY_DELAY_US_MAX 4000
+
#define MPQ8646_DEBUG(client, fmt, ...) \
dev_dbg(&(client)->dev, fmt, ##__VA_ARGS__)
@@ -498,6 +531,240 @@ static int mpq8646_dbg_reg_show(struct seq_file *s, void *unused)
}
DEFINE_SHOW_ATTRIBUTE(mpq8646_dbg_reg);
+#ifdef CONFIG_SENSORS_MPQ8646_DEBUG_UNSAFE
+/*
+ * Write/provisioning data, disabled by default: NVM commit and
+ * revert, the CLEAR_LAST_FAULT sequences and a small set of named
+ * writable registers.
+ */
+
+static void mpq8646_unsafe_warn_once(struct mpq8646_priv *priv)
+{
+ dev_warn_once(&priv->client->dev,
+ "UNSAFE debugfs write to core-rail regulator; board integrity not guaranteed (CONFIG_SENSORS_MPQ8646_DEBUG_UNSAFE)\n");

This is also not strong enough. You insist adding this functionality to the driver.
Please inform the user that they are about to shoot themselves into the foot.
This is much worse than "board integrity not guaranteed".

CONFIG_SENSORS_MPQ8646_DEBUG_UNSAFE in the message is pretty pointless.

Also, the message should be shown when the driver is loaded, as multi-line
message, similar to the unhashed kernel memory address warning.

**********************************************************
** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **
** **
** This system shows unhashed kernel memory addresses **
** via the console, logs, and other interfaces. This **
** might reduce the security of your system. **
** **
** If you see this message and you are not debugging **
** the kernel, report this immediately to your system **
** administrator! **
** **
** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **
**********************************************************

only as WARNING not as NOTICE.

I'll leave it up to you to find an appropriate and strong enough message.

Thanks,
Guenter