[PATCH v4 0/3] hwmon: (pmbus/adm1266) add clear_blackbox and powerup_counter debugfs entries

[PATCH v4 0/3] hwmon: (pmbus/adm1266) add clear_blackbox and powerup_counter debugfs entries

[Abdurrahman Hussain]

This is what remains of the v3 series after Guenter applied patches
1/5 (firmware_revision) and 5/5 (GPIO line label) to hwmon-next, and
asked for patch 4/5 (rtc_class) to be dropped.

Patch 1 adds a write-only clear_blackbox debugfs file. Devices
configured for single-recording mode (BLACKBOX_CONFIG[0] = 0) need
an explicit clear once the 32-record buffer fills; the documented
sub-command ({0xFE, 0x00} block-write to 0xDE) wasn't reachable
from userspace. The patch also acquires pmbus_lock at the
adm1266_nvmem_read() callback boundary so the memset of
data->dev_mem, the blackbox refill, and the memcpy to userspace run
as a single critical section -- the nvmem core does not serialize
concurrent reg_read calls.

Patch 2 exposes the non-volatile POWERUP_COUNTER (0xE4) via debugfs.
The same value is embedded in every blackbox record, so the live
value lets userspace match a captured record back to the boot it
came from when correlating logs. The block-read is taken under
pmbus_lock to serialise against any pmbus_core PAGE+register
sequence on the device.

Patch 3 takes pmbus_lock in adm1266_state_read() (the pre-existing
sequencer_state debugfs handler) for the same defensive-locking
reason that motivates the new debugfs files in patches 1 and 2:
any direct device access from outside pmbus_core should be ordered
with respect to pmbus_core's own PAGE+register sequences.

Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
Changes in v4:
- Drop former patches 1 (firmware_revision) and 5 (GPIO line label):
  applied to hwmon-next by Guenter.
- Drop former patch 4 (rtc_class) per Guenter's review: turning the
  chip into a pseudo rtc_class device and making the driver depend
  on the RTC subsystem was rejected. The underlying CLOCK_MONOTONIC
  -vs- wall-clock bug in the original probe-time seed is fixed
  independently in the "hwmon: (pmbus/adm1266) buffer-bound and
  timestamp fixes" series sent in parallel, so the seed-at-probe
  behaviour becomes correct without this driver needing to depend
  on RTC_CLASS.
- Patch 1 (clear_blackbox, was 2/5): take pmbus_lock at the outer
  adm1266_nvmem_read() callback rather than inside
  adm1266_nvmem_read_blackbox() so it covers the memset of
  data->dev_mem, the refill, and the memcpy to userspace as a
  single critical section. The nvmem core does not serialize
  concurrent reg_read invocations, so the v3 placement still let
  the memset race with another reader's memcpy to userspace
  (Sashiko review of v3).
- Patch 2 (powerup_counter, was 3/5): take pmbus_lock around the
  block-read so the access serialises with any pmbus_core sequence
  that sets PAGE on the device (Sashiko review of v3).
- New patch 3: take pmbus_lock in adm1266_state_read() (the
  sequencer_state debugfs handler).  Same defensive-locking
  argument as patches 1 and 2; surfaced while folding similar
  fixes into the parallel "hwmon: (pmbus/adm1266) GPIO accessor
  fixes" series.
- Link to v3: https://patch.msgid.link/20260512-adm1266-v3-0-a81a479b0bb0@nexthop.ai

Changes in v3:
- Patch 2: take guard(pmbus_lock)(client) in
  adm1266_clear_blackbox_write() and adm1266_nvmem_read_blackbox()
  to serialise the clear against the multi-record nvmem read loop
  (both share command 0xDE). Also move adm1266_config_nvmem() to
  after pmbus_do_probe() so the blackbox nvmem entry isn't exposed
  to userspace before the PMBus mutex is initialised (Sashiko
  review).
- Patch 4: take guard(pmbus_lock)(client) in the RTC
  read_time/set_time callbacks to serialise against the PMBus
  core's PAGE+register sequences. Move adm1266_register_rtc() to
  after pmbus_do_probe() for the same reason as the config_nvmem
  move; /dev/rtcN being opened by userspace (systemd-timesyncd,
  udev) before probe completed would otherwise hit an
  uninitialised mutex (observed on hardware, also flagged by
  Sashiko).
- Link to v2: https://patch.msgid.link/20260510-adm1266-v2-0-3a22b903c2f1@nexthop.ai

Changes in v2:
- Drop the v1 "use wall-clock seconds for SET_RTC" and "write
  fractional-seconds field of SET_RTC" patches. v1 patch 6 already
  added an rtc_class device, so userspace now owns the time-set
  policy and the probe-time seed is no longer needed - removing it
  also fixes the cross-reboot blackbox-correlation regression the
  seed introduced.
- In the rtc_class patch, write the SET_RTC fractional-seconds
  bytes as zero rather than computing them from a timespec64. The
  rtc_class API is second-precision, so the divide that v1 patch 2
  was adding never produced a non-zero result anyway. This also
  drops the 64-bit divide that would have failed to link on 32-bit
  builds (Sashiko review).
- Use %d (decimal) for the I2C adapter number in GPIO line labels,
  matching the i2c-N convention used elsewhere in Linux. v1 used
  %x, which rendered bus numbers >= 10 in hexadecimal (Sashiko
  review).
- Link to v1: https://patch.msgid.link/20260508-adm1266-v1-0-ec08bf29e0ce@nexthop.ai

