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.
