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

[sashiko-bot@kernel.org]

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-adm1266-v2-0-7a6d34f0de5d@nexthop.ai?part=3