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

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

> On May 9, 2022, at 9:35 PM, Tzung-Bi Shih <tzungbi@kernel.org> 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.

I was under the impression that strings were an exception to the
line-length rule.  Is that not the case?

`scripts/checkpatch.pl --strict` complains if I break the string up:

WARNING: quoted string split across lines
#162: FILE: drivers/watchdog/pseries-wdt.c:162:
+MODULE_PARM_DESC(action, "Action taken when watchdog expires: "
+		 "\"hard-poweroff\", \"hard-restart\", or \"dump-restart\" "

WARNING: quoted string split across lines
#163: FILE: drivers/watchdog/pseries-wdt.c:163:
+		 "\"hard-poweroff\", \"hard-restart\", or \"dump-restart\" "
+		 "(default=\"hard-restart\")");

>> +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?

Sure, removed.

> Does `timer_id` or some other equivalent names make more sense for the
> variable?

The hardware documentation calls the parameter "watchdogNumber", so
I think "num" is better than "id" in this case.