[PATCH] platform/x86/amd/pmc: Fix RTC reference leak in amd_pmc_verify_czn_rtc()

[PATCH] platform/x86/amd/pmc: Fix RTC reference leak in amd_pmc_verify_czn_rtc()

[Ma Ke]

amd_pmc_verify_czn_rtc() opens the RTC with rtc_class_open(), which
takes both a device reference and a module reference on the RTC
driver. rtc_class_close() is the matching release, but it is not
called.

Use a single exit path and close the RTC there, in the same way
ntp.c:sync_hw_clock() does with its out_close: label.

Found by code review.

Fixes: 59348401ebed ("platform/x86: amd-pmc: Add special handling for timer based S0i3 wakeup")
Cc: stable@vger.kernel.org
Signed-off-by: Ma Ke <make_ruc2021@163.com>
---
 drivers/platform/x86/amd/pmc/pmc.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index 6792aa2c6187..543bd95e4766 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -671,33 +671,41 @@ static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
 	if (rc) {
 		if (rc == -ENOENT)
 			dev_dbg(pdev->dev, "no alarm pending\n");
-		return rc == -ENOENT ? 0 : rc;
+		rc = rc == -ENOENT ? 0 : rc;
+		goto out_close;
 	}
 	if (!alarm.enabled) {
 		dev_dbg(pdev->dev, "alarm not enabled\n");
-		return 0;
+		rc = 0;
+		goto out_close;
 	}
 	rc = rtc_read_time(rtc_device, &tm);
 	if (rc)
-		return rc;
+		goto out_close;
 	then = rtc_tm_to_time64(&alarm.time);
 	now = rtc_tm_to_time64(&tm);
 	duration = then-now;
 
 	/* in the past */
-	if (then < now)
-		return 0;
+	if (then < now) {
+		rc = 0;
+		goto out_close;
+	}
 
 	/* will be stored in upper 16 bits of s0i3 hint argument,
 	 * so timer wakeup from s0i3 is limited to ~18 hours or less
 	 */
-	if (duration <= 4 || duration > U16_MAX)
-		return -EINVAL;
+	if (duration <= 4 || duration > U16_MAX) {
+		rc = -EINVAL;
+		goto out_close;
+	}
 
 	*arg |= (duration << 16);
 	rc = rtc_alarm_irq_enable(rtc_device, 0);
 	pm_pr_dbg("wakeup timer programmed for %lld seconds\n", duration);
 
+out_close:
+	rtc_class_close(rtc_device);
 	return rc;
 }
 
-- 
2.43.0

Re: [PATCH] platform/x86/amd/pmc: Fix RTC reference leak in amd_pmc_verify_czn_rtc()

[Mario Limonciello]

On 9/14/26 07:20, Ma Ke wrote:
> amd_pmc_verify_czn_rtc() opens the RTC with rtc_class_open(), which
> takes both a device reference and a module reference on the RTC
> driver. rtc_class_close() is the matching release, but it is not
> called.
> 
> Use a single exit path and close the RTC there, in the same way
> ntp.c:sync_hw_clock() does with its out_close: label.
> 
> Found by code review.
> 
> Fixes: 59348401ebed ("platform/x86: amd-pmc: Add special handling for timer based S0i3 wakeup")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ma Ke <make_ruc2021@163.com>

Rather than adding all the gotos, can this be done with a __free() 
cleanup call on the declaration?

