Re: [RESEND v4 4/5] rtc: pcf85363: add oscillator offset calibration support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] The offset callbacks operate in raw register steps instead of parts per billion (ppb), violating the RTC subsystem API.
- [High] The offset value is wrongly treated as an 8-bit signed integer, which stomps on the OFFM (mode) bit and silently triggers Fast Mode.
- [High] The IRQ handler clears the timestamp interrupt flags before the timestamp data is read, creating a hardware race condition.
- [Low] User space can trigger unratelimited dev_warn() by writing out-of-bounds offset values.
- [Low] The `ts_valid_flags` variable is modified and read concurrently without appropriate lockless access annotations.

Pre-existing issues:
- [High] The static `nvmem_cfg` array is modified concurrently during device probe, risking corruption of private data pointers.
--

--- Patch [4]: [RESEND v4 4/5] rtc: pcf85363: add oscillator offset calibration support ---
commit cf1f1a013f4f67e8c4a20b8646213cc7ca971733
Author: Lakshay Piplani <lakshay.piplani@nxp.com>

rtc: pcf85363: add oscillator offset calibration support

This commit adds oscillator offset calibration support to the PCF85363 RTC
driver. It exposes the read_offset and set_offset callbacks, allowing
userspace to apply frequency correction for drift compensation.

> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index e10e58f690127..665bbbb169b05 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
> @@ -123,6 +123,11 @@
>  #define TSR2_SHIFT      2
>  #define TSR3_SHIFT      6
>  
> +#define OFFSET_SIGN_BIT 7
> +#define OFFSET_MINIMUM  -128
> +#define OFFSET_MAXIMUM  127
> +#define OFFSET_MASK     0xFF

[Severity: High]
Does treating this as an 8-bit signed integer stomp on the OFFM (mode) bit?

The PCF85363 CTRL_OFFSET register uses bit 7 as the OFFM (mode) bit and bits
6-0 as a 7-bit two's complement offset (range -64 to +63). Writing a negative
8-bit integer sets bit 7 to 1, silently reconfiguring the chip into Fast Mode,
while a positive integer switches it to Normal Mode.

Should the driver explicitly preserve the OFFM bit and restrict the offset to
a 7-bit value?

[ ... ]
> @@ -359,6 +364,45 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
>  	return handled ? IRQ_HANDLED : IRQ_NONE;
>  }
>  
> +/*
> + * Read the current RTC offset from the CTRL_OFFSET
> + * register. This value is an 8-bit signed 2's complement
> + * value that corrects osciallator drift.
> + */
> +static int pcf85363_read_offset(struct device *dev, long *offset)
> +{
> +	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(pcf85363->regmap, CTRL_OFFSET, &val);
> +
> +	if (ret)
> +		return ret;
> +
> +	*offset = sign_extend32(val & OFFSET_MASK, OFFSET_SIGN_BIT);
> +
> +	return 0;
> +}
> +
> +/*
> + * Write an oscillator offset correction value to
> + * the CTRL_OFFSET register. The valid range is
> + * -128 to 127 (8-bit signed), typically used to fine
> + * tune accuracy.
> + */
> +static int pcf85363_set_offset(struct device *dev, long offset)
> +{
> +	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> +
> +	if (offset < OFFSET_MINIMUM || offset > OFFSET_MAXIMUM) {

[Severity: High]
Does this code reject valid ppb offsets from userspace?

The rtc_read_offset and rtc_set_offset kernel API expects offset values in
parts per billion (ppb), but this implementation appears to operate directly
in raw register values.

Consequently, if userspace passes a valid offset like 4340 ppb, wouldn't it be
bounds-checked against [-128, 127] and rejected with -ERANGE, rendering the
API unusable?

> +		dev_warn(dev, "Offset out of range: %ld\n", offset);

[Severity: Low]
Can this unratelimited warning log be triggered directly by userspace?

It appears an out-of-bounds write to the sysfs offset node will spam the
kernel log, which could be avoided by simply returning -ERANGE.

> +		return -ERANGE;
> +	}
> +
> +	return regmap_write(pcf85363->regmap, CTRL_OFFSET, offset & OFFSET_MASK);
> +}

[Severity: High]
This isn't a bug introduced by this patch, but does clearing the timestamp
interrupt flags before reading the timestamp data create a hardware race
condition?

In pcf85363_rtc_handle_irq():

    if (flags & FLAGS_TSR1F) {
        regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_TSR1F, 0);
        pcf85363->ts_valid_flags |= FLAGS_TSR1F;
        handled = true;
    }

The driver exposes timestamp registers via sysfs which performs a lockless
regmap_bulk_read over I2C in pcf85363_read_timestamp(). According to the
PCF85363 behavior, clearing FLAGS_TSR1F rearms the hardware latch.

If a new timestamp event occurs while userspace is in the middle of reading
the timestamp registers, couldn't the hardware update the registers
concurrently and result in a torn read?

[Severity: Low]
This is a pre-existing issue, but is the lockless access to ts_valid_flags
missing necessary concurrency annotations?

The flag is modified in the threaded IRQ handler:

drivers/rtc/rtc-pcf85363.c:pcf85363_rtc_handle_irq() {
...
    pcf85363->ts_valid_flags |= FLAGS_TSR1F;
...
}

And read locklessly in sysfs:

drivers/rtc/rtc-pcf85363.c:pcf85363_timestamp_show() {
...
    if (!(pcf85363->ts_valid_flags & timestamp_flag))
...
}

While it's a 1-byte read/write that won't practically tear on supported
architectures, missing READ_ONCE() and WRITE_ONCE() annotations violates
kernel concurrency standards and could trigger KCSAN warnings.

[Severity: High]
This is a pre-existing issue, but does modifying the static nvmem_cfg array
during device probe risk corrupting private data pointers?

drivers/rtc/rtc-pcf85363.c:pcf85363_probe() {
...
    static struct nvmem_config nvmem_cfg[] = {
...
    for (i = 0; i < config->num_nvram; i++) {
        nvmem_cfg[i].priv = pcf85363;
        devm_rtc_nvmem_register(pcf85363->rtc, &nvmem_cfg[i]);
    }
...
}

If multiple PCF85363 instances are probed on the system, wouldn't the later
probe overwrite the priv pointer of the earlier instances in the shared
static array? This would cause subsequent NVRAM reads/writes for the first
RTC device to incorrectly target the last probed device's regmap.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812074533.3545058-1-lakshay.piplani@nxp.com?part=4