Skip to contentSkip to conversation

watchdog: pnx833x_wdt: fix typo in MODULE_PARM_DESC

PATCH In review Dan Carpenter 3 participants 4 messages Jan 8, 2016 – Mar 6, 2016

Subsystems (2)
Kernel janitors Watchdog device drivers
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 watchdog: pnx833x_wdt: fix typo in MODULE_PARM_DESC

The module_param() is "pnx833x_wdt_timeout" and MODULE_PARM_DESC() should match.

Signed-off-by Dan Carpenter <[email protected]>
drivers/watchdog/pnx833x_wdt.c +3 -2
index 882fdcb..69da25a 100644
@@ -52,8 +52,9 @@ static int pnx833x_wdt_alive;
 /* Set default timeout in MHZ.*/
 static int pnx833x_wdt_timeout = PNX_WATCHDOG_TIMEOUT;
 module_param(pnx833x_wdt_timeout, int, 0);
-MODULE_PARM_DESC(timeout, "Watchdog timeout in Mhz. (68Mhz clock), default="
-			__MODULE_STRING(PNX_TIMEOUT_VALUE) "(30 seconds).");
+MODULE_PARM_DESC(pnx833x_wdt_timeout,
+		 "Watchdog timeout in Mhz. (68Mhz clock), default="
+		 __MODULE_STRING(PNX_TIMEOUT_VALUE) "(30 seconds).");
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
maintainer
#1 lore raw
Quoted 10 lines · Dan Carpenter

The module_param() is "pnx833x_wdt_timeout" and MODULE_PARM_DESC() should match.

Signed-off-by Dan Carpenter <[email protected]>
drivers/watchdog/pnx833x_wdt.c +0 -0
index 882fdcb..69da25a 100644

drivers/watchdog/pnx833x_wdt.c static int pnx833x_wdt_alive; · line 57

Quoted 8 lines
drivers/watchdog/pnx833x_wdt.c +3 -2
@@ -52,8 +52,9 @@ static int pnx833x_wdt_alive;
  /* Set default timeout in MHZ.*/
  static int pnx833x_wdt_timeout = PNX_WATCHDOG_TIMEOUT;
  module_param(pnx833x_wdt_timeout, int, 0);
-MODULE_PARM_DESC(timeout, "Watchdog timeout in Mhz. (68Mhz clock), default="
-			__MODULE_STRING(PNX_TIMEOUT_VALUE) "(30 seconds).");
+MODULE_PARM_DESC(pnx833x_wdt_timeout,
+		 "Watchdog timeout in Mhz. (68Mhz clock), default="
+		 __MODULE_STRING(PNX_TIMEOUT_VALUE) "(30 seconds).");

The intent here was to have a module parameter named 'timeout'. Of course that was never the case. Wonder if anyone ever noticed. The correct fix would be to use something like

	module_param_named(timeout, pnx833x_wdt_timeout, int, 0);

but of course that would change the ABI. On the other side, 'timeout' is the documented module parameter in Documentation/watchdog/watchdog-parameters.txt, so one could argue that using module_param_named() would be a bug fix and not an ABI change.

Not really sure what the best approach is here. Wim, any comments ? Provide module_param_named() in _addition_ to the existing module_param(), maybe, if that is possible ?

Guenter

#2 lore raw

Hi All,

Quoted 26 lines · Dan Carpenter
Quoted 16 lines

The module_param() is "pnx833x_wdt_timeout" and MODULE_PARM_DESC() should match.

Signed-off-by Dan Carpenter <[email protected]>
drivers/watchdog/pnx833x_wdt.c +0 -1
index 882fdcb..69da25a 100644
@@ -52,8 +52,9 @@ static int pnx833x_wdt_alive;
 /* Set default timeout in MHZ.*/
 static int pnx833x_wdt_timeout = PNX_WATCHDOG_TIMEOUT;
 module_param(pnx833x_wdt_timeout, int, 0);
