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

Hi all,

On Thu, 2026-08-20 at 11:39 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [High] Read-modify-write operations on CTRL2 inadvertently clear Write-0-to-Clear (W0C) TF and AF interrupt flags, causing dropped interrupts.
> - [High] A non-atomic read-modify-write sequence in the IRQ handler causes a data race that overwrites concurrent register updates.
> 
> Pre-existing issues:
> - [High] Requesting the threaded IRQ before registering the RTC device creates a devres ordering inversion leading to a Use-After-Free.
> - [High] Missing of_clk_del_provider() cleanup causes a Use-After-Free of the OF clock provider on driver unbind.
> --
> 
> --- Patch [1]: [PATCH v2] rtc: pcf85063: add 1 Hz update interrupt (UIE) support ---
> commit 79dacfbe2c0fd3c5e8261fd4569fedb1fe4b32a5
> 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. This patch offloads UIE to the hardware by enabling this timer
> when uie_rtctimer is active, reporting RTC_UF when the timer flag is set.
> 
> > diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
> > --- a/drivers/rtc/rtc-pcf85063.c
> > +++ b/drivers/rtc/rtc-pcf85063.c
> 
> [ ... ]
> 
> > @@ -212,22 +274,31 @@ static int pcf85063_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> >  
> >  	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> >  				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF,
> > -				  alrm->enabled ? PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF : PCF85063_CTRL2_AF);
> > +				  pcf85063->rtc->aie_timer.enabled ?
> > +				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF : PCF85063_CTRL2_AF);
> >  }
> 
> [Severity: High]
> Does this code inadvertently drop the TF interrupt flag when updating the
> alarm?
> 
> If PCF85063_CTRL2_TF is a write-0-to-clear bit, excluding it from the
> regmap_update_bits() mask means it will be written back as 0 if it was
> already set in the register, clearing any pending timer interrupt.

will be fixed in v3

> > @@ -274,22 +287,31 @@ static int pcf85063_rtc_alarm_irq_enable(struct device *dev,
> >  					 unsigned int enabled)
> >  {
> >  	struct pcf85063 *pcf85063 = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = pcf85063_set_timer_1hz(pcf85063, pcf85063->rtc->uie_rtctimer.enabled);
> > +	if (ret)
> > +		return ret;
> >  
> >  	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> >  				  PCF85063_CTRL2_AIE,
> > -				  enabled ? PCF85063_CTRL2_AIE : 0);
> > +				  pcf85063->rtc->aie_timer.enabled ?
> > +				  PCF85063_CTRL2_AIE : 0);
> >  }
> 
> [Severity: High]
> Could this function also clear pending interrupts?
> 
> Similar to pcf85063_rtc_set_alarm(), both AF and TF are excluded from the
> mask here. If they are write-0-to-clear flags, regmap_update_bits() will
> write them back as read (0), neutralizing any concurrent assertions.

will be fixed in v3

> > @@ -236,14 +307,26 @@ static irqreturn_t pcf85063_rtc_handle_irq(int irq, void *dev_id)
> >  {
> >  	struct pcf85063 *pcf85063 = dev_id;
> > +	unsigned long events = RTC_IRQF;
> > +	irqreturn_t ret = IRQ_NONE;
> >  	unsigned int val;
> >  	int err;
> >  
> >  	err = regmap_read(pcf85063->regmap, PCF85063_REG_CTRL2, &val);
> >  	if (err)
> >  		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;
> > +		val &= ~(PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF);
> > +		ret = IRQ_HANDLED;
> > +	} else {
> > +		/* Preserve AF if it comes after the above regmap_read() */
> > +		val |= PCF85063_CTRL2_AF;
> > +	}
> > +
> > +	if (val & PCF85063_CTRL2_TF) {
> > +		events |= RTC_UF;
> > +		val &= ~PCF85063_CTRL2_TF;
> > +		ret = IRQ_HANDLED;
> > +	}
> 
> [Severity: High]
> Does this code risk clearing an unhandled UIE interrupt if TF asserts
> after the register read?
> 
> If TF asserts post-read, the read value will have it as 0. Because there is
> no else branch to preserve TF (like the one preserving AF), the subsequent
> regmap_write() will commit a 0 for TF, clearing the flag before it is
> handled.

