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

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

On Mon May 11, 2026 at 8:32 PM PDT, sashiko-bot wrote:
> 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?
>
> [ ... ]

Agreed. In v3 both callbacks wrap their I2C transfer in
pmbus_lock_interruptible() / pmbus_unlock(). adm1266_write_rtc() is
no longer reachable from probe (the probe-time SET_RTC seed is what
this patch removes), so the new lock acquisition is purely on the
userspace path and a signal can interrupt a contended wait cleanly.

>> @@ -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?

The ordering follows the existing pattern for the GPIO and nvmem
registrations in this driver. None of the three registration helpers
performs I2C at registration time — devm_{gpiochip,nvmem,rtc}_*()
only install callbacks that fire later when userspace touches the
interface. With the v3 locking in place, any userspace RTC access
that lands during pmbus_do_probe() will serialise on
pmbus_lock_interruptible() against the core's PAGE+register
sequences rather than interleave with them, so the
window-without-protection that the original concern describes is
closed by the locking change.

I would prefer to leave the registration ordering as-is for
consistency with the GPIO and nvmem paths, but I am happy to move
all three after pmbus_do_probe() if you would rather have a stricter
"nothing visible to userspace until probe is done" invariant across
the driver. Happy to do that in a follow-up if so.

Best regards,
Abdurrahman