linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] watchdog: da9062: da9063: prevent watchdog pings ahead of reboot
@ 2021-07-06 11:21 Primoz Fiser
  2021-07-06 13:28 ` Guenter Roeck
  2021-07-08  8:21 ` [PATCH v2] watchdog: da9062: da9063: prevent pings ahead of machine reset Primoz Fiser
  0 siblings, 2 replies; 6+ messages in thread
From: Primoz Fiser @ 2021-07-06 11:21 UTC (permalink / raw)
  To: linux-watchdog, Guenter Roeck, Wim Van Sebroeck, Support Opensource
  Cc: Tero Kristo, Stefan Riedmueller, Wolfram Sang

Proper machine resets via da9062/da9063 PMICs are very tricky as they
require special i2c atomic transfers when interrupts are not available
anymore. This is also a reason why both PMIC's restart handlers do not
use regmap but instead opt for i2c_smbus_write_byte_data() which does
i2c transfer in atomic manner. Under the hood, this function tries to
obtain i2c bus lock with call to i2c_adapter_trylock_bus() which will
return -EAGAIN (-11) if lock is not available.

Since commit 982bb70517aef ("watchdog: reset last_hw_keepalive time at
start") occasional restart handler failures with "Failed to shutdown
(err = -11)" error messages were observed, indicating that some
process is holding the i2c bus lock. Investigation into the matter
uncovered that sometimes during reboot sequence watchdog ping is issued
late into reboot phase which didn't happen before mentioned commit
(usually watchdog ping happened immediately as commit message suggests).
As of now, when watchdog ping usually happens late into reboot stage
when interrupts are not available anymore, i2c bus lock cannot be
released anymore and pending restart handler in turn fails.

Thus, to prevent such late watchdog pings from happening ahead of
pending machine restart and consequently locking up the i2c bus, install
a reboot notifier callback in both PMIC's watchdog drivers. When reboot
notifier is called, it will raise the restart_pending flag signaling to
the watchdog ping handler to not send pings anymore.

Signed-off-by: Primoz Fiser <primoz.fiser@norik.com>
---
 drivers/watchdog/da9062_wdt.c   | 26 ++++++++++++++++++++++++++
 drivers/watchdog/da9063_wdt.c   | 22 ++++++++++++++++++++++
 include/linux/mfd/da9063/core.h |  3 +++
 3 files changed, 51 insertions(+)

diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
index 706fb09c2f24..ebb45d445d87 100644
--- a/drivers/watchdog/da9062_wdt.c
+++ b/drivers/watchdog/da9062_wdt.c
@@ -19,6 +19,7 @@
 #include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/of.h>
+#include <linux/reboot.h>
 
 static const unsigned int wdt_timeout[] = { 0, 2, 4, 8, 16, 32, 65, 131 };
 #define DA9062_TWDSCALE_DISABLE		0
@@ -33,6 +34,9 @@ struct da9062_watchdog {
 	struct da9062 *hw;
 	struct watchdog_device wdtdev;
 	bool use_sw_pm;
+
+	struct notifier_block reboot_nb;
+	bool restart_pending;
 };
 
 static unsigned int da9062_wdt_read_timeout(struct da9062_watchdog *wdt)
@@ -117,6 +121,9 @@ static int da9062_wdt_ping(struct watchdog_device *wdd)
 	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
 	int ret;
 
+	if (wdt->restart_pending)
+		return 0;
+
 	ret = da9062_reset_watchdog_timer(wdt);
 	if (ret)
 		dev_err(wdt->hw->dev, "Failed to ping the watchdog (err = %d)\n",
@@ -143,6 +150,22 @@ static int da9062_wdt_set_timeout(struct watchdog_device *wdd,
 	return ret;
 }
 
+static int da9062_wdt_reboot_notifier(struct notifier_block *nb,
+				  unsigned long code, void *unused)
+{
+	struct da9062_watchdog *wdt =
+			container_of(nb, struct da9062_watchdog, reboot_nb);
+
+	/*
+	 * Use reboot notifer to raise flag and in turn prevent watchdog pings
+	 * from occurring late in system reboot sequence and possibly locking
+	 * out restart handler from accessing i2c bus.
+	 */
+	wdt->restart_pending = true;
+
+	return NOTIFY_DONE;
+}
+
 static int da9062_wdt_restart(struct watchdog_device *wdd, unsigned long action,
 			      void *data)
 {
@@ -229,6 +252,9 @@ static int da9062_wdt_probe(struct platform_device *pdev)
 		set_bit(WDOG_HW_RUNNING, &wdt->wdtdev.status);
 	}
 
+	wdt->reboot_nb.notifier_call = da9062_wdt_reboot_notifier;
+	devm_register_reboot_notifier(dev, &wdt->reboot_nb);
+
 	return devm_watchdog_register_device(dev, &wdt->wdtdev);
 }
 
diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index 423584252606..bea2ba62dff2 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -18,6 +18,7 @@
 #include <linux/mfd/da9063/registers.h>
 #include <linux/mfd/da9063/core.h>
 #include <linux/regmap.h>
+#include <linux/reboot.h>
 
 /*
  * Watchdog selector to timeout in seconds.
@@ -121,6 +122,9 @@ static int da9063_wdt_ping(struct watchdog_device *wdd)
 	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
 	int ret;
 
+	if (da9063->restart_pending)
+		return 0;
+
 	ret = regmap_write(da9063->regmap, DA9063_REG_CONTROL_F,
 			   DA9063_WATCHDOG);
 	if (ret)
@@ -158,6 +162,21 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
 	return ret;
 }
 
+static int da9063_wdt_reboot_notifier(struct notifier_block *nb,
+				  unsigned long code, void *unused)
+{
+	struct da9063 *da9063 = container_of(nb, struct da9063, reboot_nb);
+
+	/*
+	 * Use reboot notifer to raise flag and in turn prevent watchdog pings
+	 * from occurring late in system reboot sequence and possibly locking
+	 * out restart handler from accessing i2c bus.
+	 */
+	da9063->restart_pending = true;
+
+	return NOTIFY_DONE;
+}
+
 static int da9063_wdt_restart(struct watchdog_device *wdd, unsigned long action,
 			      void *data)
 {
@@ -233,6 +252,9 @@ static int da9063_wdt_probe(struct platform_device *pdev)
 		set_bit(WDOG_HW_RUNNING, &wdd->status);
 	}
 
