lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 23 Oct 2017 17:24:09 -0700 From: Andrey Smirnov <andrew.smirnov@...il.com> To: Guenter Roeck <linux@...ck-us.net> Cc: linux-kernel <linux-kernel@...r.kernel.org>, linux-watchdog@...r.kernel.org, Chris Healy <cphealy@...il.com>, Lucas Stach <l.stach@...gutronix.de>, Nikita Yushchenko <nikita.yoush@...entembedded.com>, Lee Jones <lee.jones@...aro.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Pavel Machek <pavel@....cz>, Andy Shevchenko <andy.shevchenko@...il.com>, Rob Herring <robh@...nel.org>, Johan Hovold <johan@...nel.org> Subject: Re: [v8,4/5] watchdog: Add RAVE SP watchdog driver On Mon, Oct 23, 2017 at 3:04 PM, Guenter Roeck <linux@...ck-us.net> wrote: > On Mon, Oct 23, 2017 at 10:01:42AM -0700, Andrey Smirnov wrote: >> On Sat, Oct 21, 2017 at 9:47 AM, Guenter Roeck <linux@...ck-us.net> wrote: >> > On Wed, Oct 18, 2017 at 10:01:35AM -0700, Andrey Smirnov wrote: >> >> This driver provides access to RAVE SP watchdog functionality. >> >> >> >> Cc: linux-kernel@...r.kernel.org >> >> Cc: linux-watchdog@...r.kernel.org >> >> Cc: cphealy@...il.com >> >> Cc: Lucas Stach <l.stach@...gutronix.de> >> >> Cc: Nikita Yushchenko <nikita.yoush@...entembedded.com> >> >> Cc: Lee Jones <lee.jones@...aro.org> >> >> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org> >> >> Cc: Pavel Machek <pavel@....cz> >> >> Cc: Andy Shevchenko <andy.shevchenko@...il.com> >> >> Cc: Guenter Roeck <linux@...ck-us.net> >> >> Cc: Rob Herring <robh@...nel.org> >> >> Cc: Johan Hovold <johan@...nel.org> >> >> Signed-off-by: Nikita Yushchenko <nikita.yoush@...entembedded.com> >> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@...il.com> >> >> --- >> >> drivers/watchdog/Kconfig | 7 + >> >> drivers/watchdog/Makefile | 1 + >> >> drivers/watchdog/rave-sp-wdt.c | 310 +++++++++++++++++++++++++++++++++++++++++ >> >> 3 files changed, 318 insertions(+) >> >> create mode 100644 drivers/watchdog/rave-sp-wdt.c >> >> >> >> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >> >> index c722cbfdc7e6..533a72248cd1 100644 >> >> --- a/drivers/watchdog/Kconfig >> >> +++ b/drivers/watchdog/Kconfig >> >> @@ -223,6 +223,13 @@ config ZIIRAVE_WATCHDOG >> >> To compile this driver as a module, choose M here: the >> >> module will be called ziirave_wdt. >> >> >> >> +config RAVE_SP_WATCHDOG >> >> + tristate "RAVE SP Watchdog timer" >> >> + depends on RAVE_SP_CORE >> >> + select WATCHDOG_CORE >> >> + help >> >> + Support for the watchdog on RAVE SP device. >> >> + >> >> # ALPHA Architecture >> >> >> >> # ARM Architecture >> >> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile >> >> index 56adf9fa67d0..5c9556c09f6e 100644 >> >> --- a/drivers/watchdog/Makefile >> >> +++ b/drivers/watchdog/Makefile >> >> @@ -223,3 +223,4 @@ obj-$(CONFIG_MAX77620_WATCHDOG) += max77620_wdt.o >> >> obj-$(CONFIG_ZIIRAVE_WATCHDOG) += ziirave_wdt.o >> >> obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o >> >> obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o >> >> +obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o >> >> diff --git a/drivers/watchdog/rave-sp-wdt.c b/drivers/watchdog/rave-sp-wdt.c >> >> new file mode 100644 >> >> index 000000000000..86fdc7fb1f92 >> >> --- /dev/null >> >> +++ b/drivers/watchdog/rave-sp-wdt.c >> >> @@ -0,0 +1,310 @@ >> >> +/* >> >> + * rave-sp-wdt.c - Watchdog driver preseing in RAVE SP >> > >> > present ? >> > >> >> Yup, my bad, will fix in v9. >> >> >> + * >> >> + * Copyright (C) 2017 Zodiac Inflight Innovation >> >> + * >> >> + * This program is free software; you can redistribute it and/or >> >> + * modify it under the terms of the GNU General Public License as >> >> + * published by the Free Software Foundation; either version 2 of >> >> + * the License, or (at your option) any later version. >> >> + * >> >> + * This program is distributed in the hope that it will be useful, >> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> >> + * GNU General Public License for more details. >> >> + * >> >> + * You should have received a copy of the GNU General Public License >> >> + * along with this program; if not, see <http://www.gnu.org/licenses/>. >> >> + */ >> >> + >> >> +#include <linux/delay.h> >> >> +#include <linux/kernel.h> >> >> +#include <linux/module.h> >> >> +#include <linux/of_device.h> >> >> +#include <linux/platform_device.h> >> >> +#include <linux/rave-sp.h> >> >> +#include <linux/watchdog.h> >> >> +#include <linux/nvmem-consumer.h> >> >> +#include <linux/reboot.h> >> >> +#include <linux/slab.h> >> > >> > Alphabetic order please. >> > >> >> OK, will do. >> >> >> + >> >> +enum { >> >> + RAVE_SP_RESET_BYTE = 1, >> >> + RAVE_SP_RESET_REASON_NORMAL = 0, >> >> + RAVE_SP_RESET_DELAY_MS = 500, >> >> +}; >> >> + >> >> +struct rave_sp_wdt_variant { >> >> + unsigned int max_timeout; >> >> + unsigned int min_timeout; >> >> + >> >> + int (*configure)(struct watchdog_device *); >> >> + int (*restart)(struct watchdog_device *); >> >> +}; >> >> + >> >> +struct rave_sp_wdt { >> >> + struct watchdog_device wdd; >> >> + struct rave_sp *sp; >> >> + const struct rave_sp_wdt_variant *variant; >> >> + struct notifier_block reboot_notifier; >> >> +}; >> >> + >> >> +static struct rave_sp_wdt *to_rave_sp_wdt(struct watchdog_device *wdd) >> >> +{ >> >> + return container_of(wdd, struct rave_sp_wdt, wdd); >> >> +} >> >> + >> >> +static int __rave_sp_wdt_exec(struct watchdog_device *wdd, >> >> + void *data, size_t data_size, >> >> + void *reply, size_t reply_size) >> >> +{ >> >> + return rave_sp_exec(to_rave_sp_wdt(wdd)->sp, >> >> + data, data_size, reply, reply_size); >> >> +} >> > >> > This extra function is only called once and thus quite pointless. >> > >> >> True. Will remove in v9. >> >> >> + >> >> +static int rave_sp_wdt_exec(struct watchdog_device *wdd, void *data, >> >> + size_t data_size) >> >> +{ >> >> + return __rave_sp_wdt_exec(wdd, data, data_size, NULL, 0); >> >> +} >> >> + >> >> + >> > >> > Please run checkpatch --strict and fix what it reports. >> > >> >> Sure, will do. >> >> >> +static int rave_sp_wdt_legacy_configure(struct watchdog_device *wdd) >> >> +{ >> >> + const bool enable = watchdog_hw_running(wdd); >> >> + u8 cmd[] = { >> >> + [0] = RAVE_SP_CMD_SW_WDT, >> >> + [1] = 0, >> >> + [2] = 0, >> >> + [3] = !!enable, >> > >> > Useless !!. It converts a boolean to a boolean. Besides, isn't it always >> > true anyway ? >> >> Good point, will remove the "!!". This function also is called as >> .stop method (via rave_sp_wdt_configure) for corresponding >> "watchdog_device" and I rely on code in watchdog_stop() to clear >> WDOG_HW_RUNNING. Is this not a correct thing to do? >> >> > >> >> + [4] = enable ? wdd->timeout : 0, >> >> + }; >> > >> > Interesting; checkpatch doesn't require an empty line after variable >> > declarations anymore ? Lets keep it anyway for consistency. >> > >> >> OK, will fix in v9. >> >> >> + return rave_sp_wdt_exec(wdd, cmd, sizeof(cmd)); >> >> +} >> >> + >> >> +static int rave_sp_wdt_rdu_configure(struct watchdog_device *wdd) >> >> +{ >> >> + u8 cmd[] = { >> >> + [0] = RAVE_SP_CMD_SW_WDT, >> >> + [1] = 0, >> >> + [2] = watchdog_hw_running(wdd), >> > >> > Nitpick, but isn't this always true ? There is no stop function, >> > and HW_RUNNING is set before this function is called. >> > >> >> Same as above. Let me know if relying on HW_RUNNING being cleared by >> watchdog_stop() is incorrect. >> > Yes, it is incorrect; the knowledge that the flag is cleared before calling > the driver stop function is not part of the API. You should pass that in as > flag. Good to know. Will fix in v9, then. Thanks, Andrey Smirnov
Powered by blists - more mailing lists