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

[Scott Cheloha <cheloha@linux.ibm.com>]

On Wed, Jun 01, 2022 at 08:45:03AM -0700, Guenter Roeck wrote:
> 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 ?

In my original plan they weren't kept as-is.  They were converted on
the way in and on the way out.

> > 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.

Yes, we could do that.

> 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;

I like this, we'll go with this.

> 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.

Okay, that makes sense.

> 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.

Oh, I see.  Changed in v2.