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

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

Pre-existing issues:
- [High] Data race and TOCTOU in IRQ handler leading to spurious interrupts and incomplete synchronization
- [High] Hardware state lockup on timeout in rzn1_rtc_set_time()
- [High] Alarm logic erroneously uses 1-second interrupt for past alarms with the same minute
- [Medium] Race window in rzn1_rtc_read_time() fallback logic can produce inconsistent timestamps
--

--- Patch [6]: [PATCH v6 06/17] rtc: rzn1: Disable alarm interrupt before reprogramming alarm registers ---
commit 0be1a07839b0e52a936e0315319b64b90371633b
Author: Lad Prabhakar <[email protected]>
Subject: rtc: rzn1: Disable alarm interrupt before reprogramming alarm registers

This commit updates rzn1_rtc_set_alarm() to disable alarm interrupts before
reprogramming the alarm registers (ALM, ALH, ALW). This prevents a scenario
where an existing alarm interrupt fires while the registers contain a mixture
of old and new values.

> diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> index f3268655fd373..42e57bf0b4aaa 100644
> --- a/drivers/rtc/rtc-rzn1.c
> +++ b/drivers/rtc/rtc-rzn1.c
> @@ -281,6 +281,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);

[Severity: High]
This is a pre-existing issue, but does disabling the IRQ here fully prevent
the race without a call to synchronize_irq()?

If a previous alarm interrupt fires right as rzn1_rtc_set_alarm() is called,
CPU0 could enter rzn1_rtc_alarm_irq(), read tm_sec locklessly, and wait for
the spinlock.

CPU1 could then call rzn1_rtc_alarm_irq_enable(dev, 0), holding and releasing
the lock.

CPU0 could then acquire the lock and unconditionally write its locklessly
computed set_irq_bits to CTL1, re-enabling the interrupt:

drivers/rtc/rtc-rzn1.c:rzn1_rtc_alarm_irq() {
	if (rtc->tm_alarm.tm_sec == 0)
		rtc_update_irq(rtc->rtcdev, 1, RTC_AF | RTC_IRQF);
	else
		/* Switch to 1s interrupts */
		set_irq_bits = RZN1_RTC_CTL1_1SE;

	guard(spinlock)(&rtc->ctl1_access_lock);
	...
	ctl1 &= ~RZN1_RTC_CTL1_ALME;
	ctl1 |= set_irq_bits;
	writel(ctl1, rtc->base + RZN1_RTC_CTL1);
}

Could this allow the handler to override the synchronization intended by
this patch?

> +	if (ret)
> +		return ret;
> +
>  	writel(bin2bcd(tm->tm_min), rtc->base + RZN1_RTC_ALM);

[ ... ]


[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
rzn1_rtc_set_time() leave the hardware in a locked-up state?

drivers/rtc/rtc-rzn1.c:rzn1_rtc_set_time() {
	...
		ret = readl_poll_timeout(rtc->base + RZN1_RTC_CTL2, val,
					 val & RZN1_RTC_CTL2_WST, 0, 100);
		if (ret)
			return ret;
	...
}

If the system is under heavy load causing a preemption delay longer than
the 100us timeout, the function returns -ETIMEDOUT immediately without
clearing RZN1_RTC_CTL2_WAIT.

Could this stop the RTC hardware from ticking entirely across the system?


[Severity: Medium]
This is a pre-existing issue, but does the fallback logic in
rzn1_rtc_read_time() leave a race window for preemption to corrupt the
timestamp?

drivers/rtc/rtc-rzn1.c:rzn1_rtc_read_time() {
	...
	rzn1_rtc_get_time_snapshot(rtc, tm);
	secs = readl(rtc->base + RZN1_RTC_SECC);
	if (tm->tm_sec != bcd2bin(secs))
		rzn1_rtc_get_time_snapshot(rtc, tm);

	return 0;
}

If the thread is preempted precisely between the TIMEC and CALC reads in the
second snapshot, could this return an inconsistent timestamp to userspace
since there is no loop to verify the second snapshot?


[Severity: High]
This is a pre-existing issue, but does the alarm logic erroneously fall back
to the 1-second interrupt for past alarms?

drivers/rtc/rtc-rzn1.c:rzn1_rtc_alarm_irq_enable() {
	...
			if (rtc_tm_sub(tm, &tm_now) > 59 || tm->tm_min != tm_now.tm_min)
				ctl1 |= RZN1_RTC_CTL1_ALME;
			else
				ctl1 |= RZN1_RTC_CTL1_1SE;
	...
}

If an alarm is set for a time in the past where tm_min matches but
rtc_tm_sub() is negative, does this logic incorrectly apply the 1-second
interrupt and cause late or spurious wakeups?

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