Re: [PATCH 3/7] hwmon: (pmbus/adm1266) add firmware_revision debugfs entry

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added `firmware_revision` debugfs attribute does not implement the required locking, violating hwmon subsystem guidelines.
--

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

hwmon: (pmbus/adm1266) add firmware_revision debugfs entry

This commit adds a debugfs entry to read and report the ADM1266 firmware
revision. It fetches the data via the manufacturer-specific IC_DEVICE_REV
block-read command.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index ba8a025095287..5cfca00a44682 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c

[ ... ]

> +static int adm1266_firmware_revision_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_IC_DEVICE_REV, buf);

Does this function need to take the hardware monitoring lock before accessing
the device?

The subsystem guidelines state that drivers must implement locking for
attributes registered by other means than the standard sysfs info parameter.
Would it be appropriate to wrap this I2C read with hwmon_lock() and
hwmon_unlock() to prevent concurrent access issues?

> +	if (ret < 0)
> +		return ret;
> +	if (ret < 3)
> +		return -EIO;
> +
> +	seq_printf(s, "%u.%u.%u\n", buf[0], buf[1], buf[2]);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-adm1266-v1-0-ec08bf29e0ce@nexthop.ai?part=3