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

On 5/12/26 01:27, Abdurrahman Hussain wrote:
> On Mon May 11, 2026 at 5:52 PM PDT, sashiko-bot wrote:
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [High] The debugfs write handler `adm1266_clear_blackbox_write` performs an I2C transaction without acquiring the required hardware monitoring locks, violating hwmon subsystem guidelines.
>> --
>>
>> commit ab9c19373562d58415e28511b037545cfd278c2a
>> Author: Abdurrahman Hussain <[email protected]>
>> Subject: hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry
>>
>> This commit adds a debugfs entry to clear the ADM1266 blackbox buffer by
>> issuing a block-write command to the device. This is needed when the
>> device operates in single-recording mode to preserve fault history.
>>
>>> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
>>> index 7b8433226176f..31aefa8fc75f5 100644
>>> --- 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);
>>
>> Does this I2C block write need to be protected by hwmon_lock() and
>> hwmon_unlock()?
>>
>> The hardware monitoring subsystem guidelines require drivers to implement
>> locking for attributes registered outside of the core registration functions,
>> such as debugfs entries.
>>
>> Without these locks, could this write operation race with other multi-record
>> transactions? For example, if this clear command is interleaved while
>> adm1266_nvmem_read_blackbox() is iterating through multiple records, might
>> it corrupt the read sequence?
>>
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	return count;
>>> +}
> 
> Good catch!
> 
> Will use pmbus_lock_interruptible() in v3.
> 

There is also "guard(pmbus_lock)(client);" or "scoped_guard(pmbus_lock, client) { ... }".
Guenter