Re: [PATCH 4/7] hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing locking in the debugfs clear_blackbox attribute handler creates a race condition with nvmem blackbox reads and violates hwmon subsystem guidelines.
--

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

hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry

This commit exposes a write-only debugfs file alongside sequencer_state to
clear the ADM1266 blackbox. Writing any data to this file issues a 2-byte
block-write command to the READ_BLACKBOX register.

> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
[ ... ]
> +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;
> +
> +	ret = i2c_smbus_write_block_data(client, ADM1266_READ_BLACKBOX,
> +					 sizeof(payload), payload);

Could this clear command race with concurrent reads of the blackbox?

Looking at adm1266_nvmem_read_blackbox(), it performs a multi-transaction
sequence where it first reads the record_count, then loops to read individual
records:

drivers/hwmon/pmbus/adm1266.c:adm1266_nvmem_read_blackbox() {
    ...
    ret = i2c_smbus_read_block_data(data->client, ADM1266_BLACKBOX_INFO, buf);
    ...
    record_count = buf[3];

    for (index = 0; index < record_count; index++) {
        ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1,
                                       &index, read_buff);
    ...
}

If this write runs concurrently, it seems it could clear the hardware
blackbox mid-read since the core I2C bus lock only serializes individual
transactions. This might cause the remaining nvmem reads to fetch corrupted
data or fail with I/O errors.

The hwmon subsystem guidelines also mention that drivers must implement
locking for attributes registered outside the core subsystem. Should this
write be protected to serialize it against other hardware accesses?

> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}

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