> ---
>   drivers/platform/x86/amd/pmc/pmc.c | 22 +++++++++++++++-------
>   1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index 6792aa2c6187..543bd95e4766 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -671,33 +671,41 @@ static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
>   	if (rc) {
>   		if (rc == -ENOENT)
>   			dev_dbg(pdev->dev, "no alarm pending\n");
> -		return rc == -ENOENT ? 0 : rc;
> +		rc = rc == -ENOENT ? 0 : rc;
> +		goto out_close;
>   	}
>   	if (!alarm.enabled) {
>   		dev_dbg(pdev->dev, "alarm not enabled\n");
> -		return 0;
> +		rc = 0;
> +		goto out_close;
>   	}
>   	rc = rtc_read_time(rtc_device, &tm);
>   	if (rc)
> -		return rc;
> +		goto out_close;
>   	then = rtc_tm_to_time64(&alarm.time);
>   	now = rtc_tm_to_time64(&tm);
>   	duration = then-now;
>   
>   	/* in the past */
> -	if (then < now)
> -		return 0;
> +	if (then < now) {
> +		rc = 0;
> +		goto out_close;
> +	}
>   
>   	/* will be stored in upper 16 bits of s0i3 hint argument,
>   	 * so timer wakeup from s0i3 is limited to ~18 hours or less
>   	 */
> -	if (duration <= 4 || duration > U16_MAX)
> -		return -EINVAL;
> +	if (duration <= 4 || duration > U16_MAX) {
> +		rc = -EINVAL;
> +		goto out_close;
> +	}
>   
>   	*arg |= (duration << 16);
>   	rc = rtc_alarm_irq_enable(rtc_device, 0);
>   	pm_pr_dbg("wakeup timer programmed for %lld seconds\n", duration);
>   
> +out_close:
> +	rtc_class_close(rtc_device);
>   	return rc;
>   }
>   

Re: [PATCH] platform/x86/amd/pmc: Fix RTC reference leak in amd_pmc_verify_czn_rtc()

[Ilpo Järvinen]

On Mon, 14 Sep 2026, Mario Limonciello wrote:
> On 9/14/26 07:20, Ma Ke wrote:
> > amd_pmc_verify_czn_rtc() opens the RTC with rtc_class_open(), which
> > takes both a device reference and a module reference on the RTC
> > driver. rtc_class_close() is the matching release, but it is not
> > called.
> > 
> > Use a single exit path and close the RTC there, in the same way
> > ntp.c:sync_hw_clock() does with its out_close: label.
> > 
> > Found by code review.
> > 
> > Fixes: 59348401ebed ("platform/x86: amd-pmc: Add special handling for timer
> > based S0i3 wakeup")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Ma Ke <make_ruc2021@163.com>
> 
> Rather than adding all the gotos, can this be done with a __free() cleanup
> call on the declaration?

Yes, that's what I too want to see.

I think adding a local DEFINE_FREE() is necessary besides just using 
__free().

-- 
 i.

> > ---
> >   drivers/platform/x86/amd/pmc/pmc.c | 22 +++++++++++++++-------
> >   1 file changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/amd/pmc/pmc.c
> > b/drivers/platform/x86/amd/pmc/pmc.c
> > index 6792aa2c6187..543bd95e4766 100644
> > --- a/drivers/platform/x86/amd/pmc/pmc.c
> > +++ b/drivers/platform/x86/amd/pmc/pmc.c
> > @@ -671,33 +671,41 @@ static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev
> > *pdev, u32 *arg)
> >   	if (rc) {
> >   		if (rc == -ENOENT)
> >   			dev_dbg(pdev->dev, "no alarm pending\n");
> > -		return rc == -ENOENT ? 0 : rc;
> > +		rc = rc == -ENOENT ? 0 : rc;
> > +		goto out_close;
> >   	}
> >   	if (!alarm.enabled) {
> >   		dev_dbg(pdev->dev, "alarm not enabled\n");
> > -		return 0;
> > +		rc = 0;
> > +		goto out_close;
> >   	}
> >   	rc = rtc_read_time(rtc_device, &tm);
> >   	if (rc)
> > -		return rc;
> > +		goto out_close;
> >   	then = rtc_tm_to_time64(&alarm.time);
> >   	now = rtc_tm_to_time64(&tm);
> >   	duration = then-now;
> >     	/* in the past */
> > -	if (then < now)
> > -		return 0;
> > +	if (then < now) {
> > +		rc = 0;
> > +		goto out_close;
> > +	}
> >     	/* will be stored in upper 16 bits of s0i3 hint argument,
> >   	 * so timer wakeup from s0i3 is limited to ~18 hours or less
> >   	 */
> > -	if (duration <= 4 || duration > U16_MAX)
> > -		return -EINVAL;
> > +	if (duration <= 4 || duration > U16_MAX) {
> > +		rc = -EINVAL;
> > +		goto out_close;
> > +	}
> >     	*arg |= (duration << 16);
> >   	rc = rtc_alarm_irq_enable(rtc_device, 0);
> >   	pm_pr_dbg("wakeup timer programmed for %lld seconds\n", duration);
> >   +out_close:
> > +	rtc_class_close(rtc_device);
> >   	return rc;
> >   }
> >