Re: [PATCH 09/12] rtc: rzn1: Use temporary variable for struct device

[Lad, Prabhakar]

Hi Wolfram,

On Wed, Jun 17, 2026 at 12:08 PM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> Sashiko found issues, not introduced by your patch, but still. I
> curated the list and left only those which I think are valid. Do you
> have time to look at these issues?
>
> Thank you and happy hacking,
>
>    Wolfram
>
> > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> >
> > Pre-existing issues:
> > - [High] Asynchronous pm_runtime_put() is used instead of pm_runtime_put_sync() before runtime PM is disabled, potentially leaving the device powered on.
> > - [High] If the optional 'xtal' clock is missing, readl_poll_timeout() loops forever with a 0 timeout, risking a boot hang.
> > - [High] The driver's `remove` callback prematurely disables hardware interrupts and drops the runtime PM reference while the RTC device is still fully registered.
> > --
> >
> > commit b7f73c7d0932a6e78b12419c5912ad3243637b8f
> > Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > rtc: rzn1: Use temporary variable for struct device
> >
> > This patch simplifies the rzn1_rtc_probe function by using a local temporary
> > variable for the struct device pointer. This avoids repeated dereferences of
> > pdev->dev throughout the function.
> >
> > > diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> > > index bc6af59744e48..25cad9213147d 100644
> > > --- a/drivers/rtc/rtc-rzn1.c
> > > +++ b/drivers/rtc/rtc-rzn1.c
> > [ ... ]
> > >     if (IS_ERR(xtal)) {
> > >             ret = PTR_ERR(xtal);
> > >             goto dis_runtime_pm;
> > [ ... ]
> > > @@ -486,18 +487,18 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
> > [ ... ]
> > >     device_init_wakeup(dev, true);
> > >
> > >     ret = devm_rtc_register_device(rtc->rtcdev);
> >
> > [Severity: High]
> > This isn't a bug introduced by this patch, but does registering the RTC
> > device using devm_rtc_register_device() create a race condition during
> > teardown?
> >
> > In rzn1_rtc_remove(), the driver manually disables interrupts and calls
> > pm_runtime_put() before devres has unregistered the RTC device:
> >
> >       writel(0, rtc->base + RZN1_RTC_CTL1);
> >       pm_runtime_put(&pdev->dev);
> >
> > Could userspace still interact with the RTC during this window via ioctls
> > or sysfs, potentially causing synchronous bus faults since the hardware is
> > already suspended? Should teardown be bound using
> > devm_add_action_or_reset() to guarantee correct reverse execution order?
>
> [wsa] Or maybe not use devm then?
>
Maybe just add two `devm_add_action_or_reset()` calls: one to clear
interrupts and another to call pm_runtime_put()?

> >
> > >     if (ret)
> > > @@ -506,7 +507,7 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
> > >     return 0;
> > >
> > >  dis_runtime_pm:
> > > -   pm_runtime_put(&pdev->dev);
> > > +   pm_runtime_put(dev);
> >
> > [Severity: High]
> > This is a pre-existing issue, but does using pm_runtime_put() here (and in
> > rzn1_rtc_remove) leave the device powered on indefinitely?
> >
> > Since devm_pm_runtime_enable(dev) schedules pm_runtime_disable() to
> > execute during devres cleanup, calling pm_runtime_put() only queues an
> > asynchronous idle check. The immediate return triggers devres cleanup,
> > which executes a barrier that explicitly cancels pending async operations.
> >
> > Should this use pm_runtime_put_sync() instead to ensure the device is
> > synchronously suspended before teardown?
> >
Although there were some patches accepted for similar kind of issue
reported by Sashiko, do you think I should switch to
pm_runtime_put_sync() (Ive not seen any issues)

Cheers,
Prabhakar