Re: [PATCH 02/12] rtc: rzn1: Handle EPROBE_DEFER for optional pps interrupt

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 3263 bytes --]

Hi,

Sashiko found issues with your patch. I curated the list and agree. I
think, in probe(), 'rate' should be initialized to 32768 and 'sync_time'
should be calculated unconditionally.

Thank you and happy hacking,

   Wolfram

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] If the optional 'xtal' clock is not present, `rtc->sync_time` remains 0, causing `readl_poll_timeout` to use a 0 timeout (infinite wait) and `usleep_range` to use a 0 delay.
> --
> 
> commit 5d75a1049c228d2049afaf29833f1b2f212837d4
> Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Subject: rtc: rzn1: Handle EPROBE_DEFER for optional pps interrupt
> 
> This commit checks for -EPROBE_DEFER from platform_get_irq_byname_optional()
> and handles the deferred probe request properly. It intercepts this specific
> error condition, assigns it to the return value, and jumps to the
> dis_runtime_pm label to avoid ignoring a valid probe deferral.
> 
> Fixes: eea7791e00f33 ("rtc: rzn1: implement one-second accuracy for alarms")
> Cc: stable@vger.kernel.org
> 
> > diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> > index c4ed437354574..f81d691c8b9a2 100644
> > --- a/drivers/rtc/rtc-rzn1.c
> > +++ b/drivers/rtc/rtc-rzn1.c
> > @@ -465,6 +465,10 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
> >  	}
> >  
> >  	irq = platform_get_irq_byname_optional(pdev, "pps");
> > +	if (irq == -EPROBE_DEFER) {
> > +		ret = irq;
> > +		goto dis_runtime_pm;
> > +	}
> >  	if (irq >= 0)
> >  		ret = devm_request_irq(&pdev->dev, irq, rzn1_rtc_1s_irq, 0, "RZN1 RTC 1s", rtc);
> 
> [Severity: High]
> If the optional xtal clock is not present, does rtc->sync_time remain 0,
> causing readl_poll_timeout() to use a 0 timeout (infinite wait) and
> usleep_range() to use a 0 delay?
> 
> devm_clk_get_optional() returns NULL if the clock is absent in the device
> tree, skipping the calculation:
> 
> drivers/rtc/rtc-rzn1.c:rzn1_rtc_probe() {
>     ...
>     xtal = devm_clk_get_optional(dev, "xtal");
>     if (IS_ERR(xtal)) {
>         ...
>     } else if (xtal) {
>         rate = clk_get_rate(xtal);
>         ...
>         rtc->sync_time = DIV_ROUND_UP(2 * NSEC_PER_MSEC, rate);
>     }
>     ...
>     ret = readl_poll_timeout(rtc->base + RZN1_RTC_CTL0, val,
>                              !(val & RZN1_RTC_CTL0_CEST), rtc->sync_time,
>                              rtc->sync_time * 2);
>     ...
> }
> 
> In include/linux/iopoll.h, a timeout_us of 0 skips the timeout check
> entirely, which could lead to a tight infinite loop and a kernel hard lockup
> if the hardware fails to clear the CEST bit.
> 
> Additionally, rtc->sync_time being 0 causes usleep_range(0, 100) in
> rzn1_rtc_set_time(), which may return almost immediately and violate the
> hardware's explicit 2-clock-cycle delay requirement (previously hardcoded to
> 61us).
> 
> This regression was introduced by the commit 'rtc: rzn1: Dynamically
> calculate synchronization delay based on clock rate' later in this series.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260615154805.1619693-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]