Re: [PATCH v1 2/3] rtc: ftrtc010: use devm clock APIs to fix use-after-disable in remove

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incorrect clock API usage and error handling truncates probe on missing or optional clocks.

Pre-existing issues:
- [High] Time-of-Check to Time-of-Use (TOCTOU) race against hardware ticking causes time corruption.
- [High] Unconditional IRQ_HANDLED on a shared interrupt line defeats the spurious interrupt detector.
--

commit 289123a8d46a85463292da46c4edfed7ee0a88db
Author: Liu Dalin <liudalin@kylinsec.com.cn>

rtc: ftrtc010: use devm clock APIs to fix use-after-disable in remove

This commit replaces manual clock management with devm_clk_get_enabled()
to tie the clock lifecycle to devres, fixing a use-after-disable window
during device removal. It also cleans up the probe error handling path
and removes the manual remove callback.

> diff --git a/drivers/rtc/rtc-ftrtc010.c b/drivers/rtc/rtc-ftrtc010.c
> --- a/drivers/rtc/rtc-ftrtc010.c
> +++ b/drivers/rtc/rtc-ftrtc010.c
> @@ -120,51 +120,33 @@ static int ftrtc010_rtc_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	platform_set_drvdata(pdev, rtc);
>  
> -	rtc->pclk = devm_clk_get(dev, "PCLK");
> -	if (IS_ERR(rtc->pclk)) {
> +	rtc->pclk = devm_clk_get_enabled(dev, "PCLK");
> +	if (IS_ERR_OR_NULL(rtc->pclk)) {
>  		dev_err(dev, "could not get PCLK\n");
> -	} else {
> -		ret = clk_prepare_enable(rtc->pclk);
> -		if (ret) {
> -			dev_err(dev, "failed to enable PCLK\n");
> -			return ret;
> -		}
> +		return PTR_ERR_OR_ZERO(rtc->pclk);
>  	}

[Severity: High]
If CONFIG_HAVE_CLK is disabled, devm_clk_get_enabled() returns NULL. Doesn't
PTR_ERR_OR_ZERO(NULL) evaluate to 0, which would cause ftrtc010_rtc_probe() 
to silently return 0 and exit prematurely before mapping memory or registering 
the RTC device?

Additionally, since the old code treated PCLK as optional (only logging an 
error if it failed), what happens if the clock is omitted from the Device 
Tree? devm_clk_get_enabled() returns -ENOENT, which PTR_ERR_OR_ZERO() will 
propagate as an error. Could this break backward compatibility with older 
device trees that omit this clock?

> -	rtc->extclk = devm_clk_get(dev, "EXTCLK");
> -	if (IS_ERR(rtc->extclk)) {
> +	rtc->extclk = devm_clk_get_enabled(dev, "EXTCLK");
> +	if (IS_ERR_OR_NULL(rtc->extclk)) {
>  		dev_err(dev, "could not get EXTCLK\n");
> -	} else {
> -		ret = clk_prepare_enable(rtc->extclk);
> -		if (ret) {
> -			dev_err(dev, "failed to enable EXTCLK\n");
> -			goto err_disable_pclk;
> -		}
> +		return PTR_ERR_OR_ZERO(rtc->extclk);
>  	}

[Severity: High]
Does this have the same silent probe truncation or Device Tree compatibility 
issues as the PCLK conversion above?

[ ... ]

> @@ -179,29 +161,13 @@ static int ftrtc010_rtc_probe(struct platform_device *pdev)
>  	ret = devm_request_irq(dev, rtc->rtc_irq, ftrtc010_rtc_interrupt,
>  			       IRQF_SHARED, pdev->name, dev);

[Severity: High]
This is a pre-existing issue, but I noticed the driver requests a shared 
interrupt with IRQF_SHARED. Looking at ftrtc010_rtc_interrupt(), it returns 
IRQ_HANDLED unconditionally:

static irqreturn_t ftrtc010_rtc_interrupt(int irq, void *dev)
{
        return IRQ_HANDLED;
}

If another device sharing this IRQ line asserts an interrupt and gets stuck, 
would this unconditional return prevent the kernel's spurious interrupt 
detector from disabling the broken IRQ line, potentially leading to an 
interrupt storm and a system hang?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but looking through the driver, 
is there a race condition against the hardware ticking in both 
ftrtc010_rtc_read_time() and ftrtc010_rtc_set_time()?

In ftrtc010_rtc_read_time():

	sec  = readl(rtc->rtc_base + FTRTC010_RTC_SECOND);
	min  = readl(rtc->rtc_base + FTRTC010_RTC_MINUTE);
	hour = readl(rtc->rtc_base + FTRTC010_RTC_HOUR);
	days = readl(rtc->rtc_base + FTRTC010_RTC_DAYS);

If the hardware clock rolls over (for example, incrementing the minute from 
59 seconds to 00 seconds) between the reads of two registers, could the 
software combine the old value of one register with the newly incremented 
value of another, resulting in wildly corrupted time calculations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827063019.3342085-1-liudalin@kylinsec.com.cn?part=2