Re: [PATCH v5 06/17] rtc: rzn1: Disable alarm interrupt before reprogramming alarm registers

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

Pre-existing issues:
- [High] Concurrent lockless read and write of `rtc->tm_alarm` and a race with delayed IRQ handlers can cause a newly configured alarm to be silently disabled.
--

--- Patch [6]: [PATCH v5 06/17] rtc: rzn1: Disable alarm interrupt before reprogramming alarm registers ---
commit 6227942d78d91abfb1648a64bce64f0011716b61
Author: Lad Prabhakar <[email protected]>

rtc: rzn1: Disable alarm interrupt before reprogramming alarm registers

This commit updates rzn1_rtc_set_alarm() to disable the alarm interrupt
before reprogramming the alarm registers. This prevents the interrupt from
firing while the alarm registers contain a mixture of old and newly written
values.

> diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> index 44e8db466cb78..09631a3a51345 100644
> --- a/drivers/rtc/rtc-rzn1.c
> +++ b/drivers/rtc/rtc-rzn1.c
> @@ -280,6 +280,11 @@ static int rzn1_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  	if (alarm > farest)
>  		return -ERANGE;
>  
> +	/* Disable alarm interrupts before reprogramming the alarm. */
> +	ret = rzn1_rtc_alarm_irq_enable(dev, 0);
> +	if (ret)
> +		return ret;
> +
>  	writel(bin2bcd(tm->tm_min), rtc->base + RZN1_RTC_ALM);
>  	writel(bin2bcd(tm->tm_hour), rtc->base + RZN1_RTC_ALH);
>  	writel(BIT(tm->tm_wday), rtc->base + RZN1_RTC_ALW);

[Severity: High]
This is a pre-existing issue, but does this fully protect against a race
with a delayed IRQ handler?

If a previous alarm fires and the IRQ handler rzn1_rtc_alarm_irq() is
delayed on CPU1, and concurrently CPU0 runs rzn1_rtc_set_alarm():

CPU0 completes rzn1_rtc_set_alarm(), updating the lockless rtc->tm_alarm
and re-enabling the alarm by setting ALME in CTL1.

CPU1 then resumes in rzn1_rtc_alarm_irq():

	if (rtc->tm_alarm.tm_sec == 0)
		...
	else
		set_irq_bits = RZN1_RTC_CTL1_1SE;

	guard(spinlock)(&rtc->ctl1_access_lock);

	ctl1 = readl(rtc->base + RZN1_RTC_CTL1);
	ctl1 &= ~RZN1_RTC_CTL1_ALME;
	ctl1 |= set_irq_bits;
	writel(ctl1, rtc->base + RZN1_RTC_CTL1);

Could CPU1 read the newly updated rtc->tm_alarm locklessly, and then
unconditionally clear ALME in CTL1? This sequence appears to silently
disable the newly configured alarm, which could result in missed wakeups.

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