Re: [PATCH v4 08/15] rtc: rzn1: Use pm_runtime_put_sync() in rzn1_rtc_disable_hardware()

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

Pre-existing issues:
- [Medium] Race condition in rzn1_rtc_set_alarm() leading to spurious hardware interrupts and lockless torn reads of rtc->tm_alarm.
--

--- Patch [8]: [PATCH v4 08/15] rtc: rzn1: Use pm_runtime_put_sync() in rzn1_rtc_disable_hardware() ---
commit dff0f7a5386396694cffd2ad789f85740e53b9b9
Author: Lad Prabhakar <[email protected]>

rtc: rzn1: Use pm_runtime_put_sync() in rzn1_rtc_disable_hardware()
    
pm_runtime_put() may trigger the idle check after pm_runtime_disable()
is run as part of devm_pm_runtime_enable()'s cleanup action, leaving
runtime PM active.

> diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> index da7d13337c0da..425d9590c55b9 100644
> --- a/drivers/rtc/rtc-rzn1.c
> +++ b/drivers/rtc/rtc-rzn1.c
> @@ -379,7 +379,7 @@ static void rzn1_rtc_disable_hardware(void *data)
>  	/* Disable all interrupts */
>  	writel(0, rtc->base + RZN1_RTC_CTL1);
>  
> -	pm_runtime_put(dev);
> +	pm_runtime_put_sync(dev);
>  }

[Severity: Medium]
This is a pre-existing issue, but while reviewing this file I noticed a
potential race condition in rzn1_rtc_set_alarm() that could lead to
spurious hardware interrupts and lockless torn reads of rtc->tm_alarm.

If rtc_timer_enqueue() calls __rtc_set_alarm() to replace an existing
alarm with an earlier one without disabling the hardware alarm first, ALME
might still be 1. During this time, rzn1_rtc_set_alarm() sequentially writes
to the registers:

drivers/rtc/rtc-rzn1.c:rzn1_rtc_set_alarm() {
    ...
	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);

	rtc->tm_alarm = alrm->time;
    ...
}

If the intermediate hybrid state of these registers matches the current
hardware time, would the RTC immediately fire an alarm interrupt?

Concurrently, if the interrupt fires, the IRQ handler reads the potentially
stale or partially updated rtc->tm_alarm structure without locks:

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;
    ...
}

Could this result in spurious RTC_AF events sent to userspace or incorrect
toggling of the 1-second interrupt mode?

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