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

[Tzung-Bi Shih <tzungbi@kernel.org>]

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?

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?

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