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

["Abdurrahman Hussain" <abdurrahman@nexthop.ai>]

On Tue May 12, 2026 at 2:19 AM PDT, Guenter Roeck wrote:
> 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 <abdurrahman@nexthop.ai>
>>> 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

Thanks for the tip! Does that mean you're ok with losing the
interruptibility, since there's no DEFINE_GUARD_COND for interruptible
variant?

Abdurrahman