watchdog: imx_sc_wdt: fix pretimeout

PATCH Superseded [email protected] 3 participants 5 messages Apr 5, 2021

Subsystems (2)

Arm i.MX and Layerscape Watchdog device drivers

lore

Share & tools

lore

[email protected]

author

2021-04-05 13:01 UTC #0 lore raw

PATCH 1/1 watchdog: imx_sc_wdt: fix pretimeout

From: Stefan Eichenberger <[email protected]>

If the WDIOF_PRETIMEOUTE flag is not set when registering the device the driver will not show the sysfs entries or register the default governor. By moving the registering after the decision whether pretimeout is supported this gets fixed.

Signed-off-by Stefan Eichenberger <[email protected]>

---
 drivers/watchdog/imx_sc_wdt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/watchdog/imx_sc_wdt.c +4 -4

index e9ee22a7cb45..eddb1ae630e0 100644
@@ -183,10 +183,6 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
 	watchdog_stop_on_reboot(wdog);
 	watchdog_stop_on_unregister(wdog);
 
-	ret = devm_watchdog_register_device(dev, wdog);
-	if (ret)
-		return ret;
-
 	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
 				       SC_IRQ_WDOG,
 				       true);

@@ -213,6 +209,10 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
 	else
 		dev_warn(dev, "Add action failed, pretimeout NOT supported\n");
 
+	ret = devm_watchdog_register_device(dev, wdog);
+	if (ret)
+		return ret;
+
 	return 0;
 }

Signature · 1 line

2.27.0

Ahmad Fatoum

2021-04-05 13:23 UTC #1 lore raw

Quoted 28 lines · On 05.04.21 14:59, [email protected] wrote:

From: Stefan Eichenberger <[email protected]>
If the WDIOF_PRETIMEOUTE flag is not set when registering the device the driver will not show the sysfs entries or register the default governor. By moving the registering after the decision whether pretimeout is supported this gets fixed.
Signed-off-by Stefan Eichenberger <[email protected]>
---
 drivers/watchdog/imx_sc_wdt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/watchdog/imx_sc_wdt.c +0 -4
index e9ee22a7cb45..eddb1ae630e0 100644
@@ -183,10 +183,6 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
 	watchdog_stop_on_reboot(wdog);
 	watchdog_stop_on_unregister(wdog);
 
-	ret = devm_watchdog_register_device(dev, wdog);
-	if (ret)
-		return ret;
-
 	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
 				       SC_IRQ_WDOG,
 				       true);

drivers/watchdog/imx_sc_wdt.c static int imx_sc_wdt_probe(struct platform_device *pdev) · line 214

Quoted 6 lines

drivers/watchdog/imx_sc_wdt.c +3 -0

@@ -213,6 +209,10 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
 	else
 		dev_warn(dev, "Add action failed, pretimeout NOT supported\n");
 
+	ret = devm_watchdog_register_device(dev, wdog);
+	if (ret)
+		return ret;

This could be rewritten as return devm_watchdog_register_device(dev, wdog);

Quoted 5 lines

drivers/watchdog/imx_sc_wdt.c +1 -0
+
 	return 0;
 }
 

Signature · 4 lines

Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Guenter Roeck

maintainer

2021-04-05 14:42 UTC #2 lore raw

In reply to [email protected]

Quoted 3 lines · On 4/5/21 5:59 AM, [email protected] wrote:

From: Stefan Eichenberger <[email protected]>
If the WDIOF_PRETIMEOUTE flag is not set when registering the device the

WDIOF_PRETIMEOUT

Quoted 4 lines

driver will not show the sysfs entries or register the default governor. By moving the registering after the decision whether pretimeout is supported this gets fixed.

This is problematic. If the notifier is called and the watchdog is not yet registered, it will call watchdog_notify_pretimeout on an unregistered watchdog device which would crash the system. The notifier function needs to handle that situation. This is for both registration and removal: On removal, the watchdog device would be unregistered first (because of the use of devm_ functions) and, again, the notifier could be called on an unregistered watchdog device.

Quoted 21 lines

Signed-off-by Stefan Eichenberger <[email protected]>

---
 drivers/watchdog/imx_sc_wdt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/watchdog/imx_sc_wdt.c +0 -4

index e9ee22a7cb45..eddb1ae630e0 100644
@@ -183,10 +183,6 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
 	watchdog_stop_on_reboot(wdog);
 	watchdog_stop_on_unregister(wdog);
 
-	ret = devm_watchdog_register_device(dev, wdog);
-	if (ret)
-		return ret;
-
 	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
 				       SC_IRQ_WDOG,
 				       true);

drivers/watchdog/imx_sc_wdt.c static int imx_sc_wdt_probe(struct platform_device *pdev) · line 216

Quoted 8 lines

drivers/watchdog/imx_sc_wdt.c +4 -0

@@ -213,6 +209,10 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
 	else
 		dev_warn(dev, "Add action failed, pretimeout NOT supported\n");
 
+	ret = devm_watchdog_register_device(dev, wdog);
+	if (ret)
+		return ret;
+
 	return 0;

This is equivalent to just "return ret;".

