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

[Wolfram Sang]

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?

> 
> >  	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?
> 
> >  	return ret;
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260615154805.1619693-1-prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx?part=9

Attachment: signature.asc
Description: PGP signature