[PATCH] watchdog: npcm: fix sleeping clock call in restart handler

[PATCH] watchdog: npcm: fix sleeping clock call in restart handler

[zhaofeng2]

From: Zhao Feng <zhaofeng2@cmss.chinamobile.com>

The watchdog .restart callback runs in atomic context: it is invoked
from the restart handler chain via atomic_notifier_call_chain(), under
an RCU read-side critical section, on every reboot or restart.  Since
commit af084fdccfaf ("watchdog: npcm: Enable clock if provided"),
npcm_wdt_restart() calls clk_prepare_enable(), and clk_prepare()
takes a mutex unconditionally, so kernels built with
CONFIG_DEBUG_ATOMIC_SLEEP=y report

  BUG: sleeping function called from invalid context

on every reboot.  Worse, a panic-time restart can block on
prepare_lock and never issue the reset at all.

Prepare the clock once at probe time and keep it prepared for the
lifetime of the driver (balanced by a devm action), so the restart
handler only needs clk_enable(), which is safe in atomic context.
clk_prepare()/clk_unprepare() accept a NULL clock, so the
optional-clock case is unaffected.

Fixes: af084fdccfaf ("watchdog: npcm: Enable clock if provided")
Cc: stable@vger.kernel.org
Signed-off-by: Zhao Feng <zhaofeng2@cmss.chinamobile.com>
---
Issue reported by Sashiko's review of Triet Hoang's pending
error-propagation patch; maintainer confirmed the problem in
https://lore.kernel.org/linux-kernel/19258f94-ec01-48f4-9d04-22e5ecb5cac2@roeck-us.net/

Based on linux-next, which already carries
"watchdog: npcm: fix clock prepare leak when setting timeout on a
running watchdog".

Verified against linux-next.  Not runtime-tested (no Nuvoton NPCM
hardware).

 drivers/watchdog/npcm_wdt.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

--- a/drivers/watchdog/npcm_wdt.c
+++ b/drivers/watchdog/npcm_wdt.c
@@ -242,8 +242,12 @@
 {
 	struct npcm_wdt *wdt = to_npcm_wdt(wdd);
 
-	/* For reset, we start the WDT clock and leave it running. */
-	clk_prepare_enable(wdt->clk);
+	/*
+	 * For reset, we start the WDT clock and leave it running.
+	 * The clock was prepared at probe time; only clk_enable() is
+	 * safe in this atomic (restart notifier) context.
+	 */
+	clk_enable(wdt->clk);
 
 	writel(NPCM_WTR | NPCM_WTRE | NPCM_WTE, wdt->reg);
 	udelay(1000);
@@ -393,6 +397,11 @@
 	.restart = npcm_wdt_restart,
 };
 
+static void npcm_clk_unprepare(void *data)
+{
+	clk_unprepare(data);
+}
+
 static int npcm_wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -421,6 +430,20 @@
 	if (IS_ERR(wdt->clk))
 		return PTR_ERR(wdt->clk);
 
+	/*
+	 * Prepare the clock once and keep it prepared for the lifetime
+	 * of the driver: npcm_wdt_restart() runs in atomic context and
+	 * may only call clk_enable().
+	 */
+	ret = clk_prepare(wdt->clk);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(&pdev->dev, npcm_clk_unprepare,
+				       wdt->clk);
+	if (ret)
+		return ret;
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
 		return irq;

Re: [PATCH] watchdog: npcm: fix sleeping clock call in restart handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] watchdog: npcm: open-coded devm_clk_get_optional_prepared() in
  npcm_wdt_probe()
--

commit d887f7d373bb7c1fa8a0907fd176d60b84597a65
Author: Zhao Feng <zhaofeng2@cmss.chinamobile.com>

watchdog: npcm: fix sleeping clock call in restart handler

This commit prepares the watchdog clock at probe time and registers a devm
cleanup action so that npcm_wdt_restart() can call clk_enable() in atomic
context instead of clk_prepare_enable().

> diff --git a/drivers/watchdog/npcm_wdt.c b/drivers/watchdog/npcm_wdt.c
> index 4d8f212871447..4fb17f758cdbc 100644
> --- a/drivers/watchdog/npcm_wdt.c
> +++ b/drivers/watchdog/npcm_wdt.c

[ ... ]