+	da9063->reboot_nb.notifier_call = da9063_wdt_reboot_notifier;
+	devm_register_reboot_notifier(dev, &da9063->reboot_nb);
+
 	return devm_watchdog_register_device(dev, wdd);
 }
 
diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
index fa7a43f02f27..6be39fd9300d 100644
--- a/include/linux/mfd/da9063/core.h
+++ b/include/linux/mfd/da9063/core.h
@@ -85,6 +85,9 @@ struct da9063 {
 	int		chip_irq;
 	unsigned int	irq_base;
 	struct regmap_irq_chip_data *regmap_irq;
+
+	struct notifier_block reboot_nb;
+	bool restart_pending;
 };
 
 int da9063_device_init(struct da9063 *da9063, unsigned int irq);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] watchdog: da9062: da9063: prevent watchdog pings ahead of reboot
  2021-07-06 11:21 [PATCH] watchdog: da9062: da9063: prevent watchdog pings ahead of reboot Primoz Fiser
@ 2021-07-06 13:28 ` Guenter Roeck
  2021-07-07  4:39   ` Primoz Fiser
  2021-07-08  8:21 ` [PATCH v2] watchdog: da9062: da9063: prevent pings ahead of machine reset Primoz Fiser
  1 sibling, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2021-07-06 13:28 UTC (permalink / raw)
  To: Primoz Fiser, linux-watchdog, Wim Van Sebroeck, Support Opensource
  Cc: Tero Kristo, Stefan Riedmueller, Wolfram Sang

On 7/6/21 4:21 AM, Primoz Fiser wrote:
> Proper machine resets via da9062/da9063 PMICs are very tricky as they
> require special i2c atomic transfers when interrupts are not available
> anymore. This is also a reason why both PMIC's restart handlers do not
> use regmap but instead opt for i2c_smbus_write_byte_data() which does
> i2c transfer in atomic manner. Under the hood, this function tries to
> obtain i2c bus lock with call to i2c_adapter_trylock_bus() which will
> return -EAGAIN (-11) if lock is not available.
> 
> Since commit 982bb70517aef ("watchdog: reset last_hw_keepalive time at
> start") occasional restart handler failures with "Failed to shutdown
> (err = -11)" error messages were observed, indicating that some
> process is holding the i2c bus lock. Investigation into the matter
> uncovered that sometimes during reboot sequence watchdog ping is issued
> late into reboot phase which didn't happen before mentioned commit
> (usually watchdog ping happened immediately as commit message suggests).
> As of now, when watchdog ping usually happens late into reboot stage
> when interrupts are not available anymore, i2c bus lock cannot be
> released anymore and pending restart handler in turn fails.
> 
> Thus, to prevent such late watchdog pings from happening ahead of
> pending machine restart and consequently locking up the i2c bus, install
> a reboot notifier callback in both PMIC's watchdog drivers. When reboot
> notifier is called, it will raise the restart_pending flag signaling to
> the watchdog ping handler to not send pings anymore.
> 
> Signed-off-by: Primoz Fiser <primoz.fiser@norik.com>
> ---
>   drivers/watchdog/da9062_wdt.c   | 26 ++++++++++++++++++++++++++
>   drivers/watchdog/da9063_wdt.c   | 22 ++++++++++++++++++++++
>   include/linux/mfd/da9063/core.h |  3 +++
>   3 files changed, 51 insertions(+)
> 
> diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
> index 706fb09c2f24..ebb45d445d87 100644
> --- a/drivers/watchdog/da9062_wdt.c
> +++ b/drivers/watchdog/da9062_wdt.c
> @@ -19,6 +19,7 @@
>   #include <linux/property.h>
>   #include <linux/regmap.h>
>   #include <linux/of.h>
> +#include <linux/reboot.h>
>   
>   static const unsigned int wdt_timeout[] = { 0, 2, 4, 8, 16, 32, 65, 131 };
>   #define DA9062_TWDSCALE_DISABLE		0
> @@ -33,6 +34,9 @@ struct da9062_watchdog {
>   	struct da9062 *hw;
>   	struct watchdog_device wdtdev;
>   	bool use_sw_pm;
> +
> +	struct notifier_block reboot_nb;
> +	bool restart_pending;
>   };
>   
>   static unsigned int da9062_wdt_read_timeout(struct da9062_watchdog *wdt)
> @@ -117,6 +121,9 @@ static int da9062_wdt_ping(struct watchdog_device *wdd)
>   	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
>   	int ret;
>   
> +	if (wdt->restart_pending)
> +		return 0;
> +
>   	ret = da9062_reset_watchdog_timer(wdt);
>   	if (ret)
>   		dev_err(wdt->hw->dev, "Failed to ping the watchdog (err = %d)\n",
> @@ -143,6 +150,22 @@ static int da9062_wdt_set_timeout(struct watchdog_device *wdd,
>   	return ret;
>   }
>   
> +static int da9062_wdt_reboot_notifier(struct notifier_block *nb,
> +				  unsigned long code, void *unused)
> +{
> +	struct da9062_watchdog *wdt =
> +			container_of(nb, struct da9062_watchdog, reboot_nb);
> +
> +	/*
> +	 * Use reboot notifer to raise flag and in turn prevent watchdog pings
> +	 * from occurring late in system reboot sequence and possibly locking
> +	 * out restart handler from accessing i2c bus.
> +	 */
> +	wdt->restart_pending = true;
> +
> +	return NOTIFY_DONE;
> +}
> +
>   static int da9062_wdt_restart(struct watchdog_device *wdd, unsigned long action,
>   			      void *data)
>   {
> @@ -229,6 +252,9 @@ static int da9062_wdt_probe(struct platform_device *pdev)
>   		set_bit(WDOG_HW_RUNNING, &wdt->wdtdev.status);
>   	}
>   
> +	wdt->reboot_nb.notifier_call = da9062_wdt_reboot_notifier;
> +	devm_register_reboot_notifier(dev, &wdt->reboot_nb);
> +
>   	return devm_watchdog_register_device(dev, &wdt->wdtdev);
>   }
>   
> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> index 423584252606..bea2ba62dff2 100644
> --- a/drivers/watchdog/da9063_wdt.c
> +++ b/drivers/watchdog/da9063_wdt.c
> @@ -18,6 +18,7 @@
>   #include <linux/mfd/da9063/registers.h>
>   #include <linux/mfd/da9063/core.h>
>   #include <linux/regmap.h>
> +#include <linux/reboot.h>
>   
>   /*
>    * Watchdog selector to timeout in seconds.
> @@ -121,6 +122,9 @@ static int da9063_wdt_ping(struct watchdog_device *wdd)
>   	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
>   	int ret;
>   
> +	if (da9063->restart_pending)
> +		return 0;
> +

Why all that complexity and not just check system_state here ?

Guenter

>   	ret = regmap_write(da9063->regmap, DA9063_REG_CONTROL_F,
>   			   DA9063_WATCHDOG);
>   	if (ret)
> @@ -158,6 +162,21 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
>   	return ret;
>   }
>   
> +static int da9063_wdt_reboot_notifier(struct notifier_block *nb,
> +				  unsigned long code, void *unused)
> +{
> +	struct da9063 *da9063 = container_of(nb, struct da9063, reboot_nb);
> +
> +	/*
> +	 * Use reboot notifer to raise flag and in turn prevent watchdog pings
> +	 * from occurring late in system reboot sequence and possibly locking
> +	 * out restart handler from accessing i2c bus.
> +	 */
> +	da9063->restart_pending = true;
> +
> +	return NOTIFY_DONE;
> +}
> +
>   static int da9063_wdt_restart(struct watchdog_device *wdd, unsigned long action,
>   			      void *data)
>   {
> @@ -233,6 +252,9 @@ static int da9063_wdt_probe(struct platform_device *pdev)
>   		set_bit(WDOG_HW_RUNNING, &wdd->status);
>   	}
>   
> +	da9063->reboot_nb.notifier_call = da9063_wdt_reboot_notifier;
> +	devm_register_reboot_notifier(dev, &da9063->reboot_nb);
> +
>   	return devm_watchdog_register_device(dev, wdd);
>   }
>   
> diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
> index fa7a43f02f27..6be39fd9300d 100644
> --- a/include/linux/mfd/da9063/core.h
> +++ b/include/linux/mfd/da9063/core.h
> @@ -85,6 +85,9 @@ struct da9063 {
>   	int		chip_irq;
>   	unsigned int	irq_base;
>   	struct regmap_irq_chip_data *regmap_irq;
> +
> +	struct notifier_block reboot_nb;
> +	bool restart_pending;
>   };
>   
>   int da9063_device_init(struct da9063 *da9063, unsigned int irq);
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] watchdog: da9062: da9063: prevent watchdog pings ahead of reboot
  2021-07-06 13:28 ` Guenter Roeck
