Re: [PATCH 6/7] hwmon: (pmbus/adm1266) register rtc_class device backed by SET_RTC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] 64-bit division using the `/` operator will cause a build failure on 32-bit architectures.
- [High] RTC operations bypass mandatory subsystem locking rules.
- [Medium] Unchecked arithmetic truncation of 64-bit system time during device probe.
--

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

hwmon: (pmbus/adm1266) register rtc_class device backed by SET_RTC

This commit registers an rtc_class device backed by the SET_RTC block-R/W
register to allow userspace to frequently synchronize the ADM1266 UNIX
time. It maps the standard /dev/rtcN ->set_time and ->read_time operations
to the 6-byte SET_RTC frame. The probe-time wall-clock seed is also
preserved.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index a058616d42156..07c20746f0831 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -513,28 +514,85 @@ static int adm1266_config_nvmem(struct adm1266_data *data)
>  	return 0;
>  }
>  
> -static int adm1266_set_rtc(struct adm1266_data *data)
> +/*
> + * SET_RTC frame layout (datasheet Rev. D, Table 84):
> + *   bytes [1:0] = fractional seconds, LSB = 1/65536 s
> + *   bytes [5:2] = seconds since 1970-01-01 UTC
> + */
> +static int adm1266_write_rtc(struct i2c_client *client, const struct timespec64 *ts)
>  {
> -	struct timespec64 ts;
> -	char write_buf[6];
> +	u8 buf[6];
>  	u16 frac;
>  	int i;
>  
> +	frac = (u16)(((u64)ts->tv_nsec << 16) / NSEC_PER_SEC);

Will this 64-bit division cause a link error on 32-bit architectures?

Using the / operator on a 64-bit integer can lead to undefined references to
__udivdi3 on 32-bit builds. Could this use div_u64() instead?

> +	for (i = 0; i < 2; i++)
> +		buf[i] = (frac >> (i * 8)) & 0xFF;
> +	for (i = 0; i < 4; i++)
> +		buf[2 + i] = (ts->tv_sec >> (i * 8)) & 0xFF;

Since ts->tv_sec is a 64-bit value, does this loop silently truncate values
exceeding 32 bits when called during probe?

adm1266_probe()
  adm1266_set_rtc()
    adm1266_write_rtc()

The hwmon subsystem guidelines ask to check for overflows in arithmetic
calculations.

> +
> +	return i2c_smbus_write_block_data(client, ADM1266_SET_RTC, sizeof(buf), buf);
> +}

[ ... ]

> +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;
> +
> +	ret = i2c_smbus_read_block_data(client, ADM1266_SET_RTC, buf);

The hardware monitoring subsystem guidelines state that drivers must implement
locking for attributes registered by any other means.

Are these callbacks missing a call to hwmon_lock() (or pmbus_lock())?

If an RTC class device implicitly exposes time attributes to userspace, could
these concurrent I2C accesses risk disrupting stateful hardware transactions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-adm1266-v1-0-ec08bf29e0ce@nexthop.ai?part=6