* [RFC v2 0/2] Add driver for PAPR watchdog timers
@ 2022-05-09 17:43 Scott Cheloha
2022-05-09 17:43 ` [RFC v2 1/2] powerpc/pseries: hvcall.h: add H_WATCHDOG opcode, H_NOOP return code Scott Cheloha
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Scott Cheloha @ 2022-05-09 17:43 UTC (permalink / raw)
To: linux-watchdog
Cc: linux, tzungbi, brking, nathanl, aik, npiggin, vaishnavi, wvoigt
This second RFC incorporates feedback from the previous RFC:
https://lore.kernel.org/linux-watchdog/20220413165104.179144-1-cheloha@linux.ibm.com/
v2 changes of note:
- Add a firmware feature flag for the H_WATCHDOG feature,
FW_FEATURE_WATCHDOG.
- Register a platform_device for the first watchdog timer during a
pseries initcall if we have FW_FEATURE_WATCHDOG. Use id zero, as
there could be more than one timer in the future.
- Alphabetize Makefile changes.
- Add missing copyright information to pseries-wdt.c.
- Add an 'action' module parameter that configures how the guest is
terminated on watchdog expiration.
- Use dev_*() for logging critical errors instead of pr_*().
- Handle the H_NOOP case when trying to stop the watchdog. If the
given watchdog is not actually running, H_WATCHDOG returns H_NOOP.
This is harmless, so we should treat it as a success.
- We don't need pseries_wdt_remove() at all.
- Check watchdog_active() before stopping/starting the timer across
suspend/resume.
- Consolidate all code from pseries_wdt_module_init() into
pseries_wdt_probe(). We can then use module_platform_driver().
I have one lingering question:
- The pseries-wdt module is not "automatically" loaded during boot.
When I do
# modprobe pseries-wdt
the driver attaches to the platform bus as expected and the
/dev/watchdog* devices for the pseries-wdt.0 platform device
are created.
I was under the impression that driver/device matching for
the platform bus was simple string comparison.
... what am I doing wrong? Is this expected behavior? Do
I need to do additional configuration to get the module to
load automatically at boot time?
^ permalink raw reply [flat|nested] 12+ messages in thread* [RFC v2 1/2] powerpc/pseries: hvcall.h: add H_WATCHDOG opcode, H_NOOP return code 2022-05-09 17:43 [RFC v2 0/2] Add driver for PAPR watchdog timers Scott Cheloha @ 2022-05-09 17:43 ` Scott Cheloha 2022-05-09 17:43 ` [RFC v2 2/2] watchdog: pseries-wdt: initial support for PAPR virtual watchdog timers Scott Cheloha 2022-05-11 5:38 ` [RFC v2 0/2] Add driver for PAPR " Alexey Kardashevskiy 2 siblings, 0 replies; 12+ messages in thread From: Scott Cheloha @ 2022-05-09 17:43 UTC (permalink / raw) To: linux-watchdog Cc: linux, tzungbi, brking, nathanl, aik, npiggin, vaishnavi, wvoigt, Scott Cheloha PAPR v2.12 defines a new hypercall, H_WATCHDOG. The hypercall permits guest control of one or more virtual watchdog timers. This patch adds the opcode for the H_WATCHDOG hypercall. It also defines H_NOOP, a possible return code for H_WATCHDOG. Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com> --- arch/powerpc/include/asm/hvcall.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h index d92a20a85395..4b4f69c35b4f 100644 --- a/arch/powerpc/include/asm/hvcall.h +++ b/arch/powerpc/include/asm/hvcall.h @@ -87,6 +87,7 @@ #define H_P7 -60 #define H_P8 -61 #define H_P9 -62 +#define H_NOOP -63 #define H_TOO_BIG -64 #define H_UNSUPPORTED -67 #define H_OVERLAP -68 @@ -324,7 +325,8 @@ #define H_RPT_INVALIDATE 0x448 #define H_SCM_FLUSH 0x44C #define H_GET_ENERGY_SCALE_INFO 0x450 -#define MAX_HCALL_OPCODE H_GET_ENERGY_SCALE_INFO +#define H_WATCHDOG 0x45C +#define MAX_HCALL_OPCODE H_WATCHDOG /* Scope args for H_SCM_UNBIND_ALL */ #define H_UNBIND_SCOPE_ALL (0x1) -- 2.27.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC v2 2/2] watchdog: pseries-wdt: initial support for PAPR virtual watchdog timers 2022-05-09 17:43 [RFC v2 0/2] Add driver for PAPR watchdog timers Scott Cheloha 2022-05-09 17:43 ` [RFC v2 1/2] powerpc/pseries: hvcall.h: add H_WATCHDOG opcode, H_NOOP return code Scott Cheloha @ 2022-05-09 17:43 ` Scott Cheloha 2022-05-10 2:35 ` Tzung-Bi Shih 2022-05-11 5:38 ` [RFC v2 0/2] Add driver for PAPR " Alexey Kardashevskiy 2 siblings, 1 reply; 12+ messages in thread From: Scott Cheloha @ 2022-05-09 17:43 UTC (permalink / raw) To: linux-watchdog Cc: linux, tzungbi, brking, nathanl, aik, npiggin, vaishnavi, wvoigt, Scott Cheloha PAPR v2.12 defines a new hypercall, H_WATCHDOG. It permits guest control of one or more virtual watchdog timers. The timers have millisecond granularity and terminate the guest upon expiration. This patch adds a new driver for these timers, "pseries-wdt". Unfortunately, these timers defy PowerPC device conventions. They are not affixed to any bus, nor do they have full representation in the device tree. As a workaround, we represent them as platform devices. A platform device corresponding to a given timer is registered with the platform bus via a machine_subsys_initcall() if firmware support for the H_WATCHDOG hypercall is detected early in boot. For now, only a single timer is registered with the platform bus and exposed to userspace. In the future we could expose additional timers if there is good reason to do so. Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com> --- arch/powerpc/include/asm/firmware.h | 3 +- arch/powerpc/platforms/pseries/firmware.c | 1 + arch/powerpc/platforms/pseries/setup.c | 17 ++ drivers/watchdog/Kconfig | 8 + drivers/watchdog/Makefile | 1 + drivers/watchdog/pseries-wdt.c | 300 ++++++++++++++++++++++ 6 files changed, 329 insertions(+), 1 deletion(-) create mode 100644 drivers/watchdog/pseries-wdt.c diff --git a/arch/powerpc/include/asm/firmware.h b/arch/powerpc/include/asm/firmware.h index 8dddd34b8ecf..398e0b5e485f 100644 --- a/arch/powerpc/include/asm/firmware.h +++ b/arch/powerpc/include/asm/firmware.h @@ -55,6 +55,7 @@ #define FW_FEATURE_RPT_INVALIDATE ASM_CONST(0x0000010000000000) #define FW_FEATURE_FORM2_AFFINITY ASM_CONST(0x0000020000000000) #define FW_FEATURE_ENERGY_SCALE_INFO ASM_CONST(0x0000040000000000) +#define FW_FEATURE_WATCHDOG ASM_CONST(0x0000080000000000) #ifndef __ASSEMBLY__ @@ -76,7 +77,7 @@ enum { FW_FEATURE_DRC_INFO | FW_FEATURE_BLOCK_REMOVE | FW_FEATURE_PAPR_SCM | FW_FEATURE_ULTRAVISOR | FW_FEATURE_RPT_INVALIDATE | FW_FEATURE_FORM2_AFFINITY | - FW_FEATURE_ENERGY_SCALE_INFO, + FW_FEATURE_ENERGY_SCALE_INFO | FW_FEATURE_WATCHDOG, FW_FEATURE_PSERIES_ALWAYS = 0, FW_FEATURE_POWERNV_POSSIBLE = FW_FEATURE_OPAL | FW_FEATURE_ULTRAVISOR, FW_FEATURE_POWERNV_ALWAYS = 0, diff --git a/arch/powerpc/platforms/pseries/firmware.c b/arch/powerpc/platforms/pseries/firmware.c index 09c119b2f623..080108d129ed 100644 --- a/arch/powerpc/platforms/pseries/firmware.c +++ b/arch/powerpc/platforms/pseries/firmware.c @@ -67,6 +67,7 @@ hypertas_fw_features_table[] = { {FW_FEATURE_PAPR_SCM, "hcall-scm"}, {FW_FEATURE_RPT_INVALIDATE, "hcall-rpt-invalidate"}, {FW_FEATURE_ENERGY_SCALE_INFO, "hcall-energy-scale-info"}, + {FW_FEATURE_WATCHDOG, "hcall-watchdog"}, }; /* Build up the firmware features bitmask using the contents of diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c index 955ff8aa1644..6e8b67ea2a33 100644 --- a/arch/powerpc/platforms/pseries/setup.c +++ b/arch/powerpc/platforms/pseries/setup.c @@ -14,6 +14,7 @@ #include <linux/cpu.h> #include <linux/errno.h> +#include <linux/platform_device.h> #include <linux/sched.h> #include <linux/kernel.h> #include <linux/mm.h> @@ -169,6 +170,22 @@ static void __init fwnmi_init(void) #endif } +/* + * Affix a device for the first timer to the platform bus if + * we have firmware support for the H_WATCHDOG hypercall. + */ +static struct platform_device *pseries_wdt_pdev; + +static __init int pseries_wdt_init(void) +{ + if (!firmware_has_feature(FW_FEATURE_WATCHDOG)) + return 0; + pseries_wdt_pdev = platform_device_register_simple("pseries-wdt", + 0, NULL, 0); + return 0; +} +machine_subsys_initcall(pseries, pseries_wdt_init); + static void pseries_8259_cascade(struct irq_desc *desc) { struct irq_chip *chip = irq_desc_get_chip(desc); diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index c4e82a8d863f..c5755f1e4540 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -1932,6 +1932,14 @@ config MEN_A21_WDT # PPC64 Architecture +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). + config WATCHDOG_RTAS tristate "RTAS watchdog" depends on PPC_RTAS diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index f7da867e8782..f35660409f17 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -184,6 +184,7 @@ obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o # PPC64 Architecture +obj-$(CONFIG_PSERIES_WDT) += pseries-wdt.o obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o # S390 Architecture diff --git a/drivers/watchdog/pseries-wdt.c b/drivers/watchdog/pseries-wdt.c new file mode 100644 index 000000000000..501211bd3f26 --- /dev/null +++ b/drivers/watchdog/pseries-wdt.c @@ -0,0 +1,300 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +/* + * Copyright (c) 2022 International Business Machines, Inc. + */ + +#include <linux/bitops.h> +#include <linux/kernel.h> +#include <linux/limits.h> +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/platform_device.h> +#include <linux/watchdog.h> + +#define DRV_NAME "pseries-wdt" + +/* + * The PAPR's MSB->LSB bit ordering is is 0->63. These macros + * simplify defining bitfields as described in the PAPR without + * needing to transpose values to the more C-like 63->0 ordering. + */ +#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)) + +/* + * H_WATCHDOG Hypercall Input + * + * R4: "flags": + * + * A 64-bit value structured as follows: + * + * Bits 0-46: Reserved (must be zero). + */ +#define PSERIES_WDTF_RESERVED PPC_BITMASK(0, 46) + +/* + * Bit 47: "leaveOtherWatchdogsRunningOnTimeout" + * + * 0 Stop outstanding watchdogs on timeout. + * 1 Leave outstanding watchdogs running on timeout. + */ +#define PSERIES_WDTF_LEAVE_OTHER PPC_BIT(47) + +/* + * Bits 48-55: "operation" + * + * 0x01 Start Watchdog + * 0x02 Stop Watchdog + * 0x03 Query Watchdog Capabilities + * 0x04 Query Watchdog LPM Requirement + */ +#define PSERIES_WDTF_OP(op) SETFIELD((op), 48, 55) +#define PSERIES_WDTF_OP_START PSERIES_WDTF_OP(0x1) +#define PSERIES_WDTF_OP_STOP PSERIES_WDTF_OP(0x2) +#define PSERIES_WDTF_OP_QUERY PSERIES_WDTF_OP(0x3) +#define PSERIES_WDTF_OP_QUERY_LPM PSERIES_WDTF_OP(0x4) + +/* + * Bits 56-63: "timeoutAction" + * + * 0x01 Hard poweroff + * 0x02 Hard restart + * 0x03 Dump restart + */ +#define PSERIES_WDTF_ACTION(ac) SETFIELD(ac, 56, 63) +#define PSERIES_WDTF_ACTION_HARD_POWEROFF PSERIES_WDTF_ACTION(0x1) +#define PSERIES_WDTF_ACTION_HARD_RESTART PSERIES_WDTF_ACTION(0x2) +#define PSERIES_WDTF_ACTION_DUMP_RESTART PSERIES_WDTF_ACTION(0x3) + +/* + * R5: "watchdogNumber": + * + * The target watchdog. Watchdog numbers are 1-based. The + * maximum supported watchdog number may be obtained via the + * "Query Watchdog Capabilities" operation. + * + * This input is ignored for the "Query Watchdog Capabilities" + * operation. + * + * R6: "timeoutInMs": + * + * The timeout in milliseconds. The minimum supported timeout may + * be obtained via the "Query Watchdog Capabilities" operation. + * + * This input is ignored for the "Stop Watchdog", "Query Watchdog + * Capabilities", and "Query LPM Requirement" operations. + */ + +/* + * H_WATCHDOG Hypercall Output + * + * R3: Return code + * + * H_SUCCESS The operation completed. + * + * H_BUSY The hypervisor is too busy; retry the operation. + * + * H_PARAMETER The given "flags" are somehow invalid. Either the + * "operation" or "timeoutAction" is invalid, or a + * reserved bit is set. + * + * H_P2 The given "watchdogNumber" is zero or exceeds the + * supported maximum value. + * + * H_P3 The given "timeoutInMs" is below the supported + * minimum value. + * + * H_NOOP The given "watchdogNumber" is already stopped. + * + * H_HARDWARE The operation failed for ineffable reasons. + * + * H_FUNCTION The H_WATCHDOG hypercall is not supported by this + * hypervisor. + * + * R4: + * + * - For the "Query Watchdog Capabilities" operation, a 64-bit + * structure defined as: + * + * Bits 0-15: The minimum supported timeout in milliseconds. + * Bits 16-31: The number of watchdogs supported. + * Bits 32-63: Reserved. + */ +#define PSERIES_WDTQ_MIN_TIMEOUT(cap) GETFIELD((cap), 0, 15) +#define PSERIES_WDTQ_MAX_NUMBER(cap) GETFIELD((cap), 16, 31) +#define PSERIES_WDTQ_RESERVED PPC_BITMASK(32, 63) + +/* + * - For the "Query Watchdog LPM Requirement" operation: + * + * 1 The given "watchdogNumber" must be stopped prior to + * suspending. + * + * 2 The given "watchdogNumber" does not have to be stopped + * prior to suspending. + */ +#define PSERIES_WDTQL_MUST_STOP 1 +#define PSERIES_WDTQL_NEED_NOT_STOP 2 + +static unsigned long action = PSERIES_WDTF_ACTION_HARD_RESTART; +static int action_set(const char *val, const struct kernel_param *kp) +{ + if (strcmp(val, "hard-poweroff") == 0) { + action = PSERIES_WDTF_ACTION_HARD_POWEROFF; + return 0; + } + if (strcmp(val, "hard-restart") == 0) { + action = PSERIES_WDTF_ACTION_HARD_RESTART; + return 0; + } + if (strcmp(val, "dump-restart") == 0) { + action = PSERIES_WDTF_ACTION_DUMP_RESTART; + return 0; + } + return -EINVAL; +} +static const struct kernel_param_ops action_ops = { .set = action_set }; +module_param_cb(action, &action_ops, NULL, S_IRUGO); +MODULE_PARM_DESC(action, "Action taken when watchdog expires: \"hard-poweroff\", \"hard-restart\", or \"dump-restart\" (default=\"hard-restart\")"); + +static bool nowayout = WATCHDOG_NOWAYOUT; +module_param(nowayout, bool, S_IRUGO); +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); + +#define WATCHDOG_TIMEOUT 60 +static unsigned int timeout = WATCHDOG_TIMEOUT; +module_param(timeout, uint, S_IRUGO); +MODULE_PARM_DESC(timeout, "Initial watchdog timeout in seconds (default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ")"); + +struct pseries_wdt { + struct watchdog_device wd; + unsigned long num; /* NB: Watchdog numbers are 1-based */ +}; + +static int pseries_wdt_start(struct watchdog_device *wdd) +{ + struct device *dev = wdd->parent; + struct pseries_wdt *pw = watchdog_get_drvdata(wdd); + unsigned long flags, msecs; + long rc; + + flags = action | PSERIES_WDTF_OP_START; + msecs = wdd->timeout * 1000UL; + 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; + } + return 0; +} + +static int pseries_wdt_stop(struct watchdog_device *wdd) +{ + struct device *dev = wdd->parent; + struct pseries_wdt *pw = watchdog_get_drvdata(wdd); + long rc; + + rc = plpar_hcall_norets(H_WATCHDOG, PSERIES_WDTF_OP_STOP, pw->num); + if (rc != H_SUCCESS && rc != H_NOOP) { + dev_crit(dev, "H_WATCHDOG: %ld: failed to stop timer %lu", + rc, pw->num); + return -EIO; + } + return 0; +} + +static struct watchdog_info pseries_wdt_info = { + .identity = DRV_NAME, + .options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT \ + | WDIOF_PRETIMEOUT, +}; + +static const struct watchdog_ops pseries_wdt_ops = { + .owner = THIS_MODULE, + .ping = pseries_wdt_start, + .start = pseries_wdt_start, + .stop = pseries_wdt_stop, +}; + +static int pseries_wdt_probe(struct platform_device *pdev) +{ + unsigned long ret[PLPAR_HCALL_BUFSIZE] = { 0 }; + unsigned long cap, min_timeout_ms; + long rc; + struct pseries_wdt *pw; + int err; + + rc = plpar_hcall(H_WATCHDOG, ret, PSERIES_WDTF_OP_QUERY); + if (rc != H_SUCCESS) + return (rc == H_FUNCTION) ? -ENODEV : -EIO; + cap = ret[0]; + + /* + * If the underlying id exceeds the number of available timers + * there is a bug in the platform device registration code. + */ + if (pdev->id < 0 || pdev->id >= PSERIES_WDTQ_MAX_NUMBER(cap)) + return -ENODEV; + + pw = devm_kzalloc(&pdev->dev, sizeof *pw, GFP_KERNEL); + if (pw == NULL) + return -ENOMEM; + pw->num = pdev->id + 1; /* 0-based -> 1-based */ + pw->wd.parent = &pdev->dev; + pw->wd.info = &pseries_wdt_info; + pw->wd.ops = &pseries_wdt_ops; + min_timeout_ms = PSERIES_WDTQ_MIN_TIMEOUT(cap); + pw->wd.min_timeout = roundup(min_timeout_ms, 1000) / 1000; + pw->wd.max_timeout = UINT_MAX; + watchdog_init_timeout(&pw->wd, timeout, NULL); + watchdog_set_nowayout(&pw->wd, nowayout); + watchdog_stop_on_reboot(&pw->wd); + watchdog_stop_on_unregister(&pw->wd); + watchdog_set_drvdata(&pw->wd, pw); + + err = devm_watchdog_register_device(&pdev->dev, &pw->wd); + if (err) + return err; + + platform_set_drvdata(pdev, &pw->wd); + + return 0; +} + +static int pseries_wdt_suspend(struct platform_device *pdev, pm_message_t state) +{ + struct watchdog_device *wd = platform_get_drvdata(pdev); + + if (watchdog_active(wd)) + return pseries_wdt_stop(wd); + return 0; +} + +static int pseries_wdt_resume(struct platform_device *pdev) +{ + struct watchdog_device *wd = platform_get_drvdata(pdev); + + if (watchdog_active(wd)) + return pseries_wdt_start(wd); + return 0; +} + +static struct platform_driver pseries_wdt_driver = { + .driver = { + .name = DRV_NAME, + .owner = THIS_MODULE, + }, + .probe = pseries_wdt_probe, + .resume = pseries_wdt_resume, + .suspend = pseries_wdt_suspend, +}; +module_platform_driver(pseries_wdt_driver); + +MODULE_AUTHOR("Alexey Kardashevskiy <aik@ozlabs.ru>"); +MODULE_AUTHOR("Scott Cheloha <cheloha@linux.ibm.com>"); +MODULE_DESCRIPTION("POWER Architecture Platform Watchdog Driver"); +MODULE_LICENSE("GPL"); -- 2.27.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC v2 2/2] watchdog: pseries-wdt: initial support for PAPR virtual watchdog timers 2022-05-09 17:43 ` [RFC v2 2/2] watchdog: pseries-wdt: initial support for PAPR virtual watchdog timers Scott Cheloha @ 2022-05-10 2:35 ` Tzung-Bi Shih 2022-05-10 3:34 ` Guenter Roeck 2022-05-10 15:48 ` Scott Cheloha 0 siblings, 2 replies; 12+ messages in thread From: Tzung-Bi Shih @ 2022-05-10 2:35 UTC (permalink / raw) To: Scott Cheloha Cc: linux-watchdog, linux, brking, nathanl, aik, npiggin, vaishnavi, wvoigt 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). ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 2/2] watchdog: pseries-wdt: initial support for PAPR virtual watchdog timers 2022-05-10 2:35 ` Tzung-Bi Shih @ 2022-05-10 3:34 ` Guenter Roeck 2022-05-11 5:08 ` Alexey Kardashevskiy 2022-05-10 15:48 ` Scott Cheloha 1 sibling, 1 reply; 12+ messages in thread From: Guenter Roeck @ 2022-05-10 3:34 UTC (permalink / raw) To: Tzung-Bi Shih, Scott Cheloha Cc: linux-watchdog, brking, nathanl, aik, npiggin, vaishnavi, wvoigt 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 2/2] watchdog: pseries-wdt: initial support for PAPR virtual watchdog timers 2022-05-10 3:34 ` Guenter Roeck @ 2022-05-11 5:08 ` Alexey Kardashevskiy 2022-05-11 5:27 ` Guenter Roeck 0 siblings, 1 reply; 12+ messages in thread From: Alexey Kardashevskiy @ 2022-05-11 5:08 UTC (permalink / raw) To: Guenter Roeck, Tzung-Bi Shih, Scott Cheloha Cc: linux-watchdog, brking, nathanl, npiggin, vaishnavi, wvoigt 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 2/2] watchdog: pseries-wdt: initial support for PAPR virtual watchdog timers 2022-05-11 5:08 ` Alexey Kardashevskiy @ 2022-05-11 5:27 ` Guenter Roeck 0 siblings, 0 replies; 12+ messages in thread From: Guenter Roeck @ 2022-05-11 5:27 UTC (permalink / raw) To: Alexey Kardashevskiy, Tzung-Bi Shih, Scott Cheloha Cc: linux-watchdog, brking, nathanl, npiggin, vaishnavi, wvoigt On 5/10/22 22:08, Alexey Kardashevskiy wrote: > > > 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, > I won't accept this. I don't mind giving an Ack to a patch applied elsewhere, and I don't mind applying patches from another subsystem if Acked by affected maintainers, but I strongly believe that there should be no cross-subsystem patches. Also, I don't consider 2) and 3) to be a single logical change, and the rule for patches is "one logical change per patch". Other maintainers may handle this differently, but that is their call and responsibility. Guenter ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 2/2] watchdog: pseries-wdt: initial support for PAPR virtual watchdog timers 2022-05-10 2:35 ` Tzung-Bi Shih 2022-05-10 3:34 ` Guenter Roeck @ 2022-05-10 15:48 ` Scott Cheloha 2022-05-10 16:04 ` Guenter Roeck 1 sibling, 1 reply; 12+ messages in thread From: Scott Cheloha @ 2022-05-10 15:48 UTC (permalink / raw) To: Tzung-Bi Shih Cc: linux-watchdog, linux, brking, Nathan Lynch, aik, npiggin, vaishnavi, wvoigt > 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 2/2] watchdog: pseries-wdt: initial support for PAPR virtual watchdog timers 2022-05-10 15:48 ` Scott Cheloha @ 2022-05-10 16:04 ` Guenter Roeck 2022-05-10 16:15 ` Scott Cheloha 0 siblings, 1 reply; 12+ messages in thread From: Guenter Roeck @ 2022-05-10 16:04 UTC (permalink / raw) To: Scott Cheloha, Tzung-Bi Shih Cc: linux-watchdog, brking, Nathan Lynch, aik, npiggin, vaishnavi, wvoigt On 5/10/22 08:48, Scott Cheloha wrote: >> 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? > You can use MODULE_PARM_DESC(action, "Some text"); > `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. > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 2/2] watchdog: pseries-wdt: initial support for PAPR virtual watchdog timers 2022-05-10 16:04 ` Guenter Roeck @ 2022-05-10 16:15 ` Scott Cheloha 2022-05-10 19:00 ` Guenter Roeck 0 siblings, 1 reply; 12+ messages in thread From: Scott Cheloha @ 2022-05-10 16:15 UTC (permalink / raw) To: Guenter Roeck Cc: Tzung-Bi Shih, linux-watchdog, brking, Nathan Lynch, aik, npiggin, vaishnavi, wvoigt > On May 10, 2022, at 11:04 AM, Guenter Roeck <linux@roeck-us.net> wrote: > > On 5/10/22 08:48, Scott Cheloha wrote: >>> 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? > You can use > > MODULE_PARM_DESC(action, > "Some text"); The line is still over 100 columns if I do this. I can shrink the line by removing the valid inputs from the string if 100 columns is a hard rule. If so, where should I document the valid inputs instead? Is Documentation/watchdog/watchdog-parameters.rst a better place for them? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 2/2] watchdog: pseries-wdt: initial support for PAPR virtual watchdog timers 2022-05-10 16:15 ` Scott Cheloha @ 2022-05-10 19:00 ` Guenter Roeck 0 siblings, 0 replies; 12+ messages in thread From: Guenter Roeck @ 2022-05-10 19:00 UTC (permalink / raw) To: Scott Cheloha Cc: Tzung-Bi Shih, linux-watchdog, brking, Nathan Lynch, aik, npiggin, vaishnavi, wvoigt On 5/10/22 09:15, Scott Cheloha wrote: > > >> On May 10, 2022, at 11:04 AM, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 5/10/22 08:48, Scott Cheloha wrote: >>>> 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? >> You can use >> >> MODULE_PARM_DESC(action, >> "Some text"); > > The line is still over 100 columns if I do this. > You might consider using shorter strings, such as "poweroff", "restart", "dump". The meaning needs to be documented anyway. You could also drop "or" because it doesn't add value in this context, and the '"' don't really add value either. On a side note, personally I don't like textual module parameters because they make the command line extremely long and add complexity to decoding it, but that is your call and responsibility (and you are not the only one using strings). Guenter > I can shrink the line by removing the valid inputs from the string if 100 columns > is a hard rule. > > If so, where should I document the valid inputs instead? > > Is Documentation/watchdog/watchdog-parameters.rst a better place for them? > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 0/2] Add driver for PAPR watchdog timers 2022-05-09 17:43 [RFC v2 0/2] Add driver for PAPR watchdog timers Scott Cheloha 2022-05-09 17:43 ` [RFC v2 1/2] powerpc/pseries: hvcall.h: add H_WATCHDOG opcode, H_NOOP return code Scott Cheloha 2022-05-09 17:43 ` [RFC v2 2/2] watchdog: pseries-wdt: initial support for PAPR virtual watchdog timers Scott Cheloha @ 2022-05-11 5:38 ` Alexey Kardashevskiy 2 siblings, 0 replies; 12+ messages in thread From: Alexey Kardashevskiy @ 2022-05-11 5:38 UTC (permalink / raw) To: Scott Cheloha, linux-watchdog Cc: linux, tzungbi, brking, nathanl, npiggin, vaishnavi, wvoigt You should cc: linuxppc-dev@lists.ozlabs.org this. Thanks, On 5/10/22 03:43, Scott Cheloha wrote: > This second RFC incorporates feedback from the previous RFC: > > https://lore.kernel.org/linux-watchdog/20220413165104.179144-1-cheloha@linux.ibm.com/ > > v2 changes of note: > > - Add a firmware feature flag for the H_WATCHDOG feature, > FW_FEATURE_WATCHDOG. > > - Register a platform_device for the first watchdog timer during a > pseries initcall if we have FW_FEATURE_WATCHDOG. Use id zero, as > there could be more than one timer in the future. > > - Alphabetize Makefile changes. > > - Add missing copyright information to pseries-wdt.c. > > - Add an 'action' module parameter that configures how the guest is > terminated on watchdog expiration. > > - Use dev_*() for logging critical errors instead of pr_*(). > > - Handle the H_NOOP case when trying to stop the watchdog. If the > given watchdog is not actually running, H_WATCHDOG returns H_NOOP. > This is harmless, so we should treat it as a success. > > - We don't need pseries_wdt_remove() at all. > > - Check watchdog_active() before stopping/starting the timer across > suspend/resume. > > - Consolidate all code from pseries_wdt_module_init() into > pseries_wdt_probe(). We can then use module_platform_driver(). > > I have one lingering question: > > - The pseries-wdt module is not "automatically" loaded during boot. > > When I do > > # modprobe pseries-wdt > > the driver attaches to the platform bus as expected and the > /dev/watchdog* devices for the pseries-wdt.0 platform device > are created. > > I was under the impression that driver/device matching for > the platform bus was simple string comparison. > > ... what am I doing wrong? Is this expected behavior? Do > I need to do additional configuration to get the module to > load automatically at boot time? > -- Alexey ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-05-11 5:38 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-09 17:43 [RFC v2 0/2] Add driver for PAPR watchdog timers Scott Cheloha 2022-05-09 17:43 ` [RFC v2 1/2] powerpc/pseries: hvcall.h: add H_WATCHDOG opcode, H_NOOP return code Scott Cheloha 2022-05-09 17:43 ` [RFC v2 2/2] watchdog: pseries-wdt: initial support for PAPR virtual watchdog timers Scott Cheloha 2022-05-10 2:35 ` Tzung-Bi Shih 2022-05-10 3:34 ` Guenter Roeck 2022-05-11 5:08 ` Alexey Kardashevskiy 2022-05-11 5:27 ` Guenter Roeck 2022-05-10 15:48 ` Scott Cheloha 2022-05-10 16:04 ` Guenter Roeck 2022-05-10 16:15 ` Scott Cheloha 2022-05-10 19:00 ` Guenter Roeck 2022-05-11 5:38 ` [RFC v2 0/2] Add driver for PAPR " Alexey Kardashevskiy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.