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

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.

[ ... ]

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

[ ... ]

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

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

[ ... ]

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

[ ... ]

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1