Re: [PATCH RESEND v2 3/5] hwmon: (pmbus/adm1266) add powerup_counter debugfs entry

["Abdurrahman Hussain" <abdurrahman@nexthop.ai>]

On Mon May 11, 2026 at 6:29 PM PDT, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Missing lock in debugfs read function violating hwmon subsystem guidelines.
> --
>
> commit 59d29098021812d7f7bd1712ee63ceafd8397c54
> Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>
>     hwmon: (pmbus/adm1266) add powerup_counter debugfs entry
>     
>     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.
>
>> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
>> index 31aefa8fc75f5..99e92a8365fb7 100644
>> --- a/drivers/hwmon/pmbus/adm1266.c
>> +++ b/drivers/hwmon/pmbus/adm1266.c
> [ ... ]
>> @@ -356,6 +357,31 @@ static int adm1266_firmware_revision_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;
>> +
>> +	ret = i2c_smbus_read_block_data(client, ADM1266_POWERUP_COUNTER, buf);
>
> Does this debugfs read need to be protected by hwmon_lock()?
>
> According to the hwmon subsystem guidelines:
>
> "The hardware monitoring subsystem core serializes sysfs operations
> for attributes registered with the info parameter of
> hwmon_device_register_with_info() and
> devm_hwmon_device_register_with_info().
> Drivers must implement locking required for interrupt handling and for
> attributes registered by any other means. Drivers should use hwmon_lock()
> and hwmon_unlock() for this purpose."
>
> Since this attribute is registered via debugfs rather than the info
> parameter, concurrent I2C operations here could potentially interleave
> with multi-part transactions from the hwmon core (like PMBus page switching),
> which could disrupt the device's state machine or cause cross-page data
> corruption.
>
>> +	if (ret < 0)
>> +		return ret;
>> +	if (ret != 2)
>> +		return -EIO;
>> +
>> +	seq_printf(s, "%u\n", buf[0] | (buf[1] << 8));
>> +
>> +	return 0;
>> +}

POWERUP_COUNTER (0xE4) is a global, non-paged register. The read here
is a single i2c_smbus_read_block_data() call, which the i2c core
already serialises at the bus level for that transfer. There is no
driver-side multi-step sequence to interleave with, and PMBus PAGE
state has no bearing on the value returned, so an extra lock around
this read would not be protecting anything.

(For what it's worth, hwmon_lock()/hwmon_unlock() aren't exported as
a generic kernel API. The relevant primitive for a PMBus driver is
pmbus_lock() / pmbus_unlock() from drivers/hwmon/pmbus/pmbus.h, which
fences against the PMBus core's own PAGE+register sequences.)

I have added pmbus_lock_interruptible() coverage in v3 for the paths
where it is justified:

- clear_blackbox (patch 2/5) shares command 0xDE with
  adm1266_nvmem_read_blackbox(), and the read path walks records
  one at a time; a concurrent clear could land between records, so
  both paths now hold the lock across the full sequence.
- The RTC read_time/set_time callbacks (patch 4/5) can fire while
  the PMBus core has a PAGE write outstanding, so the I2C transfer
  is wrapped in pmbus_lock_interruptible() to avoid interleaving.

I would prefer to keep the powerup_counter read lock-free unless
there is a specific scenario I am missing.

Best regards,
Abdurrahman