To: Guenter Roeck <linux@roeck-us.net>
To: Alexandru Tachici <alexandru.tachici@analog.com>
Cc: linux-hwmon@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Abdurrahman Hussain (3):
      hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry
      hwmon: (pmbus/adm1266) add powerup_counter debugfs entry
      hwmon: (pmbus/adm1266) serialize sequencer_state debugfs read with pmbus_lock

 drivers/hwmon/pmbus/adm1266.c | 77 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 71 insertions(+), 6 deletions(-)
---
base-commit: 70eda68668d1476b459b64e69b8f36659fa9dfa8
change-id: 20260507-adm1266-cf3af42dc3d2

Best regards,
--  
Abdurrahman Hussain <abdurrahman@nexthop.ai>

[PATCH v4 1/3] hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry

[Abdurrahman Hussain]

The ADM1266 blackbox can be configured in two recording modes via
BLACKBOX_CONFIG[0]: cyclic, where the device overwrites the oldest
record once the 32-record buffer fills, and single, where it stops
recording until the buffer is cleared. Deployments that need to
preserve the full record history across multiple fault episodes
typically run in single mode and need a way to clear the buffer
after the records have been collected.

Expose a write-only debugfs file alongside sequencer_state. Writing
any data to it issues the documented clear-blackbox sub-command:
a 2-byte block-write to READ_BLACKBOX (0xDE) with payload
{0xFE, 0x00} (datasheet Rev. D).

The clear is taken under pmbus_lock because READ_BLACKBOX is also
used by adm1266_nvmem_read_blackbox() to walk records one at a
time; without the lock the clear could interleave mid-iteration
and corrupt the read sequence.

Also acquire pmbus_lock in adm1266_nvmem_read() so the memset of
data->dev_mem, the blackbox refill, and the memcpy out to userspace
run as a single critical section. The nvmem core does not serialize
concurrent reg_read calls, so two concurrent reads of the nvmem
device could otherwise race the memset against the memcpy and copy
garbage to userspace.

Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/hwmon/pmbus/adm1266.c | 47 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index d90f8f80be8e..9f4709bc85af 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -331,6 +331,39 @@ static int adm1266_state_read(struct seq_file *s, void *pdata)
 	return 0;
 }
 
+/*
+ * Clearing the blackbox is required when the device is configured in
+ * single-recording mode (BLACKBOX_CONFIG[0] = 0): once the 32-record
+ * buffer is full the device stops recording until cleared.
+ *
+ * The clear is issued as a 2-byte block-write to READ_BLACKBOX with
+ * payload {0xFE, 0x00} per the datasheet. READ_BLACKBOX is also used
+ * by adm1266_nvmem_read_blackbox() to walk records one at a time;
+ * both paths run under pmbus_lock so the clear cannot interleave
+ * mid-iteration and corrupt the read sequence.
+ */
+static ssize_t adm1266_clear_blackbox_write(struct file *file, const char __user *ubuf,
+					    size_t count, loff_t *ppos)
+{
+	struct i2c_client *client = file->private_data;
+	u8 payload[2] = { 0xFE, 0x00 };
+	int ret;
+
+	guard(pmbus_lock)(client);
+	ret = i2c_smbus_write_block_data(client, ADM1266_READ_BLACKBOX,
+					 sizeof(payload), payload);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+static const struct file_operations adm1266_clear_blackbox_fops = {
+	.owner = THIS_MODULE,
+	.open = simple_open,
+	.write = adm1266_clear_blackbox_write,
+	.llseek = noop_llseek,
+};
 static void adm1266_init_debugfs(struct adm1266_data *data)
 {
 	struct dentry *root;
@@ -343,6 +376,8 @@ static void adm1266_init_debugfs(struct adm1266_data *data)
 
 	debugfs_create_devm_seqfile(&data->client->dev, "sequencer_state", data->debugfs_dir,
 				    adm1266_state_read);
+	debugfs_create_file("clear_blackbox", 0200, data->debugfs_dir, data->client,
+			    &adm1266_clear_blackbox_fops);
 }
 
 static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
@@ -355,7 +390,6 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
 	ret = i2c_smbus_read_block_data(data->client, ADM1266_BLACKBOX_INFO, buf);
 	if (ret < 0)
 		return ret;
-
 	if (ret != 4)
 		return -EIO;
 
@@ -365,7 +399,6 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
 		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, read_buff);
 		if (ret < 0)
 			return ret;
-
 		if (ret != ADM1266_BLACKBOX_SIZE)
 			return -EIO;
 