@ 2021-07-07  4:39   ` Primoz Fiser
  0 siblings, 0 replies; 6+ messages in thread
From: Primoz Fiser @ 2021-07-07  4:39 UTC (permalink / raw)
  To: Guenter Roeck, linux-watchdog, Wim Van Sebroeck, Support Opensource
  Cc: Tero Kristo, Stefan Riedmueller, Wolfram Sang

Hi Guenter,

I will re-spin boards in boot loop tests to see if your solution to the 
problem also works. If this is the case, that would significantly reduce 
code complexity, indeed.

On the other hand I fear that it might be too late to check for 
(system_state > SYSTEM_RUNNING) in watchdog ping handler since if I 
remember correctly reboot notifier still runs in system_state == 
SYSTEM_RUNNING which might be just enough sooner to hit the sweet spot.

But nevertheless, thank you for your input and the idea.

Keep you informed,

BR,
Primoz


On 6. 07. 21 15:28, Guenter Roeck wrote:
> Why all that complexity and not just check system_state here ?
> 
> Guenter

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] watchdog: da9062: da9063: prevent pings ahead of machine reset
  2021-07-06 11:21 [PATCH] watchdog: da9062: da9063: prevent watchdog pings ahead of reboot Primoz Fiser
  2021-07-06 13:28 ` Guenter Roeck
