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

[Guenter Roeck <linux@roeck-us.net>]

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.

Thanks,
Guenter