On 6/1/22 08:07, Scott Cheloha wrote:
[ ... ]
+static unsigned long action = PSERIES_WDTF_ACTION_HARD_RESTART;
+
+static int action_get(char *buf, const struct kernel_param *kp)
+{
+ int val;
+
+ switch (action) {
+ case PSERIES_WDTF_ACTION_HARD_POWEROFF:
+ val = 1;
+ break;
+ case PSERIES_WDTF_ACTION_HARD_RESTART:
+ val = 2;
+ break;
+ case PSERIES_WDTF_ACTION_DUMP_RESTART:
+ val = 3;
+ break;
+ default:
+ return -EINVAL;
+ }
+ return sprintf(buf, "%d\n", val);
+}
+
+static int action_set(const char *val, const struct kernel_param *kp)
+{
+ int choice;
+
+ if (kstrtoint(val, 10, &choice))
+ return -EINVAL;
+ switch (choice) {
+ case 1:
+ action = PSERIES_WDTF_ACTION_HARD_POWEROFF;
+ return 0;
+ case 2:
+ action = PSERIES_WDTF_ACTION_HARD_RESTART;
+ return 0;
+ case 3:
+ action = PSERIES_WDTF_ACTION_DUMP_RESTART;
+ return 0;
+ }
+ return -EINVAL;
+}
+
+static const struct kernel_param_ops action_ops = {
+ .get = action_get,
+ .set = action_set,
+};
+module_param_cb(action, &action_ops, NULL, 0444);
0644 here and below?
That would make the module parameters have to be runtime
configurable, which does not make sense at least for
the two parameters below.
Agreed.
I don't know though if it is really valuable to have all the
above code instead of just
storing the action numbers and doing the conversion to action
once in the probe function. The above code really only
makes sense if the action is changeable during runtime and more
is done that just converting it to another value.
Having a setter that runs exactly once during module attach is
obvious. We need a corresponding .get() method to convert on the way
out anyway.
Why would a get method be needed if the module parameter is just kept as-is ?
I don't see any upside to moving the action_set() code into
pseries_wdt_probe() aside from maybe shaving a few SLOC. The module
is already very compact.
I disagree. The get method is unnecessary. The module parameter values (1..3)
add unnecessary complexity. It could as well be 0..2, making it easier to convert.
The actual action could be stored in struct pseries_wdt, or converted using something
like
u8 pseries_actions[] = {
PSERIES_WDTF_ACTION_HARD_POWEROFF,
PSERIES_WDTF_ACTION_HARD_RESTART,
PSERIES_WDTF_ACTION_DUMP_RESTART
};
flags = pseries_actions[action] | PSERIES_WDTF_OP_START;
or, alternatively, in probe
if (action > 2)
return -EINVAL;
pw->action = pseries_actions[action];
and, in the start function,
flags = pw->action | PSERIES_WDTF_OP_START;
That not only reduces code size but also improves readability.
get/set methods are useful, but should be limited to cases where they
are really needed, ie do something besides converting values. You could argue
that you want to be able to change the reboot method on the fly, by making
the module parameter writeable, but then the .set method should actually
change the method accordingly and not just convert values. And even then
I'd argue that it would be better not to convert the 'action' value itself
but to keep it at 0, 1, 2 (or 1, 2, 3 if you prefer) and use param_get_uint
(or param_get_ulong) for the get method.
Regarding max_timeout, we have calculations such as
unsigned int t = wdd->timeout * 1000;
in the assumption that timeouts larger than UINT_MAX/1000 seconds (or ~50 days)
don't really make much sense. watchdog_timeout_invalid() will also return -EINVAL
if the provided timeout value is larger than UINT_MAX / 1000.
Guenter