@@ -383,6 +416,8 @@ static int adm1266_nvmem_read(void *priv, unsigned int offset, void *val, size_t
 	if (offset + bytes > data->nvmem_config.size)
 		return -EINVAL;
 
+	guard(pmbus_lock)(data->client);
+
 	if (offset == 0) {
 		memset(data->dev_mem, 0, data->nvmem_config.size);
 
@@ -470,14 +505,14 @@ static int adm1266_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
-	ret = adm1266_config_nvmem(data);
-	if (ret < 0)
-		return ret;
-
 	ret = pmbus_do_probe(client, &data->info);
 	if (ret)
 		return ret;
 
+	ret = adm1266_config_nvmem(data);
+	if (ret < 0)
+		return ret;
+
 	adm1266_init_debugfs(data);
 
 	return 0;

-- 
2.53.0

Re: [PATCH v4 1/3] hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Heap buffer overflow in adm1266_nvmem_read_blackbox() via unvalidated record_count read from I2C.
- [High] Out-of-bounds write in adm1266_pmbus_block_xfer() due to blindly trusting I2C block read length.
- [High] Out-of-bounds write in adm1266_pmbus_block_xfer() caused by mismatch between buffer capacity and I2C transfer length.
--

commit 0afcc14abebf60e90db3728285512325b57755bc
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry

This commit introduces a clear_blackbox debugfs entry for the ADM1266
device to allow clearing the blackbox recording buffer. It issues a
2-byte block-write using the READ_BLACKBOX command under the pmbus_lock.
The commit also acquires the pmbus_lock in adm1266_nvmem_read() to
serialize blackbox reads and clear operations.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index d90f8f80be8e0..9f4709bc85afe 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c

[ ... ]

> @@ -355,7 +390,6 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
>  	ret = i2c_smbus_read_block_data(data->client, ADM1266_BLACKBOX_INFO, buf);
>  	if (ret < 0)
>  		return ret;
> -
>  	if (ret != 4)
>  		return -EIO;
>  

This regression wasn't introduced by this patch, but pre-dates it. Can the
unvalidated record_count read from I2C cause a heap buffer overflow?

The function reads ADM1266_BLACKBOX_INFO into buf and uses buf[3] as
record_count:

	record_count = buf[3];

It then loops record_count times, appending ADM1266_BLACKBOX_SIZE
(64 bytes) to read_buff on each iteration. Since read_buff points to
data->dev_mem, which is allocated to 2048 bytes (32 records), could
an I2C device returning a record_count greater than 32 cause an
out-of-bounds write into adjacent heap memory?

> @@ -365,7 +399,6 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
>  		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, read_buff);
>  		if (ret < 0)
>  			return ret;
> -
>  		if (ret != ADM1266_BLACKBOX_SIZE)
>  			return -EIO;

There is another regression that wasn't introduced by this patch, but
pre-dates it. Does blindly trusting the I2C block read length in
adm1266_pmbus_block_xfer() lead to an out-of-bounds write?

In adm1266_pmbus_block_xfer(), the first byte of the I2C response is used
as the transfer length for memcpy():

	memcpy(data_r, &msgs[1].buf[1], msgs[1].buf[0]);

The caller adm1266_nvmem_read_blackbox() anticipates 64 bytes per iteration
and offsets the data_r pointer by 64 bytes per loop. If the I2C device
returns a block length greater than 64, could the memcpy overwrite
subsequent records in the buffer and write past the end of the 2048-byte
allocation?

Additionally, another regression pre-dates this patch. Does a mismatch
between the buffer capacity and I2C transfer length cause an
out-of-bounds write?

The data->read_buf array is declared with a size of
ADM1266_PMBUS_BLOCK_MAX + 1 (256 bytes):

	u8 read_buf[ADM1266_PMBUS_BLOCK_MAX + 1] ____cacheline_aligned;

However, when preparing the I2C read message in adm1266_pmbus_block_xfer(),
msgs[1].len is set to ADM1266_PMBUS_BLOCK_MAX + 2 (257 bytes):

		{
			.addr = client->addr,
			.flags = I2C_M_RD | I2C_M_DMA_SAFE,
			.buf = data->read_buf,
			.len = ADM1266_PMBUS_BLOCK_MAX + 2,
		}

Because the I2C_M_RECV_LEN flag is absent, could the I2C controller
unconditionally attempt to read 257 bytes into the 256-byte read_buf,
writing past the end of struct adm1266_data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-adm1266-v4-0-1f8df4797258@nexthop.ai?part=1

[PATCH v4 2/3] hwmon: (pmbus/adm1266) add powerup_counter debugfs entry

[Abdurrahman Hussain]

The ADM1266 maintains a 16-bit non-volatile POWERUP_COUNTER register
(0xE4, datasheet Rev. D, Table 93) that increments on every power
cycle and cannot be reset by the host. Each blackbox record already
embeds the counter at record time, so the standalone live value is
primarily useful for matching a captured record back to the boot it
came from when correlating logs.

Expose it as a read-only debugfs file alongside sequencer_state. The
block-read returns two payload bytes in little-endian order.

Take pmbus_lock around the block-read so the access serialises with
any pmbus_core sequence that sets PAGE on the device. Without it, a
PAGE write from another thread could interleave between a PAGE set
and a paged read elsewhere in the driver and corrupt either side's
view of the device state machine.

Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/hwmon/pmbus/adm1266.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index 9f4709bc85af..2edf3a424679 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -27,6 +27,7 @@
 #define ADM1266_READ_BLACKBOX	0xDE
 #define ADM1266_SET_RTC		0xDF
 #define ADM1266_GPIO_CONFIG	0xE1
+#define ADM1266_POWERUP_COUNTER	0xE4
 #define ADM1266_BLACKBOX_INFO	0xE6
 #define ADM1266_PDIO_STATUS	0xE9
 #define ADM1266_GPIO_STATUS	0xEA
@@ -331,6 +332,32 @@ static int adm1266_state_read(struct seq_file *s, void *pdata)
 	return 0;
 }
 
