Re: [PATCH v1 4/4] watchdog/pseries-wdt: initial support for PAPR H_WATCHDOG timers

[Guenter Roeck <linux@roeck-us.net>]

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