@ 2021-07-08  8:21 ` Primoz Fiser
  2021-07-08 14:00   ` Guenter Roeck
  2021-07-08 14:15   ` Adam Thomson
  1 sibling, 2 replies; 6+ messages in thread
From: Primoz Fiser @ 2021-07-08  8:21 UTC (permalink / raw)
  To: linux-watchdog, Guenter Roeck, Wim Van Sebroeck, Support Opensource
  Cc: Tero Kristo, Stefan Riedmueller, Wolfram Sang

Proper machine resets via da9062/da9063 PMICs are very tricky as they
require special i2c atomic transfers when interrupts are not available
anymore. This is also a reason why both PMIC's restart handlers do not
use regmap but instead opt for i2c_smbus_write_byte_data() which does
i2c transfer in atomic manner. Under the hood, this function tries to
obtain i2c bus lock with call to i2c_adapter_trylock_bus() which will
return -EAGAIN (-11) if lock is not available.

Since commit 982bb70517aef ("watchdog: reset last_hw_keepalive time at
start") occasional restart handler failures with "Failed to shutdown
(err = -11)" error messages were observed, indicating that some
process is holding the i2c bus lock. Investigation into the matter
uncovered that sometimes during reboot sequence watchdog ping is issued
late into poweroff/reboot phase which did not happen before mentioned
commit (usually the watchdog ping happened immediately as commit message
suggests). As of now, when watchdog ping usually happens late into
poweroff/reboot stage when interrupts are not available anymore, i2c bus
lock cannot be released anymore and pending restart handler in turn
fails.

Thus, to prevent such late watchdog pings from happening ahead of
pending machine restart and consequently locking up the i2c bus, check
for system_state in watchdog ping handler and consequently do not send
pings anymore in case system_state > SYSTEM_RUNNING.

Signed-off-by: Primoz Fiser <primoz.fiser@norik.com>
---
Implemented proposal suggested by Guenter Roeck.

Re-spun boards in boot/reboot loop tests and done 5000 cycles on each
one with flying colors. 

Changes in v2:
- reduce code complexity by removing reboot notifiers and use
  system_state variable instead
- minor commit message rewording

 drivers/watchdog/da9062_wdt.c | 7 +++++++
 drivers/watchdog/da9063_wdt.c | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
index 706fb09c2f24..f02cbd530538 100644
--- a/drivers/watchdog/da9062_wdt.c
+++ b/drivers/watchdog/da9062_wdt.c
@@ -117,6 +117,13 @@ static int da9062_wdt_ping(struct watchdog_device *wdd)
 	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
 	int ret;
 
+	/*
+	 * Prevent pings from occurring late in system poweroff/reboot sequence
+	 * and possibly locking out restart handler from accessing i2c bus.
+	 */
+	if (system_state > SYSTEM_RUNNING)
+		return 0;
+
 	ret = da9062_reset_watchdog_timer(wdt);
 	if (ret)
 		dev_err(wdt->hw->dev, "Failed to ping the watchdog (err = %d)\n",
diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index 423584252606..d79ce64e26a9 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -121,6 +121,13 @@ static int da9063_wdt_ping(struct watchdog_device *wdd)
 	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
 	int ret;
 
+	/*
+	 * Prevent pings from occurring late in system poweroff/reboot sequence
+	 * and possibly locking out restart handler from accessing i2c bus.
+	 */
+	if (system_state > SYSTEM_RUNNING)
+		return 0;
+
 	ret = regmap_write(da9063->regmap, DA9063_REG_CONTROL_F,
 			   DA9063_WATCHDOG);
 	if (ret)
-- 
2.25.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] watchdog: da9062: da9063: prevent pings ahead of machine reset
  2021-07-08  8:21 ` [PATCH v2] watchdog: da9062: da9063: prevent pings ahead of machine reset Primoz Fiser