Quoted 3 lines

drivers/watchdog/imx_sc_wdt.c +0 -0
 }
 

Stefan Eichenberger

author

2021-04-05 17:43 UTC #3 lore raw

Hi Guenter,

Quoted 7 lines · Guenter Roeck

Quoted 3 lines · On 4/5/21 5:59 AM, [email protected] wrote:
From: Stefan Eichenberger <[email protected]>
If the WDIOF_PRETIMEOUTE flag is not set when registering the device the
WDIOF_PRETIMEOUT

Thanks for that finding.

Quoted 5 lines

Quoted 4 lines
driver will not show the sysfs entries or register the default governor. By moving the registering after the decision whether pretimeout is supported this gets fixed.

Quoted 8 lines

This is problematic. If the notifier is called and the watchdog is not yet registered, it will call watchdog_notify_pretimeout on an unregistered watchdog device which would crash the system. The notifier function needs to handle that situation. This is for both registration and removal: On removal, the watchdog device would be unregistered first (because of the use of devm_ functions) and, again, the notifier could be called on an unregistered watchdog device.

Isn't that handled in watchdog_notify_pretimeout with if (!wdd->gov)? As I understand the code, a governor is registered earliest in watchdog_register_pretimeout, which is called by watchdog_dev_register. The same when doing an unregister, which should be called when doing a watchdog_dev_unregister.

Quoted 24 lines

Signed-off-by Stefan Eichenberger <[email protected]>

---
 drivers/watchdog/imx_sc_wdt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/watchdog/imx_sc_wdt.c +0 -4

index e9ee22a7cb45..eddb1ae630e0 100644
@@ -183,10 +183,6 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
 	watchdog_stop_on_reboot(wdog);
 	watchdog_stop_on_unregister(wdog);
 
-	ret = devm_watchdog_register_device(dev, wdog);
-	if (ret)
-		return ret;
-
 	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
 				       SC_IRQ_WDOG,
 				       true);

@@ -213,6 +209,10 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
 	else
 		dev_warn(dev, "Add action failed, pretimeout NOT supported\n");

Quoted 8 lines

drivers/watchdog/imx_sc_wdt.c static int imx_sc_wdt_probe(struct platform_device *pdev) · line 216
Quoted 5 lines
drivers/watchdog/imx_sc_wdt.c +4 -0
+	ret = devm_watchdog_register_device(dev, wdog);
+	if (ret)
+		return ret;
+
 	return 0;
This is equivalent to just "return ret;".

Thanks, will fix that too.

Regards,
Stefan

Guenter Roeck

maintainer

2021-04-05 18:02 UTC #4 lore raw

Quoted 27 lines · Stefan Eichenberger

Hi Guenter,
Quoted 7 lines · Guenter Roeck
Quoted 3 lines · On 4/5/21 5:59 AM, [email protected] wrote:
From: Stefan Eichenberger <[email protected]>
If the WDIOF_PRETIMEOUTE flag is not set when registering the device the
WDIOF_PRETIMEOUT
Thanks for that finding.
Quoted 5 lines
Quoted 4 lines
driver will not show the sysfs entries or register the default governor. By moving the registering after the decision whether pretimeout is supported this gets fixed.
Quoted 8 lines
This is problematic. If the notifier is called and the watchdog is not yet registered, it will call watchdog_notify_pretimeout on an unregistered watchdog device which would crash the system. The notifier function needs to handle that situation. This is for both registration and removal: On removal, the watchdog device would be unregistered first (because of the use of devm_ functions) and, again, the notifier could be called on an unregistered watchdog device.

Quoted 6 lines

Isn't that handled in watchdog_notify_pretimeout with if (!wdd->gov)? As I understand the code, a governor is registered earliest in watchdog_register_pretimeout, which is called by watchdog_dev_register. The same when doing an unregister, which should be called when doing a watchdog_dev_unregister.

Ah yes, you are correct. Thanks for checking, and sorry for the noise.

Guenter

Quoted 37 lines

Quoted 24 lines

Signed-off-by Stefan Eichenberger <[email protected]>

---
 drivers/watchdog/imx_sc_wdt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/watchdog/imx_sc_wdt.c +0 -4

index e9ee22a7cb45..eddb1ae630e0 100644
@@ -183,10 +183,6 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
 	watchdog_stop_on_reboot(wdog);
 	watchdog_stop_on_unregister(wdog);
 
-	ret = devm_watchdog_register_device(dev, wdog);
-	if (ret)
-		return ret;
-
 	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
 				       SC_IRQ_WDOG,
 				       true);

@@ -213,6 +209,10 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
 	else
 		dev_warn(dev, "Add action failed, pretimeout NOT supported\n");

Quoted 8 lines

drivers/watchdog/imx_sc_wdt.c static int imx_sc_wdt_probe(struct platform_device *pdev) · line 216
Quoted 5 lines
drivers/watchdog/imx_sc_wdt.c +4 -0
+	ret = devm_watchdog_register_device(dev, wdog);
+	if (ret)
+		return ret;
+
 	return 0;
This is equivalent to just "return ret;".

Thanks, will fix that too.

Regards,
Stefan