Re: [PATCH RESEND 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 2 potential issue(s) to consider:
- [High] Missing locking in RTC callbacks causes race condition with PMBus transactions.
- [Medium] Premature exposure of the RTC device before hardware initialization is complete.
--

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

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

This commit replaces the probe-time RTC seeding with an rtc_class device
backed by SET_RTC. This allows userspace tooling to drive the re-sync against
/dev/rtcN once it trusts the system clock.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 99e92a8365fb7..0dfb02db86837 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
[ ... ]
> +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);

Since this RTC device is exposed to userspace, can this callback be invoked
concurrently with hwmon sysfs attributes managed by the PMBus core?

The PMBus core uses pmbus_lock to serialize multi-step sequences. If an RTC
I2C transaction interleaves in the middle of a PMBus sequence, can it disrupt
the device's internal state machine or cause unpredictable behavior?

Should this callback acquire pmbus_lock(client) (or its interruptible
variant) around the I2C transfer to prevent a race condition?

> +	if (ret < 0)
> +		return ret;
[ ... ]
> +static int adm1266_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +
> +	return adm1266_write_rtc(client, rtc_tm_to_time64(tm));
> +}

Does this callback also need to acquire pmbus_lock(client) around the I2C
transfer to prevent concurrent access with PMBus transactions?

[ ... ]
> @@ -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;

By registering the RTC device here, is the /dev/rtcX interface exposed to
userspace before pmbus_do_probe() completes the hardware discovery and
initialization sequence?

If a userspace process immediately opens the RTC device and issues read/write
commands, can the resulting I2C traffic interleave with the driver's
initial setup?

Should adm1266_register_rtc() be called at the end of the probe function,
after pmbus_do_probe() succeeds, to ensure the device is fully initialized
before it is accessed externally?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-adm1266-v2-0-7a6d34f0de5d@nexthop.ai?part=4