+/*
+ * POWERUP_COUNTER (0xE4) is a 2-byte little-endian non-volatile counter
+ * that increments on every device power cycle (datasheet Rev. D, Table
+ * 93). It saturates at 65535 and cannot be reset by the host. Each
+ * blackbox record embeds the counter value at record time, so this live
+ * read is mainly useful for matching a record back to its boot.
+ */
+static int adm1266_powerup_counter_read(struct seq_file *s, void *pdata)
+{
+	struct device *dev = s->private;
+	struct i2c_client *client = to_i2c_client(dev);
+	u8 buf[I2C_SMBUS_BLOCK_MAX];
+	int ret;
+
+	guard(pmbus_lock)(client);
+	ret = i2c_smbus_read_block_data(client, ADM1266_POWERUP_COUNTER, buf);
+	if (ret < 0)
+		return ret;
+	if (ret != 2)
+		return -EIO;
+
+	seq_printf(s, "%u\n", buf[0] | (buf[1] << 8));
+
+	return 0;
+}
+
 /*
  * Clearing the blackbox is required when the device is configured in
  * single-recording mode (BLACKBOX_CONFIG[0] = 0): once the 32-record
@@ -376,6 +403,8 @@ static void adm1266_init_debugfs(struct adm1266_data *data)
 
 	debugfs_create_devm_seqfile(&data->client->dev, "sequencer_state", data->debugfs_dir,
 				    adm1266_state_read);
+	debugfs_create_devm_seqfile(&data->client->dev, "powerup_counter", data->debugfs_dir,
+				    adm1266_powerup_counter_read);
 	debugfs_create_file("clear_blackbox", 0200, data->debugfs_dir, data->client,
 			    &adm1266_clear_blackbox_fops);
 }

-- 
2.53.0

[PATCH v4 3/3] hwmon: (pmbus/adm1266) serialize sequencer_state debugfs read with pmbus_lock

[Abdurrahman Hussain]

adm1266_state_read() backs the sequencer_state debugfs entry and
issues an i2c_smbus_read_word_data(client, ADM1266_READ_STATE)
against the device without taking pmbus_lock.  The pmbus_core
framework holds pmbus_lock around its own multi-transaction
sequences (notably the "set PAGE, then read paged register" pattern
used by hwmon attributes), so an unlocked debugfs reader can land
between a PAGE write and the subsequent paged read in another
thread.  READ_STATE itself is not paged, so it cannot corrupt the
PAGE register in flight, but the same defensive serialisation that
applies to the GPIO accessors applies here: any direct device
access from outside pmbus_core should be ordered with respect to
pmbus_core's own.

Take pmbus_lock at the top of adm1266_state_read() via the
scope-based guard().

Fixes: ed1ff457e187 ("hwmon: (pmbus/adm1266) add debugfs for states")
Cc: stable@vger.kernel.org
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/hwmon/pmbus/adm1266.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index 2edf3a424679..1425371bf1be 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -323,6 +323,7 @@ static int adm1266_state_read(struct seq_file *s, void *pdata)
 	struct i2c_client *client = to_i2c_client(dev);
 	int ret;
 
+	guard(pmbus_lock)(client);
 	ret = i2c_smbus_read_word_data(client, ADM1266_READ_STATE);
 	if (ret < 0)
 		return ret;

-- 
2.53.0

Re: [PATCH v4 3/3] hwmon: (pmbus/adm1266) serialize sequencer_state debugfs read with pmbus_lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Meaningful code and commit message mismatch regarding defensive serialization.
--

commit 5ae3b71ff51816fdbc3baadbd4aef89f7cc85ac0
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

hwmon: (pmbus/adm1266) serialize sequencer_state debugfs read with pmbus_lock
    
adm1266_state_read() backs the sequencer_state debugfs entry and
issues an i2c_smbus_read_word_data() against the device without taking
pmbus_lock. The pmbus_core framework holds pmbus_lock around its own
multi-transaction sequences, so an unlocked debugfs reader can land
between a page write and the subsequent paged read in another thread.

> but the same defensive serialisation that
> applies to the GPIO accessors applies here: any direct device
> access from outside pmbus_core should be ordered with respect to
> pmbus_core's own.

This isn't a bug, but is this statement in the commit message accurate?

Looking at the gpio accessors like adm1266_gpio_get() and
adm1266_gpio_get_multiple(), they appear to issue direct i2c reads without
taking pmbus_lock:

static int adm1266_gpio_get(struct gpio_chip *chip, unsigned int offset)
{
	...
	ret = i2c_smbus_read_block_data(data->client, pmbus_cmd, read_buf);
	...
}

Similarly, adm1266_gpio_dbg_show() acquires data->buf_mutex via
adm1266_pmbus_block_xfer(), which only protects local buffers rather than
serialising against the pmbus core.

Furthermore, if the gpio accessors were modified to take pmbus_lock(),
wouldn't that cause a null pointer dereference crash during driver probe?

adm1266_probe() registers the gpio chip via adm1266_config_gpio() before
calling pmbus_do_probe():

static int adm1266_probe(struct i2c_client *client)
{
	...
	ret = adm1266_config_gpio(data);
	if (ret < 0)
		return ret;

	ret = adm1266_set_rtc(data);
	if (ret < 0)
		return ret;

	ret = pmbus_do_probe(client, &data->info);
	...
}

Since pmbus_do_probe() allocates the internal pmbus_data structure that
pmbus_lock() relies on, it seems the gpio accessors cannot safely use this
lock.

Could the commit message be updated to clarify the actual locking model,
rather than citing the gpio accessors as precedent?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-adm1266-v4-0-1f8df4797258@nexthop.ai?part=3

Re: [PATCH v4 0/3] hwmon: (pmbus/adm1266) add clear_blackbox and powerup_counter debugfs entries

[Guenter Roeck]

Hi,

On 5/16/26 18:18, Abdurrahman Hussain wrote:
> This is what remains of the v3 series after Guenter applied patches
> 1/5 (firmware_revision) and 5/5 (GPIO line label) to hwmon-next, and
> asked for patch 4/5 (rtc_class) to be dropped.
> 
> Patch 1 adds a write-only clear_blackbox debugfs file. Devices
> configured for single-recording mode (BLACKBOX_CONFIG[0] = 0) need
> an explicit clear once the 32-record buffer fills; the documented
> sub-command ({0xFE, 0x00} block-write to 0xDE) wasn't reachable
> from userspace. The patch also acquires pmbus_lock at the
> adm1266_nvmem_read() callback boundary so the memset of
> data->dev_mem, the blackbox refill, and the memcpy to userspace run
> as a single critical section -- the nvmem core does not serialize
> concurrent reg_read calls.
> 
> Patch 2 exposes the non-volatile POWERUP_COUNTER (0xE4) via debugfs.
> The same value is embedded in every blackbox record, so the live
> value lets userspace match a captured record back to the boot it
> came from when correlating logs. The block-read is taken under
> pmbus_lock to serialise against any pmbus_core PAGE+register
> sequence on the device.
> 
> Patch 3 takes pmbus_lock in adm1266_state_read() (the pre-existing
> sequencer_state debugfs handler) for the same defensive-locking
> reason that motivates the new debugfs files in patches 1 and 2:
> any direct device access from outside pmbus_core should be ordered
> with respect to pmbus_core's own PAGE+register sequences.
> 
> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>

The series no longer applies after applying the bug fix series.
Please rebase it on top of the hwmon-next branch of
git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
and resubmit.

Sorry for the trouble, and thanks a lot for fixing all the problems
with the driver.

Guenter

Re: [PATCH v4 0/3] hwmon: (pmbus/adm1266) add clear_blackbox and powerup_counter debugfs entries

[Abdurrahman Hussain]

On Wed May 20, 2026 at 7:10 AM PDT, Guenter Roeck wrote:
> Hi,
>
> On 5/16/26 18:18, Abdurrahman Hussain wrote:
>> This is what remains of the v3 series after Guenter applied patches
>> 1/5 (firmware_revision) and 5/5 (GPIO line label) to hwmon-next, and
>> asked for patch 4/5 (rtc_class) to be dropped.
>> 
>> Patch 1 adds a write-only clear_blackbox debugfs file. Devices
>> configured for single-recording mode (BLACKBOX_CONFIG[0] = 0) need
>> an explicit clear once the 32-record buffer fills; the documented
>> sub-command ({0xFE, 0x00} block-write to 0xDE) wasn't reachable
>> from userspace. The patch also acquires pmbus_lock at the
>> adm1266_nvmem_read() callback boundary so the memset of
>> data->dev_mem, the blackbox refill, and the memcpy to userspace run
>> as a single critical section -- the nvmem core does not serialize
>> concurrent reg_read calls.
>> 
>> Patch 2 exposes the non-volatile POWERUP_COUNTER (0xE4) via debugfs.
>> The same value is embedded in every blackbox record, so the live
>> value lets userspace match a captured record back to the boot it
>> came from when correlating logs. The block-read is taken under
>> pmbus_lock to serialise against any pmbus_core PAGE+register
>> sequence on the device.
>> 
>> Patch 3 takes pmbus_lock in adm1266_state_read() (the pre-existing
>> sequencer_state debugfs handler) for the same defensive-locking
>> reason that motivates the new debugfs files in patches 1 and 2:
>> any direct device access from outside pmbus_core should be ordered
>> with respect to pmbus_core's own PAGE+register sequences.
>> 
>> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>
> The series no longer applies after applying the bug fix series.
> Please rebase it on top of the hwmon-next branch of
> git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
> and resubmit.
>
> Sorry for the trouble, and thanks a lot for fixing all the problems
> with the driver.
>
> Guenter

Will do, thank you for your support!

Abdurrahman

Re: [PATCH v4 0/3] hwmon: (pmbus/adm1266) add clear_blackbox and powerup_counter debugfs entries

[Abdurrahman Hussain]

On Wed May 20, 2026 at 10:10 AM PDT, Abdurrahman Hussain wrote:
> On Wed May 20, 2026 at 7:10 AM PDT, Guenter Roeck wrote:
>> Hi,
>>
>> On 5/16/26 18:18, Abdurrahman Hussain wrote:
>>> This is what remains of the v3 series after Guenter applied patches
>>> 1/5 (firmware_revision) and 5/5 (GPIO line label) to hwmon-next, and
>>> asked for patch 4/5 (rtc_class) to be dropped.
>>> 
>>> Patch 1 adds a write-only clear_blackbox debugfs file. Devices
>>> configured for single-recording mode (BLACKBOX_CONFIG[0] = 0) need
>>> an explicit clear once the 32-record buffer fills; the documented
>>> sub-command ({0xFE, 0x00} block-write to 0xDE) wasn't reachable
>>> from userspace. The patch also acquires pmbus_lock at the
>>> adm1266_nvmem_read() callback boundary so the memset of
>>> data->dev_mem, the blackbox refill, and the memcpy to userspace run
>>> as a single critical section -- the nvmem core does not serialize
>>> concurrent reg_read calls.
>>> 
>>> Patch 2 exposes the non-volatile POWERUP_COUNTER (0xE4) via debugfs.
>>> The same value is embedded in every blackbox record, so the live
>>> value lets userspace match a captured record back to the boot it
>>> came from when correlating logs. The block-read is taken under
>>> pmbus_lock to serialise against any pmbus_core PAGE+register
>>> sequence on the device.
>>> 
>>> Patch 3 takes pmbus_lock in adm1266_state_read() (the pre-existing
>>> sequencer_state debugfs handler) for the same defensive-locking
>>> reason that motivates the new debugfs files in patches 1 and 2:
>>> any direct device access from outside pmbus_core should be ordered
>>> with respect to pmbus_core's own PAGE+register sequences.
>>> 
>>> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>>
>> The series no longer applies after applying the bug fix series.
>> Please rebase it on top of the hwmon-next branch of
>> git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
>> and resubmit.
>>
>> Sorry for the trouble, and thanks a lot for fixing all the problems
>> with the driver.
>>
>> Guenter
>
> Will do, thank you for your support!
>
> Abdurrahman

Hi Guenter,

Before I send v5 of the adm1266 series, I'd like to revisit the
SET_RTC exposure question from your v3 review [1].

The use case I keep landing on is the one the datasheet itself
recommends: a userspace agent (chrony hook, systemd-timesyncd
script, or a small periodic daemon) keeps the chip's seconds
counter aligned with wall-clock so the value embedded in each
blackbox record stays correct across long uptimes.  The probe-
time ktime_get_real_seconds() seed (now in hwmon-next) only fixes
this at boot; the chip's counter drifts after that.

You ruled out rtc_class and called a kernel-side periodic timer
"a bit excessive", which I agree, it is.  That leaves a
userspace-triggered sync.  Two debugfs shapes I'd consider, both under
pmbus/<hwmon>/adm1266/ (alongside the clear_blackbox,
powerup_counter, and sequencer_state entries v5 already adds):

  A. RW since_epoch -- mirrors /sys/class/rtc/<dev>/since_epoch.
     Read returns the chip's SET_RTC seconds counter, write sets
     it.  The read has the useful side benefit of letting
     userspace measure host-vs-chip drift without writing.

  B. Write-only set_rtc -- writing anything to the file makes
     the driver read ktime_get_real_seconds() itself and push it
     to SET_RTC.  Simpler interface (kernel owns the time
     source; userspace just triggers the sync), no payload to
     parse, no way for userspace to pass in a wrong value.

Do either of these look right to you, or would you rather I just
leave the driver at "probe-time seed only" and skip a SET_RTC
interface entirely?  v5 as it stands has no such interface and
is ready to send; a SET_RTC patch can follow separately later if
you'd like one.

[1] https://lore.kernel.org/r/20260512-adm1266-v3-0-a81a479b0bb0@nexthop.ai

Thanks,
Abdurrahman

Re: [PATCH v4 0/3] hwmon: (pmbus/adm1266) add clear_blackbox and powerup_counter debugfs entries

[Guenter Roeck]

On 5/20/26 12:40, Abdurrahman Hussain wrote:
> On Wed May 20, 2026 at 10:10 AM PDT, Abdurrahman Hussain wrote:
>> On Wed May 20, 2026 at 7:10 AM PDT, Guenter Roeck wrote:
>>> Hi,
>>>
>>> On 5/16/26 18:18, Abdurrahman Hussain wrote:
>>>> This is what remains of the v3 series after Guenter applied patches
>>>> 1/5 (firmware_revision) and 5/5 (GPIO line label) to hwmon-next, and
>>>> asked for patch 4/5 (rtc_class) to be dropped.
>>>>
>>>> Patch 1 adds a write-only clear_blackbox debugfs file. Devices
>>>> configured for single-recording mode (BLACKBOX_CONFIG[0] = 0) need
>>>> an explicit clear once the 32-record buffer fills; the documented
>>>> sub-command ({0xFE, 0x00} block-write to 0xDE) wasn't reachable
>>>> from userspace. The patch also acquires pmbus_lock at the
>>>> adm1266_nvmem_read() callback boundary so the memset of
>>>> data->dev_mem, the blackbox refill, and the memcpy to userspace run
>>>> as a single critical section -- the nvmem core does not serialize
>>>> concurrent reg_read calls.
>>>>
>>>> Patch 2 exposes the non-volatile POWERUP_COUNTER (0xE4) via debugfs.
>>>> The same value is embedded in every blackbox record, so the live
>>>> value lets userspace match a captured record back to the boot it
>>>> came from when correlating logs. The block-read is taken under
>>>> pmbus_lock to serialise against any pmbus_core PAGE+register
>>>> sequence on the device.
>>>>
>>>> Patch 3 takes pmbus_lock in adm1266_state_read() (the pre-existing
>>>> sequencer_state debugfs handler) for the same defensive-locking
>>>> reason that motivates the new debugfs files in patches 1 and 2:
>>>> any direct device access from outside pmbus_core should be ordered
>>>> with respect to pmbus_core's own PAGE+register sequences.
>>>>
>>>> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>>>
>>> The series no longer applies after applying the bug fix series.
>>> Please rebase it on top of the hwmon-next branch of
>>> git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
>>> and resubmit.
>>>
>>> Sorry for the trouble, and thanks a lot for fixing all the problems
>>> with the driver.
>>>
>>> Guenter
>>
>> Will do, thank you for your support!
>>
>> Abdurrahman
> 
> Hi Guenter,
> 
> Before I send v5 of the adm1266 series, I'd like to revisit the
> SET_RTC exposure question from your v3 review [1].
> 
> The use case I keep landing on is the one the datasheet itself
> recommends: a userspace agent (chrony hook, systemd-timesyncd
> script, or a small periodic daemon) keeps the chip's seconds
> counter aligned with wall-clock so the value embedded in each
> blackbox record stays correct across long uptimes.  The probe-
> time ktime_get_real_seconds() seed (now in hwmon-next) only fixes
> this at boot; the chip's counter drifts after that.
> 
> You ruled out rtc_class and called a kernel-side periodic timer
> "a bit excessive", which I agree, it is.  That leaves a
> userspace-triggered sync.  Two debugfs shapes I'd consider, both under
> pmbus/<hwmon>/adm1266/ (alongside the clear_blackbox,
> powerup_counter, and sequencer_state entries v5 already adds):
> 
>    A. RW since_epoch -- mirrors /sys/class/rtc/<dev>/since_epoch.
>       Read returns the chip's SET_RTC seconds counter, write sets
>       it.  The read has the useful side benefit of letting
>       userspace measure host-vs-chip drift without writing.
> 
>    B. Write-only set_rtc -- writing anything to the file makes
>       the driver read ktime_get_real_seconds() itself and push it
>       to SET_RTC.  Simpler interface (kernel owns the time
>       source; userspace just triggers the sync), no payload to
>       parse, no way for userspace to pass in a wrong value.
> 

How about a combination ? read returns the current value, anything
written synchronizes with the kernel rtc.

> Do either of these look right to you, or would you rather I just
> leave the driver at "probe-time seed only" and skip a SET_RTC
> interface entirely?  v5 as it stands has no such interface and
> is ready to send; a SET_RTC patch can follow separately later if
> you'd like one.
> 
I am fine either way.

Thanks,
Guenter

Re: [PATCH v4 0/3] hwmon: (pmbus/adm1266) add clear_blackbox and powerup_counter debugfs entries

[Abdurrahman Hussain]

On Wed May 20, 2026 at 1:59 PM PDT, Guenter Roeck wrote:
> On 5/20/26 12:40, Abdurrahman Hussain wrote:
>> On Wed May 20, 2026 at 10:10 AM PDT, Abdurrahman Hussain wrote:
>>> On Wed May 20, 2026 at 7:10 AM PDT, Guenter Roeck wrote:
>>>> Hi,
>>>>
>>>> On 5/16/26 18:18, Abdurrahman Hussain wrote:
>>>>> This is what remains of the v3 series after Guenter applied patches
>>>>> 1/5 (firmware_revision) and 5/5 (GPIO line label) to hwmon-next, and
>>>>> asked for patch 4/5 (rtc_class) to be dropped.
>>>>>
>>>>> Patch 1 adds a write-only clear_blackbox debugfs file. Devices
>>>>> configured for single-recording mode (BLACKBOX_CONFIG[0] = 0) need
>>>>> an explicit clear once the 32-record buffer fills; the documented
>>>>> sub-command ({0xFE, 0x00} block-write to 0xDE) wasn't reachable
>>>>> from userspace. The patch also acquires pmbus_lock at the
>>>>> adm1266_nvmem_read() callback boundary so the memset of
>>>>> data->dev_mem, the blackbox refill, and the memcpy to userspace run
>>>>> as a single critical section -- the nvmem core does not serialize
>>>>> concurrent reg_read calls.
>>>>>
>>>>> Patch 2 exposes the non-volatile POWERUP_COUNTER (0xE4) via debugfs.
>>>>> The same value is embedded in every blackbox record, so the live
>>>>> value lets userspace match a captured record back to the boot it
>>>>> came from when correlating logs. The block-read is taken under
>>>>> pmbus_lock to serialise against any pmbus_core PAGE+register
>>>>> sequence on the device.
>>>>>
>>>>> Patch 3 takes pmbus_lock in adm1266_state_read() (the pre-existing
>>>>> sequencer_state debugfs handler) for the same defensive-locking
>>>>> reason that motivates the new debugfs files in patches 1 and 2:
>>>>> any direct device access from outside pmbus_core should be ordered
>>>>> with respect to pmbus_core's own PAGE+register sequences.
>>>>>
>>>>> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>>>>
>>>> The series no longer applies after applying the bug fix series.
>>>> Please rebase it on top of the hwmon-next branch of
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
>>>> and resubmit.
>>>>
>>>> Sorry for the trouble, and thanks a lot for fixing all the problems
>>>> with the driver.
>>>>
>>>> Guenter
>>>
>>> Will do, thank you for your support!
>>>
>>> Abdurrahman
>> 
>> Hi Guenter,
>> 
>> Before I send v5 of the adm1266 series, I'd like to revisit the
>> SET_RTC exposure question from your v3 review [1].
>> 
>> The use case I keep landing on is the one the datasheet itself
>> recommends: a userspace agent (chrony hook, systemd-timesyncd
>> script, or a small periodic daemon) keeps the chip's seconds
>> counter aligned with wall-clock so the value embedded in each
>> blackbox record stays correct across long uptimes.  The probe-
>> time ktime_get_real_seconds() seed (now in hwmon-next) only fixes
>> this at boot; the chip's counter drifts after that.
>> 
>> You ruled out rtc_class and called a kernel-side periodic timer
>> "a bit excessive", which I agree, it is.  That leaves a
>> userspace-triggered sync.  Two debugfs shapes I'd consider, both under
>> pmbus/<hwmon>/adm1266/ (alongside the clear_blackbox,
>> powerup_counter, and sequencer_state entries v5 already adds):
>> 
>>    A. RW since_epoch -- mirrors /sys/class/rtc/<dev>/since_epoch.
>>       Read returns the chip's SET_RTC seconds counter, write sets
>>       it.  The read has the useful side benefit of letting
>>       userspace measure host-vs-chip drift without writing.
>> 
>>    B. Write-only set_rtc -- writing anything to the file makes
>>       the driver read ktime_get_real_seconds() itself and push it
>>       to SET_RTC.  Simpler interface (kernel owns the time
>>       source; userspace just triggers the sync), no payload to
>>       parse, no way for userspace to pass in a wrong value.
>> 
>
> How about a combination ? read returns the current value, anything
> written synchronizes with the kernel rtc.
>

Going with that for v5.  One naming question before I send:

  - set_rtc -- matches the PMBus command name (SET_RTC = 0xDF), but
    "set" reads as write-only at first glance, which is a bit
    misleading for an RW file.
  - since_epoch -- mirrors /sys/class/rtc/<dev>/since_epoch, but
    that one is read-only in the RTC subsystem so the dual
    semantic might surprise users coming from there.
  - rtc_sync -- describes the write semantic directly, but leaves
    the read side unnamed.
  - rtc -- shortest and most neutral; doesn't bias toward either
    operation.

Any preference, or should I just pick?

Thanks,
Abdurrahman

Re: [PATCH v4 0/3] hwmon: (pmbus/adm1266) add clear_blackbox and powerup_counter debugfs entries

[Guenter Roeck]

On 5/20/26 14:18, Abdurrahman Hussain wrote:
> On Wed May 20, 2026 at 1:59 PM PDT, Guenter Roeck wrote:
>> On 5/20/26 12:40, Abdurrahman Hussain wrote:
>>> On Wed May 20, 2026 at 10:10 AM PDT, Abdurrahman Hussain wrote:
>>>> On Wed May 20, 2026 at 7:10 AM PDT, Guenter Roeck wrote:
>>>>> Hi,
>>>>>
>>>>> On 5/16/26 18:18, Abdurrahman Hussain wrote:
>>>>>> This is what remains of the v3 series after Guenter applied patches
>>>>>> 1/5 (firmware_revision) and 5/5 (GPIO line label) to hwmon-next, and
>>>>>> asked for patch 4/5 (rtc_class) to be dropped.
>>>>>>
>>>>>> Patch 1 adds a write-only clear_blackbox debugfs file. Devices
>>>>>> configured for single-recording mode (BLACKBOX_CONFIG[0] = 0) need
>>>>>> an explicit clear once the 32-record buffer fills; the documented
>>>>>> sub-command ({0xFE, 0x00} block-write to 0xDE) wasn't reachable
>>>>>> from userspace. The patch also acquires pmbus_lock at the
>>>>>> adm1266_nvmem_read() callback boundary so the memset of
>>>>>> data->dev_mem, the blackbox refill, and the memcpy to userspace run
>>>>>> as a single critical section -- the nvmem core does not serialize
>>>>>> concurrent reg_read calls.
>>>>>>
>>>>>> Patch 2 exposes the non-volatile POWERUP_COUNTER (0xE4) via debugfs.
>>>>>> The same value is embedded in every blackbox record, so the live
>>>>>> value lets userspace match a captured record back to the boot it
>>>>>> came from when correlating logs. The block-read is taken under
>>>>>> pmbus_lock to serialise against any pmbus_core PAGE+register
>>>>>> sequence on the device.
>>>>>>
>>>>>> Patch 3 takes pmbus_lock in adm1266_state_read() (the pre-existing
>>>>>> sequencer_state debugfs handler) for the same defensive-locking
>>>>>> reason that motivates the new debugfs files in patches 1 and 2:
>>>>>> any direct device access from outside pmbus_core should be ordered
>>>>>> with respect to pmbus_core's own PAGE+register sequences.
>>>>>>
>>>>>> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>>>>> The series no longer applies after applying the bug fix series.
>>>>> Please rebase it on top of the hwmon-next branch of
>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
>>>>> and resubmit.
>>>>>
>>>>> Sorry for the trouble, and thanks a lot for fixing all the problems
>>>>> with the driver.
>>>>>
>>>>> Guenter
>>>> Will do, thank you for your support!
>>>>
>>>> Abdurrahman
>>> Hi Guenter,
>>>
>>> Before I send v5 of the adm1266 series, I'd like to revisit the
>>> SET_RTC exposure question from your v3 review [1].
>>>
>>> The use case I keep landing on is the one the datasheet itself
>>> recommends: a userspace agent (chrony hook, systemd-timesyncd
>>> script, or a small periodic daemon) keeps the chip's seconds
>>> counter aligned with wall-clock so the value embedded in each
>>> blackbox record stays correct across long uptimes.  The probe-
>>> time ktime_get_real_seconds() seed (now in hwmon-next) only fixes
>>> this at boot; the chip's counter drifts after that.
>>>
>>> You ruled out rtc_class and called a kernel-side periodic timer
>>> "a bit excessive", which I agree, it is.  That leaves a
>>> userspace-triggered sync.  Two debugfs shapes I'd consider, both under
>>> pmbus/<hwmon>/adm1266/ (alongside the clear_blackbox,
>>> powerup_counter, and sequencer_state entries v5 already adds):
>>>
>>>     A. RW since_epoch -- mirrors /sys/class/rtc/<dev>/since_epoch.
>>>        Read returns the chip's SET_RTC seconds counter, write sets
>>>        it.  The read has the useful side benefit of letting
>>>        userspace measure host-vs-chip drift without writing.
>>>
>>>     B. Write-only set_rtc -- writing anything to the file makes
>>>        the driver read ktime_get_real_seconds() itself and push it
>>>        to SET_RTC.  Simpler interface (kernel owns the time
>>>        source; userspace just triggers the sync), no payload to
>>>        parse, no way for userspace to pass in a wrong value.
>>>
>> How about a combination ? read returns the current value, anything
>> written synchronizes with the kernel rtc.
>>
> Going with that for v5.  One naming question before I send:
>
>    - set_rtc -- matches the PMBus command name (SET_RTC = 0xDF), but
>      "set" reads as write-only at first glance, which is a bit
>      misleading for an RW file.
>    - since_epoch -- mirrors /sys/class/rtc/<dev>/since_epoch, but
>      that one is read-only in the RTC subsystem so the dual
>      semantic might surprise users coming from there.
>    - rtc_sync -- describes the write semantic directly, but leaves
>      the read side unnamed.
>    - rtc -- shortest and most neutral; doesn't bias toward either
>      operation.
>
> Any preference, or should I just pick?

I like rtc_sync the best, but I'll accept your choice.

Thanks,

Guenter