@ 2021-07-08 14:00   ` Guenter Roeck
  2021-07-08 14:15   ` Adam Thomson
  1 sibling, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2021-07-08 14:00 UTC (permalink / raw)
  To: Primoz Fiser, linux-watchdog, Wim Van Sebroeck, Support Opensource
  Cc: Tero Kristo, Stefan Riedmueller, Wolfram Sang

On 7/8/21 1:21 AM, Primoz Fiser wrote:
> Proper machine resets via da9062/da9063 PMICs are very tricky as they
> require special i2c atomic transfers when interrupts are not available
> anymore. This is also a reason why both PMIC's restart handlers do not
> use regmap but instead opt for i2c_smbus_write_byte_data() which does
> i2c transfer in atomic manner. Under the hood, this function tries to
> obtain i2c bus lock with call to i2c_adapter_trylock_bus() which will
> return -EAGAIN (-11) if lock is not available.
> 
> Since commit 982bb70517aef ("watchdog: reset last_hw_keepalive time at
> start") occasional restart handler failures with "Failed to shutdown
> (err = -11)" error messages were observed, indicating that some
> process is holding the i2c bus lock. Investigation into the matter
> uncovered that sometimes during reboot sequence watchdog ping is issued
> late into poweroff/reboot phase which did not happen before mentioned
> commit (usually the watchdog ping happened immediately as commit message
> suggests). As of now, when watchdog ping usually happens late into
> poweroff/reboot stage when interrupts are not available anymore, i2c bus
> lock cannot be released anymore and pending restart handler in turn
> fails.
> 
> Thus, to prevent such late watchdog pings from happening ahead of
> pending machine restart and consequently locking up the i2c bus, check
> for system_state in watchdog ping handler and consequently do not send
> pings anymore in case system_state > SYSTEM_RUNNING.
> 
> Signed-off-by: Primoz Fiser <primoz.fiser@norik.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> Implemented proposal suggested by Guenter Roeck.
> 
> Re-spun boards in boot/reboot loop tests and done 5000 cycles on each
> one with flying colors.
> 
> Changes in v2:
> - reduce code complexity by removing reboot notifiers and use
>    system_state variable instead
> - minor commit message rewording
> 
>   drivers/watchdog/da9062_wdt.c | 7 +++++++
>   drivers/watchdog/da9063_wdt.c | 7 +++++++
>   2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
> index 706fb09c2f24..f02cbd530538 100644
> --- a/drivers/watchdog/da9062_wdt.c
> +++ b/drivers/watchdog/da9062_wdt.c
> @@ -117,6 +117,13 @@ static int da9062_wdt_ping(struct watchdog_device *wdd)
>   	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
>   	int ret;
>   
> +	/*
> +	 * Prevent pings from occurring late in system poweroff/reboot sequence
> +	 * and possibly locking out restart handler from accessing i2c bus.
> +	 */
> +	if (system_state > SYSTEM_RUNNING)
> +		return 0;
> +
>   	ret = da9062_reset_watchdog_timer(wdt);
>   	if (ret)
>   		dev_err(wdt->hw->dev, "Failed to ping the watchdog (err = %d)\n",
> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> index 423584252606..d79ce64e26a9 100644
> --- a/drivers/watchdog/da9063_wdt.c
> +++ b/drivers/watchdog/da9063_wdt.c
> @@ -121,6 +121,13 @@ static int da9063_wdt_ping(struct watchdog_device *wdd)
>   	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
>   	int ret;
>   
> +	/*
> +	 * Prevent pings from occurring late in system poweroff/reboot sequence
> +	 * and possibly locking out restart handler from accessing i2c bus.
> +	 */
> +	if (system_state > SYSTEM_RUNNING)
> +		return 0;
> +
>   	ret = regmap_write(da9063->regmap, DA9063_REG_CONTROL_F,
>   			   DA9063_WATCHDOG);
>   	if (ret)
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v2] watchdog: da9062: da9063: prevent pings ahead of machine reset
  2021-07-08  8:21 ` [PATCH v2] watchdog: da9062: da9063: prevent pings ahead of machine reset Primoz Fiser
  2021-07-08 14:00   ` Guenter Roeck
@ 2021-07-08 14:15   ` Adam Thomson
  1 sibling, 0 replies; 6+ messages in thread
From: Adam Thomson @ 2021-07-08 14:15 UTC (permalink / raw)
  To: Primoz Fiser, linux-watchdog, Guenter Roeck, Wim Van Sebroeck,
	Support Opensource
  Cc: Tero Kristo, Stefan Riedmueller, Wolfram Sang

On 08 July 2021 09:21, Primoz Fiser wrote:

> Proper machine resets via da9062/da9063 PMICs are very tricky as they
> require special i2c atomic transfers when interrupts are not available
> anymore. This is also a reason why both PMIC's restart handlers do not
> use regmap but instead opt for i2c_smbus_write_byte_data() which does
> i2c transfer in atomic manner. Under the hood, this function tries to
> obtain i2c bus lock with call to i2c_adapter_trylock_bus() which will
> return -EAGAIN (-11) if lock is not available.
> 
> Since commit 982bb70517aef ("watchdog: reset last_hw_keepalive time at
> start") occasional restart handler failures with "Failed to shutdown
> (err = -11)" error messages were observed, indicating that some
> process is holding the i2c bus lock. Investigation into the matter
> uncovered that sometimes during reboot sequence watchdog ping is issued
> late into poweroff/reboot phase which did not happen before mentioned
> commit (usually the watchdog ping happened immediately as commit message
> suggests). As of now, when watchdog ping usually happens late into
> poweroff/reboot stage when interrupts are not available anymore, i2c bus
> lock cannot be released anymore and pending restart handler in turn
> fails.
> 
> Thus, to prevent such late watchdog pings from happening ahead of
> pending machine restart and consequently locking up the i2c bus, check
> for system_state in watchdog ping handler and consequently do not send
> pings anymore in case system_state > SYSTEM_RUNNING.
> 
> Signed-off-by: Primoz Fiser <primoz.fiser@norik.com>

Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-07-08 14:15 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-06 11:21 [PATCH] watchdog: da9062: da9063: prevent watchdog pings ahead of reboot Primoz Fiser
2021-07-06 13:28 ` Guenter Roeck
2021-07-07  4:39   ` Primoz Fiser
2021-07-08  8:21 ` [PATCH v2] watchdog: da9062: da9063: prevent pings ahead of machine reset Primoz Fiser
2021-07-08 14:00   ` Guenter Roeck
2021-07-08 14:15   ` Adam Thomson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).