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

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

On Thu, Apr 14, 2022 at 11:20:08AM +0800, Tzung-Bi Shih wrote:
> On Wed, Apr 13, 2022 at 11:51:04AM -0500, Scott Cheloha wrote:
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index c4e82a8d863f..f3e6db5bed74 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -1941,6 +1941,14 @@ config WATCHDOG_RTAS
> >  	  To compile this driver as a module, choose M here. The module
> >  	  will be called wdrtas.
> >  
> > +config PSERIES_WDT
> > +	tristate "POWER Architecture Platform Watchdog Timer"
> > +	depends on PPC_PSERIES
> > +	select WATCHDOG_CORE
> > +	help
> > +	  Driver for virtual watchdog timers provided by PAPR
> > +	  hypervisors (e.g. pHyp, KVM).
> > +
> 
> To maintain alphabetical order, the option should be prior to WATCHDOG_RTAS.
> 
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index f7da867e8782..3ae1f7d9f1ec 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -185,6 +185,7 @@ obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o
> >  
> >  # PPC64 Architecture
> >  obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o
> > +obj-$(CONFIG_PSERIES_WDT) += pseries-wdt.o
> 
> Same here.
> 
> > diff --git a/drivers/watchdog/pseries-wdt.c b/drivers/watchdog/pseries-wdt.c
> > new file mode 100644
> > index 000000000000..0d22ddf12a7f
> > --- /dev/null
> > +++ b/drivers/watchdog/pseries-wdt.c
> > @@ -0,0 +1,337 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#define DRV_NAME "pseries-wdt"
> > +#define pr_fmt(fmt) DRV_NAME ": " fmt
> 
> `pr_fmt` is unused.
> 

That is just a neat trick to get subsequent pr_xxx to print the driver name
as prefix.

> 
> > +static int __init pseries_wdt_init_module(void)
> > +{
> > +	unsigned long ret[PLPAR_HCALL_BUFSIZE] = { 0 };
> > +	unsigned long cap;
> > +	long rc;
> > +	int err;
> > +
> > +	rc = plpar_hcall(H_WATCHDOG, ret, PSERIES_WDTF_OP_QUERY);
> > +	if (rc != H_SUCCESS) {
> > +		if (rc == H_FUNCTION) {
> > +			pr_info("hypervisor does not support H_WATCHDOG");
> > +			return -ENODEV;
> > +		}
> > +		pr_err("H_WATCHDOG: %ld: capability query failed", rc);
> > +		return -EIO;

The init function should not print any messages. Loading the driver on
a platform not supporting it should have no effect and print no messages.

> > +	}
> > +	cap = ret[0];
> > +	min_timeout = roundup(PSERIES_WDTQ_MIN_TIMEOUT(cap), 1000) / 1000;
> > +	pr_info("hypervisor supports %lu timer(s), %lums minimum timeout",
> > +		PSERIES_WDTQ_MAX_NUMBER(cap), PSERIES_WDTQ_MIN_TIMEOUT(cap));
> 
> Could these bits be in pseries_wdt_probe()?

Yes. Also, if values have to be passed to the driver, the init code should
pass it as platform data. There should be no static variables to pass
values to the probe function.

Thanks,
Guenter