> @@ -386,6 +390,11 @@ static const struct watchdog_ops npcm_wdt_ops = {
>  	.restart = npcm_wdt_restart,
>  };
>  
> +static void npcm_clk_unprepare(void *data)
> +{
> +	clk_unprepare(data);
> +}
> +
>  static int npcm_wdt_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -414,6 +423,20 @@ static int npcm_wdt_probe(struct platform_device *pdev)
>  	if (IS_ERR(wdt->clk))
>  		return PTR_ERR(wdt->clk);
>  
> +	/*
> +	 * Prepare the clock once and keep it prepared for the lifetime
> +	 * of the driver: npcm_wdt_restart() runs in atomic context and
> +	 * may only call clk_enable().
> +	 */
> +	ret = clk_prepare(wdt->clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(&pdev->dev, npcm_clk_unprepare,
> +				       wdt->clk);
> +	if (ret)
> +		return ret;

[Severity: Low]
Could devm_clk_get_optional_prepared(&pdev->dev, NULL) be used here
instead of open-coding clk_prepare(), devm_add_action_or_reset(), and
npcm_clk_unprepare()?  Also, since wdt->clk now stays prepared for the
lifetime of the driver, npcm_wdt_start() and npcm_wdt_stop() only need
clk_enable() and clk_disable() rather than clk_prepare_enable() and
clk_disable_unprepare().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111064150.260-1-zhaofeng2@cmss.chinamobile.com?part=1

[PATCH v2] watchdog: npcm: fix sleeping clock call in restart handler

[zhaofeng2]

From: Zhao Feng <zhaofeng2@cmss.chinamobile.com>

The watchdog .restart callback runs in atomic context: it is invoked
from the restart handler chain via atomic_notifier_call_chain(), under
an RCU read-side critical section, on every reboot or restart.  Since
commit af084fdccfaf ("watchdog: npcm: Enable clock if provided"),
npcm_wdt_restart() calls clk_prepare_enable(), and clk_prepare()
takes a mutex unconditionally, so kernels built with
CONFIG_DEBUG_ATOMIC_SLEEP=y report

  BUG: sleeping function called from invalid context

on every reboot.  Worse, a panic-time restart can block on
prepare_lock and never issue the reset at all.

Get the clock with devm_clk_get_optional_prepared() at probe time and
keep it prepared for the lifetime of the driver, so the restart
handler only needs clk_enable(), which is safe in atomic context.
With the clock permanently prepared, npcm_wdt_start() and
npcm_wdt_stop() are simplified to clk_enable()/clk_disable() as well.

Fixes: af084fdccfaf ("watchdog: npcm: Enable clock if provided")
Cc: stable@vger.kernel.org
Signed-off-by: Zhao Feng <zhaofeng2@cmss.chinamobile.com>
---
v2: use devm_clk_get_optional_prepared() instead of open-coding
    clk_prepare() + devm_add_action_or_reset(), and simplify
    start/stop to clk_enable()/clk_disable(), per Sashiko's review.

Issue reported by Sashiko's review of Triet Hoang's pending
error-propagation patch; maintainer confirmed the problem in
https://lore.kernel.org/linux-kernel/19258f94-ec01-48f4-9d04-22e5ecb5cac2@roeck-us.net/

Based on linux-next, which already carries
"watchdog: npcm: fix clock prepare leak when setting timeout on a
running watchdog".

Verified against linux-next.  Not runtime-tested (no Nuvoton NPCM
hardware).

 drivers/watchdog/npcm_wdt.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

--- a/drivers/watchdog/npcm_wdt.c
+++ b/drivers/watchdog/npcm_wdt.c
@@ -180,7 +180,7 @@
 {
 	struct npcm_wdt *wdt = to_npcm_wdt(wdd);
 
-	clk_prepare_enable(wdt->clk);
+	clk_enable(wdt->clk);
 
 	__npcm_wdt_start(wdd);
 
@@ -193,7 +193,7 @@
 
 	writel(0, wdt->reg);
 
-	clk_disable_unprepare(wdt->clk);
+	clk_disable(wdt->clk);
 
 	return 0;
 }
@@ -242,8 +242,12 @@
 {
 	struct npcm_wdt *wdt = to_npcm_wdt(wdd);
 
-	/* For reset, we start the WDT clock and leave it running. */
-	clk_prepare_enable(wdt->clk);
+	/*
+	 * For reset, we start the WDT clock and leave it running.
+	 * The clock was prepared at probe time; only clk_enable() is
+	 * safe in this atomic (restart notifier) context.
+	 */
+	clk_enable(wdt->clk);
 
 	writel(NPCM_WTR | NPCM_WTRE | NPCM_WTE, wdt->reg);
 	udelay(1000);
@@ -417,7 +421,12 @@
 
 	start = res->start;
 
-	wdt->clk = devm_clk_get_optional(&pdev->dev, NULL);
+	/*
+	 * Keep the clock prepared for the lifetime of the driver:
+	 * npcm_wdt_restart() runs in atomic context and may only call
+	 * clk_enable().
+	 */
+	wdt->clk = devm_clk_get_optional_prepared(&pdev->dev, NULL);
 	if (IS_ERR(wdt->clk))
 		return PTR_ERR(wdt->clk);