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

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?

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

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

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

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