Re: [PATCH] rtc: pcf85063: add 1 Hz update interrupt (UIE) support

On Fri, 2026-08-07 at 13:15 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] Naive read-modify-write operations on the CTRL2 register can drop the write-0-to-clear AF and TF interrupt flags.
> - [High] The UIE timer interrupt is configured in pulse mode (`TI_TP = 1`), causing an interrupt storm with level-triggered IRQs.
> 
> Pre-existing issues:
> - [High] Time-of-check to time-of-use (TOCTOU) race in the threaded IRQ handler clobbers concurrent alarm enable operations.
> --
> 
> --- Patch [1]: [PATCH] rtc: pcf85063: add 1 Hz update interrupt (UIE) support ---
> commit 283190d20c79bb78192c9f090cf19e5f426cb933
> Author: Alexander Sverdlin <[email protected]>
> 
> rtc: pcf85063: add 1 Hz update interrupt (UIE) support
> 
> The PCF85063A/RV8263 have no dedicated 1 Hz update interrupt, but their
> countdown timer can be sourced from a 1 Hz clock to generate a periodic
> interrupt. Use it to offload UIE to the hardware.
> 
> > diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
> > index 01e209d88f5f3..76675b75388e4 100644
> > --- a/drivers/rtc/rtc-pcf85063.c
> > +++ b/drivers/rtc/rtc-pcf85063.c
> [ ... ]
> > @@ -188,12 +197,64 @@ static int pcf85063_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * The chip has no dedicated 1 Hz update interrupt. Drive the countdown timer
> > + * at 1 Hz to emulate it, so the RTC core can offload UIE to the hardware
> > + * instead of polling. A free-running timer is left untouched to preserve its
> > + * phase across the per-second re-arming done by the core.
> > + */
> > +static int pcf85063_set_timer_1hz(struct pcf85063 *pcf85063, bool enable)
> > +{
> > +	unsigned int mask = PCF85063_TIMER_MODE_TCF_MASK |
> > +			    PCF85063_TIMER_MODE_TIE |
> > +			    PCF85063_TIMER_MODE_TI_TP |
> > +			    PCF85063_TIMER_MODE_TE;
> > +	unsigned int mode = 0;
> > +	unsigned int cur;
> > +	int ret;
> > +
> > +	if (enable)
> > +		mode = PCF85063_TIMER_MODE_TCF_1HZ | PCF85063_TIMER_MODE_TIE |
> > +		       PCF85063_TIMER_MODE_TI_TP | PCF85063_TIMER_MODE_TE;
> 
> [Severity: High]
> Does configuring the timer in pulse mode (PCF85063_TIMER_MODE_TI_TP = 1) cause
> an interrupt storm on platforms using level-triggered interrupts?
> 
> If the hardware emits a fixed-width pulse on the INT pin instead of following
> the TF flag, when the threaded handler clears TF and returns, the INT pin might
> remain physically low for the remainder of the pulse.
> 
> Upon unmasking, would the level-triggered IRQ controller immediately fire again,
> leading to repeated IRQ_NONE returns and eventually disabling the IRQ?

I don't think TI_TP bit is controlling edge vs level INT pin behaviour, but
rather:
--- datasheet ---
7.2.2.3 Timer Flag (TF)
The bit TF is set logic 1 on the first trigger of MI, HMI, or the countdown timer.
The purpose of the flag is to allow the controlling system to interrogate what
caused the interrupt: timer or alarm. The flag can be read and cleared by command.
The status of the timer flag TF can affect the INT pulse generation depending on
the setting of TI_TP (see Section 7.6.2):
• When TI_TP is set to logic 1, the following conditions occur:
– An INT pulse is generated independent of the status of the timer flag TF.
– TF stays set until it is cleared.
– TF does not affect INT.
– The countdown timer runs in a repetitive loop and keeps generating timed periods.
• When TI_TP is set to logic 0, the following conditions occur:
– The INT generation follows the TF flag.
– TF stays set until it is cleared.
– If TF is not cleared before the next coming interrupt, no INT is generated.
– The countdown timer stops after the first countdown.
--- end datasheet ---

While datasheet never mentions, if the ~INT pin is designed for edge- or
level-triggered input logic, all the diagrams suggest that the INT output is
rather edge-triggering.

> > +
> > +	ret = regmap_read(pcf85063->regmap, PCF85063_REG_TIMER_MODE, &cur);
> > +	if (ret)
> > +		return ret;
> [ ... ]
> > +	if (enable) {
> > +		ret = regmap_write(pcf85063->regmap, PCF85063_REG_TIMER_VALUE, 1);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> > +					 PCF85063_CTRL2_TF, 0);
> 
> [Severity: High]
> Can this read-modify-write operation drop write-0-to-clear interrupt flags?
> 
> If the AF flag was read as 0 internally by regmap_update_bits(), but asserts
> in the hardware before the write, wouldn't the write of 0 to the unmasked bits
> inadvertently clear the newly arrived alarm interrupt?

Makes sense! I'll rework to preserve the AF status bit.

> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_TIMER_MODE,
> > +				  mask, mode);
> > +}
> [ ... ]
> > @@ -236,14 +307,23 @@ static irqreturn_t pcf85063_rtc_handle_irq(int irq, void *dev_id)
> >  		return IRQ_NONE;
> >  
> >  	if (val & PCF85063_CTRL2_AF) {
> > -		rtc_update_irq(pcf85063->rtc, 1, RTC_IRQF | RTC_AF);
> > -		regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> > -				   PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF,
> > -				   0);
> > -		return IRQ_HANDLED;
> > +		events |= RTC_AF;
> > +		mask |= PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF;
> 
> [Severity: High]
> This is a pre-existing issue, but does caching the decision to clear AIE here
> create a time-of-check to time-of-use race that clobbers concurrent alarm
> enable operations?
> 
> If a concurrent thread sets a new alarm (e.g., writing AIE=1) after CTRL2 was
> read at the start of this handler, wouldn't the delayed regmap_update_bits()
> at the end of the handler overwrite AIE back to 0, discarding the user's
> alarm request?
> 
> > +		ret = IRQ_HANDLED;
> > +	}
> > +
> > +	if (val & PCF85063_CTRL2_TF) {
> > +		events |= RTC_UF;
> > +		mask |= PCF85063_CTRL2_TF;
> > +		ret = IRQ_HANDLED;
> > +	}
> > +
> > +	if (ret == IRQ_HANDLED) {
> > +		regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2, mask, 0);
> 
> [Severity: High]
> Can this read-modify-write operation lose interrupts that assert during the
> RMW window?
> 
> Since both AF and TF are write-0-to-clear flags, if an interrupt arrives after
> the regmap_update_bits() read step but before the write step, the unmasked bits
> will be written back as 0, permanently losing the interrupt.

I don't see how this HW allows for completely race-free handling, but the
current handling can be improved indeed, at least regarding the AF interrupt,
I'll do it in v2.

> > +		rtc_update_irq(pcf85063->rtc, 1, events);
> >  	}
> >  
> > -	return IRQ_NONE;
> > +	return ret;
> >  }

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com