Re: [RFC v2 2/2] watchdog: pseries-wdt: initial support for PAPR virtual watchdog timers

[Alexey Kardashevskiy <aik@xxxxxxxxx>]

On 5/10/22 13:34, Guenter Roeck wrote:
> On 5/9/22 19:35, Tzung-Bi Shih wrote:
> > On Mon, May 09, 2022 at 12:43:57PM -0500, Scott Cheloha wrote:
> > > +#define SETFIELD(_v, _b, _e)    \
> > > +    (((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b), 
> > > (_e)))
> > > +#define GETFIELD(_v, _b, _e)    \
> > > +    (((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >> 
> > > PPC_BITLSHIFT(_e))
> >
> > > From `./scripts/checkpatch.pl --strict`:
> > WARNING: please, no spaces at the start of a line
> >
> > > +#define PSERIES_WDTQL_MUST_STOP           1
> >
> > > From `./scripts/checkpatch.pl --strict`:
> > WARNING: please, no space before tabs
> >
> > > +static const struct kernel_param_ops action_ops = { .set = 
> > > action_set };
> > > +module_param_cb(action, &action_ops, NULL, S_IRUGO);
> >
> > > From `./scripts/checkpatch.pl --strict`:
> > WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using
> > octal permissions '0444'.
> >
> > > +MODULE_PARM_DESC(action, "Action taken when watchdog expires: 
> > > \"hard-poweroff\", \"hard-restart\", or \"dump-restart\" 
> > > (default=\"hard-restart\")");
> >
> > The line exceeds 100 columns.
> >
> > > +static bool nowayout = WATCHDOG_NOWAYOUT;
> > > +module_param(nowayout, bool, S_IRUGO);
> >
> > > From `./scripts/checkpatch.pl --strict`:
> > WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using
> > octal permissions '0444'.
> >
> > > +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started 
> > > (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> >
> > > From `./scripts/checkpatch.pl --strict`, the line exceeds 100 columns.
> >
> > > +#define WATCHDOG_TIMEOUT 60
> > > +static unsigned int timeout = WATCHDOG_TIMEOUT;
> > > +module_param(timeout, uint, S_IRUGO);
> >
> > > From `./scripts/checkpatch.pl --strict`:
> > WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using
> > octal permissions '0444'.
> >
> > > +MODULE_PARM_DESC(timeout, "Initial watchdog timeout in seconds 
> > > (default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
> >
> > > From `./scripts/checkpatch.pl --strict`, the line exceeds 100 columns.
> >
> > > +struct pseries_wdt {
> > > +    struct watchdog_device wd;
> > > +    unsigned long num;        /* NB: Watchdog numbers are 1-based */
> >
> > What does NB stand for?  Could it be removed from the comment?
>
> Latin "Nota Bene", for "This is important".
> All comments should be important, so I agree, this has little
> if any value.
>
> > Does `timer_id` or some other equivalent names make more sense for the
> > variable?
> >
> > > +static int pseries_wdt_start(struct watchdog_device *wdd)
> > > +{
> > [...]
> > > +    rc = plpar_hcall_norets(H_WATCHDOG, flags, pw->num, msecs);
> > > +    if (rc != H_SUCCESS) {
> > > +        dev_crit(dev, "H_WATCHDOG: %ld: failed to start timer %lu",
> > > +             rc, pw->num);
> > > +               return -EIO;
> >
> > > From `./scripts/checkpatch.pl --strict`:
> > ERROR: code indent should use tabs where possible
> > WARNING: please, no space before tabs
> >
> > > +static struct watchdog_info pseries_wdt_info = {
> > > +    .identity = DRV_NAME,
> > > +    .options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | 
> > > WDIOF_SETTIMEOUT \
> > > +        | WDIOF_PRETIMEOUT,
> >
> > > From `./scripts/checkpatch.pl --strict`:
> > WARNING: Avoid unnecessary line continuations
> >
> > > +static const struct watchdog_ops pseries_wdt_ops = {
> > > +    .owner = THIS_MODULE,
> > > +    .ping = pseries_wdt_start,
> >
> > Does this mean: it needs hard restart for every ping?
>
> If there is no separate ping function, there is no need to point
> the ping function to the start function. The watchdog core uses
> the start function automatically in this case.
>
> > > +static int pseries_wdt_probe(struct platform_device *pdev)
> > > +{
> > [...]
> > > +    rc = plpar_hcall(H_WATCHDOG, ret, PSERIES_WDTF_OP_QUERY);
> > > +    if (rc != H_SUCCESS)
> > > +        return (rc == H_FUNCTION) ? -ENODEV : -EIO;
> >
> > The parentheses can be dropped.
> >
> > > +    pw = devm_kzalloc(&pdev->dev, sizeof *pw, GFP_KERNEL);
> > > +    if (pw == NULL)
> >
> > > From `./scripts/checkpatch.pl --strict`:
> > CHECK: Comparison to NULL could be written "!pw"
> >
> > > +    pw->num = pdev->id + 1;        /* 0-based -> 1-based */
> >
> > Didn't see where the platform device was registered but using the 
> > pdev->id as
> > the timer id could be unreliable (e.g. from auto increment).
>
> That is at the beginning of the patch, in 
> arch/powerpc/platforms/pseries/setup.c,
> which calls platform_device_register_simple() with an explicit device ID.
>
> I agree, though, that it is fragile: The code in the probe function
> explicitly checks for a negative ID, which would only be provided if
> platform_device_register() explicitly declares that (ie if it provides
> PLATFORM_DEVID_NONE as device ID). But if that is a concern, there
> might as well be code registering a platform device with
> PLATFORM_DEVID_AUTO as device ID, and then th device id would be automatic
> and more or less random. I think the instantiation code should be more
> explicit: Either assume that PLATFORM_DEVID_NONE or PLATFORM_DEVID_AUTO
> will never be used to register the watchdog devices, or provide other
> information such as platform data to make it explicit.
>
> Also, the changes in arch code need to be made in separate patches.


Often this is true but in case like this - the driver itself is dead 
code until the platform enables it and if there is a problem with the 
driver - bisect will point to the device enablement commit. Backporting 
becomes a problem as it is going to be 3 apart patches vs. one:
1) define hypercall
2) implement the driver
3) enable the driver + device FW feature flag.

Instead, either maintainer (powerpc or watchdog) says "ack" and the 
other one puts it in the tree, I saw this in practice. Thanks,



> Thanks,
> Guenter

--
Alexey