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

On 5/12/26 03:20, Abdurrahman Hussain wrote:
> 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 <[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
> 
> Thanks for the tip! Does that mean you're ok with losing the
> interruptibility, since there's no DEFINE_GUARD_COND for interruptible
> variant?
> 

Yes. If anything, I'd like to drop the _interruptible variant entirely.
I only keep it for historic reasons.

Guenter