Re: [PATCH v2 4/5] hwmon: (pmbus/adm1266) replace probe-time RTC seed with rtc_class device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing Kconfig dependency on `RTC_CLASS` leads to undefined reference linker errors.
- [High] Premature RTC device registration leads to race conditions during probe and potential Use-After-Free during teardown.
- [High] RTC read/write operations perform unprotected I2C bus transactions, violating hardware monitoring subsystem locking guidelines.
--

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

hwmon: (pmbus/adm1266) replace probe-time RTC seed with rtc_class device

This commit removes the unconditional probe-time RTC seeding. It adds
an rtc_class device to allow userspace to synchronize the device's internal
clock.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
[ ... ]
> @@ -513,21 +513,71 @@ static int adm1266_config_nvmem(struct adm1266_data *data)
[ ... ]
> +static int adm1266_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	u8 buf[I2C_SMBUS_BLOCK_MAX];
> +	u32 secs;
> +	int ret;
> +	int i;
>  
> -	memset(write_buf, 0, sizeof(write_buf));
> +	ret = i2c_smbus_read_block_data(client, ADM1266_SET_RTC, buf);

Does this code violate the hardware monitoring subsystem locking guidelines?

The hwmon subsystem guidelines state that drivers must implement locking
required for attributes registered outside of the core, and should use
hwmon_lock() and hwmon_unlock() for this purpose.

Since adm1266_rtc_read_time() and adm1266_rtc_set_time() invoke raw i2c
transactions without acquiring a lock, could these operations interleave
with multi-transaction PMBus sequences and lead to corrupted hardware state?

[ ... ]
> +static int adm1266_register_rtc(struct adm1266_data *data)
> +{
> +	struct rtc_device *rtc;
> +
> +	rtc = devm_rtc_allocate_device(&data->client->dev);

Is there a missing Kconfig dependency for the RTC subsystem?

This code calls devm_rtc_allocate_device() and devm_rtc_register_device(),
but if the driver is enabled while the RTC subsystem is disabled, the build
will fail with undefined reference errors during linking.

Unlike some other subsystems, linux/rtc.h does not provide static inline
stubs for these functions when the subsystem is disabled.

[ ... ]
> @@ -553,7 +603,7 @@ static int adm1266_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = adm1266_set_rtc(data);
> +	ret = adm1266_register_rtc(data);
>  	if (ret < 0)
>  		return ret;

Can this registration sequence lead to race conditions or a use-after-free
during teardown?

By calling adm1266_register_rtc() before pmbus_do_probe() (which happens
later in this function), the rtc device is exposed to userspace while
the PMBus hardware probe is still executing. Userspace could trigger
read or write operations before the PMBus core is fully initialized.

Additionally, devm_rtc_register_device() registers a devres action. If
pmbus_do_probe() later fails, or during normal device unbind, the LIFO
devres teardown will release the PMBus core structures before the rtc
device is unregistered.

If rtc callbacks are accessed during this window, could they dereference
freed PMBus structures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-adm1266-v2-0-3a22b903c2f1@nexthop.ai?part=4