will be fixed in v3

> > +	if (ret == IRQ_HANDLED) {
> > +		regmap_write(pcf85063->regmap, PCF85063_REG_CTRL2, val);
> > +		rtc_update_irq(pcf85063->rtc, 1, events);
> >  	}
> >  
> > -	return IRQ_NONE;
> > +	return ret;
> >  }
> 
> [Severity: High]
> Is there a data race in this read-modify-write sequence?
> 
> pcf85063_rtc_handle_irq() drops the regmap lock between regmap_read() and
> regmap_write(). If a process-context change happens in this unlocked window,
> such as AIE toggling via pcf85063_rtc_alarm_irq_enable(), will it be
> overwritten and lost when the IRQ handler commits its stale read data?

This is pre-existing issue, will fix is in a separate patch preceeding v3

> > @@ -607,6 +690,7 @@ static int pcf85063_probe(struct device *dev, struct regmap *regmap, int irq,
> >  		err = devm_request_threaded_irq(dev, irq,
> >  						NULL, pcf85063_rtc_handle_irq,
> >  						irqflags | IRQF_ONESHOT,
> >  						"pcf85063", pcf85063);
> >  		if (err) {
> >  			dev_warn(&pcf85063->rtc->dev,
> >  				 "unable to request IRQ, alarms disabled\n");
> >  		} else {
> > +			set_bit(RTC_FEATURE_UPDATE_INTERRUPT, pcf85063->rtc->features);
> >  			set_bit(RTC_FEATURE_ALARM, pcf85063->rtc->features);
> 
> [Severity: High]
> This is a pre-existing issue, but does the devres allocation order here cause
> a use-after-free on teardown?
> 
> Earlier in pcf85063_probe(), devm_request_threaded_irq() is called before
> devm_rtc_register_device(). During teardown, rtc_device_unregister() runs
> first, but the IRQ remains active.
> 
> If the timer fires at this time, pcf85063_rtc_handle_irq() executes and
> rtc_update_irq() re-schedules the irqwork. Devres will subsequently free the
> rtc structure, leaving the system workqueue to dereference freed memory.

Seems to be false-positive:

rtc_device_unregister() does not free the struct rtc_device; it only drops the
cdev and clears rtc->ops. The object is reference-counted and freed only by the
devm_rtc_release_device() action (put_device()), which is registered first and
therefore runs last.

devres teardown is thus:

1. devm_rtc_unregister_device() — clears ops, removes cdev; does not free.
2. free_irq() — synchronizes with the threaded handler.
3. devm_rtc_release_device() — put_device() -> rtc_device_release(), which runs
cancel_work_sync(&rtc->irqwork) before kfree().
So in the only live-IRQ window between 1 and 2, rtc_update_irq() just schedules
irqwork on a still-valid object; step 2 stops the handler and step 3 flushes the
work before freeing.

> >  	nvmem_cfg.priv = pcf85063->regmap;
> >  	devm_rtc_nvmem_register(pcf85063->rtc, &nvmem_cfg);
> >  
> >  #ifdef CONFIG_COMMON_CLK
> >  	/* register clk in common clk framework */
> >  	pcf85063_clkout_register_clk(pcf85063);
> >  #endif
> >  
> >  	return devm_rtc_register_device(pcf85063->rtc);
> >  }
> 
> [Severity: High]
> This is a pre-existing issue, but is there a missing cleanup for the OF clock
> provider?
> 
> In pcf85063_clkout_register_clk(), of_clk_add_provider() is invoked but there
> appears to be no matching of_clk_del_provider(). When devres frees the clock
> structure during driver unbind, does the global OF list retain a dangling
> pointer, triggering a use-after-free on the next device tree clock
> resolution?

will fix is in a separate patch preceeding v3

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com