Re: [PATCH v4 3/3] hwmon: (pmbus/adm1266) serialize sequencer_state debugfs read with pmbus_lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Meaningful code and commit message mismatch regarding defensive serialization.
--

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

hwmon: (pmbus/adm1266) serialize sequencer_state debugfs read with pmbus_lock
    
adm1266_state_read() backs the sequencer_state debugfs entry and
issues an i2c_smbus_read_word_data() against the device without taking
pmbus_lock. The pmbus_core framework holds pmbus_lock around its own
multi-transaction sequences, so an unlocked debugfs reader can land
between a page write and the subsequent paged read in another thread.

> but the same defensive serialisation that
> applies to the GPIO accessors applies here: any direct device
> access from outside pmbus_core should be ordered with respect to
> pmbus_core's own.

This isn't a bug, but is this statement in the commit message accurate?

Looking at the gpio accessors like adm1266_gpio_get() and
adm1266_gpio_get_multiple(), they appear to issue direct i2c reads without
taking pmbus_lock:

static int adm1266_gpio_get(struct gpio_chip *chip, unsigned int offset)
{
	...
	ret = i2c_smbus_read_block_data(data->client, pmbus_cmd, read_buf);
	...
}

Similarly, adm1266_gpio_dbg_show() acquires data->buf_mutex via
adm1266_pmbus_block_xfer(), which only protects local buffers rather than
serialising against the pmbus core.

Furthermore, if the gpio accessors were modified to take pmbus_lock(),
wouldn't that cause a null pointer dereference crash during driver probe?

adm1266_probe() registers the gpio chip via adm1266_config_gpio() before
calling pmbus_do_probe():

static int adm1266_probe(struct i2c_client *client)
{
	...
	ret = adm1266_config_gpio(data);
	if (ret < 0)
		return ret;

	ret = adm1266_set_rtc(data);
	if (ret < 0)
		return ret;

	ret = pmbus_do_probe(client, &data->info);
	...
}

Since pmbus_do_probe() allocates the internal pmbus_data structure that
pmbus_lock() relies on, it seems the gpio accessors cannot safely use this
lock.

Could the commit message be updated to clarify the actual locking model,
rather than citing the gpio accessors as precedent?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-adm1266-v4-0-1f8df4797258@nexthop.ai?part=3