Re: [PATCH 05/12] rtc: rzn1: Add system suspend/resume support and wakeup capability

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Incorrect error rollback and returning errors from rzn1_rtc_resume() breaks wakeup configuration.
- [Medium] A hardware limitation is logged as a critical error on every suspend.

Pre-existing issues:
- [High] Asynchronous pm_runtime_put() is used before devres pm_runtime_disable(), leaving hardware powered on.
--

commit 65fe5cbeb3fc2e52fb88cf7b501a98dd6399fa39
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

rtc: rzn1: Add system suspend/resume support and wakeup capability

Add system-wide power management support along with wakeup capability to
the rtc-rzn1 driver, bridging sub-minute gaps with a 1-second periodic
broadcast loop.

> diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> index 866ff595188d8..12a319e9112c1 100644
> --- a/drivers/rtc/rtc-rzn1.c
> +++ b/drivers/rtc/rtc-rzn1.c
[ ... ]
> @@ -500,6 +511,74 @@ static void rzn1_rtc_remove(struct platform_device *pdev)
>  	pm_runtime_put(&pdev->dev);
>  }

[Severity: High]
This is a pre-existing issue, but does using pm_runtime_put() here and in
the dis_runtime_pm error path of rzn1_rtc_probe() leave the hardware
powered on?

The driver initializes runtime PM using devm_pm_runtime_enable(), which
registers a devres action to call pm_runtime_disable() upon probe failure
or device removal. By calling the asynchronous pm_runtime_put(), the
suspend operation is queued but immediately cancelled by the subsequent
pm_runtime_disable() devres action.

Should pm_runtime_put_sync() be used instead when pm_runtime_disable()
follows immediately to ensure the device is properly suspended?

> +static int rzn1_rtc_suspend(struct device *dev)
> +{
> +	struct rzn1_rtc *rtc = dev_get_drvdata(dev);
> +	int ret;
[ ... ]
> +	if (rtc->alarm_enabled)
> +		dev_crit(dev, "second/minute-level wakeup accuracy cannot be guaranteed by HW\n");

[Severity: Medium]
Does this unconditionally log a critical message on every suspend if the
alarm is enabled?

Since this is a known architectural limitation rather than an acute failure,
this will spam the kernel log with critical severity messages during normal
system suspend operations.

Would it be better to downgrade this to dev_warn() or dev_info()?

> +	ret = enable_irq_wake(rtc->alarm_irq);
> +	if (ret)
> +		return ret;
> +	if (rtc->sec_irq >= 0) {
> +		ret = enable_irq_wake(rtc->sec_irq);
> +		if (ret) {
> +			disable_irq_wake(rtc->alarm_irq);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int rzn1_rtc_resume(struct device *dev)
> +{
> +	struct rzn1_rtc *rtc = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!device_may_wakeup(dev))
> +		return 0;
> +
> +	ret = disable_irq_wake(rtc->alarm_irq);
> +	if (ret)
> +		return ret;
> +	if (rtc->sec_irq >= 0) {
> +		ret = disable_irq_wake(rtc->sec_irq);
> +		if (ret) {
> +			enable_irq_wake(rtc->alarm_irq);
> +			return ret;
> +		}
> +	}

[Severity: Medium]
Is it correct to re-enable the alarm_irq wakeup source and return an error
here in rzn1_rtc_resume() if disable_irq_wake() fails?

Because the PM core ignores device resume failures and continues the
system resume sequence, returning an error here and re-enabling alarm_irq
leaves it permanently configured as a wakeup source while the system is
active. This unbalanced state could lead to spurious wakeups.

Should the rollback logic be reconsidered or removed for the resume path?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615154805.1619693-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=5