-MODULE_PARM_DESC(timeout, "Watchdog timeout in Mhz. (68Mhz clock),

default="

drivers/watchdog/pnx833x_wdt.c static int pnx833x_wdt_alive; · line 57

Quoted 4 lines
drivers/watchdog/pnx833x_wdt.c +3 -1
-			__MODULE_STRING(PNX_TIMEOUT_VALUE) "(30 seconds).");
+MODULE_PARM_DESC(pnx833x_wdt_timeout,
+		 "Watchdog timeout in Mhz. (68Mhz clock), default="
+		 __MODULE_STRING(PNX_TIMEOUT_VALUE) "(30 seconds).");

The intent here was to have a module parameter named 'timeout'. Of course that was never the case. Wonder if anyone ever noticed. The correct fix would be to use something like

	module_param_named(timeout, pnx833x_wdt_timeout, int, 0);
Quoted 8 lines

but of course that would change the ABI. On the other side, 'timeout' is the documented module parameter in Documentation/watchdog/watchdog-parameters.txt, so one could argue that using module_param_named() would be a bug fix and not an ABI change.

Not really sure what the best approach is here. Wim, any comments ? Provide module_param_named() in _addition_ to the existing module_param(), maybe, if that is possible ?

Good question. If timeout would have been a timeout in seconds then I would have said to go for module_param_named(). But since it is a value in Mhz, I would rather go for Dan's patch. It will break the parameters for everyone that has set in manually, but I personnaly think that people are using the default value or change it via the watchdog daemon.
But if we change this, we should also make sure that Documentation/watchdog/watchdog-parameters.txt gets corrected.

Kind regards,
Wim.

#3 lore raw
In reply to Guenter Roeck

Hi all,

Quoted 26 lines · Dan Carpenter
Quoted 16 lines

The module_param() is "pnx833x_wdt_timeout" and MODULE_PARM_DESC() should match.

Signed-off-by Dan Carpenter <[email protected]>
drivers/watchdog/pnx833x_wdt.c +0 -1
index 882fdcb..69da25a 100644
@@ -52,8 +52,9 @@ static int pnx833x_wdt_alive;
 /* Set default timeout in MHZ.*/
 static int pnx833x_wdt_timeout = PNX_WATCHDOG_TIMEOUT;
 module_param(pnx833x_wdt_timeout, int, 0);
-MODULE_PARM_DESC(timeout, "Watchdog timeout in Mhz. (68Mhz clock),

default="

drivers/watchdog/pnx833x_wdt.c static int pnx833x_wdt_alive; · line 57

Quoted 4 lines
drivers/watchdog/pnx833x_wdt.c +3 -1
-			__MODULE_STRING(PNX_TIMEOUT_VALUE) "(30 seconds).");
+MODULE_PARM_DESC(pnx833x_wdt_timeout,
+		 "Watchdog timeout in Mhz. (68Mhz clock), default="
+		 __MODULE_STRING(PNX_TIMEOUT_VALUE) "(30 seconds).");

The intent here was to have a module parameter named 'timeout'. Of course that was never the case. Wonder if anyone ever noticed. The correct fix would be to use something like

	module_param_named(timeout, pnx833x_wdt_timeout, int, 0);
Quoted 9 lines

but of course that would change the ABI. On the other side, 'timeout' is the documented module parameter in Documentation/watchdog/watchdog-parameters.txt, so one could argue that using module_param_named() would be a bug fix and not an ABI change.

Not really sure what the best approach is here. Wim, any comments ? Provide module_param_named() in _addition_ to the existing module_param(), maybe, if that is possible ?

I would keep the userspace variable the same.
So keep: MODULE_PARM_DESC(timeout, "Watchdog timeout in Mhz. (68Mhz clock), and fix the internal variables...

That will give us no userspace changes and no additional fixes.

Kind regards,
Wim.