Re: [PATCH v3 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:
- [Medium] Missing locking in debugfs read callback `adm1266_powerup_counter_read` violates subsystem concurrency guidelines and allows interleaving with PMBus sequences.
--

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

hwmon: (pmbus/adm1266) add powerup_counter debugfs entry

This commit exposes the adm1266 16-bit non-volatile powerup counter as a
read-only debugfs file. The live value is read using a block-read to
retrieve two payload bytes in little-endian order.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index bd3784d59a7c8..080e7dbd0c069 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 lockless hardware access introduce a regression by violating hwmon
subsystem concurrency guidelines?

The hwmon guidelines specify that drivers must implement locking for
attributes registered by any other means, such as debugfs files. While a
single i2c transaction is serialized at the adapter level, a lockless
access here could interleave with stateful multi-transaction pmbus
sequences on other threads, such as setting a page register followed by
a read.

Should this function acquire pmbus_lock(client) to safely serialize the
hardware access and prevent disruption of the device internal state machine?

> +	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/20260512-adm1266-v3-0-a81a479b0bb0@nexthop.ai?part=3