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

[sashiko-bot@kernel.org]

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@bp.renesas.com>
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
[ ... ]
> @@ -415,15 +416,15 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
[ ... ]
>  	/* Only switch to scmp if we have an xtal clock with a valid rate and != 32768 */
> -	xtal = devm_clk_get_optional(&pdev->dev, "xtal");
> +	xtal = devm_clk_get_optional(dev, "xtal");

[Severity: High]
This is a pre-existing issue, but if the optional xtal clock is not present,
devm_clk_get_optional() sets xtal to NULL. In this scenario, rtc->sync_time
remains uninitialized and defaults to 0.

Further down in rzn1_rtc_probe(), readl_poll_timeout() is called:

	ret = readl_poll_timeout(rtc->base + RZN1_RTC_CTL0, val,
				 !(val & RZN1_RTC_CTL0_CEST), rtc->sync_time,
				 rtc->sync_time * 2);

With rtc->sync_time at 0, the timeout evaluation becomes 0. Does this cause
readl_poll_timeout() to loop indefinitely and hang the boot if the hardware
fails to clear the RZN1_RTC_CTL0_CEST bit?

>  	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?

>  	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@bp.renesas.com?part=9