Messages in this thread | | | Subject | Re: [char-misc-next v3 3/8] watchdog: mei_wdt: implement MEI iAMT watchdog driver | From | Guenter Roeck <> | Date | Mon, 21 Dec 2015 20:58:06 -0800 |
| |
On 12/21/2015 03:17 PM, Tomas Winkler wrote: > Create a driver with the generic watchdog interface > for the MEI iAMT watchdog device. > > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > --- > V2: The watchdog device is no longer dynamically allocated in separate structure > V3: Revert back to dynamically allocated watchdog device wrapper > > Documentation/misc-devices/mei/mei.txt | 12 +- > MAINTAINERS | 1 + > drivers/watchdog/Kconfig | 15 + > drivers/watchdog/Makefile | 1 + > drivers/watchdog/mei_wdt.c | 481 +++++++++++++++++++++++++++++++++ > 5 files changed, 504 insertions(+), 6 deletions(-) > create mode 100644 drivers/watchdog/mei_wdt.c > > diff --git a/Documentation/misc-devices/mei/mei.txt b/Documentation/misc-devices/mei/mei.txt > index 91c1fa34f48b..2b80a0cd621f 100644 > --- a/Documentation/misc-devices/mei/mei.txt > +++ b/Documentation/misc-devices/mei/mei.txt > @@ -231,15 +231,15 @@ IT knows when a platform crashes even when there is a hard failure on the host. > The Intel AMT Watchdog is composed of two parts: > 1) Firmware feature - receives the heartbeats > and sends an event when the heartbeats stop. > - 2) Intel MEI driver - connects to the watchdog feature, configures the > - watchdog and sends the heartbeats. > + 2) Intel MEI iAMT watchdog driver - connects to the watchdog feature, > + configures the watchdog and sends the heartbeats. > > -The Intel MEI driver uses the kernel watchdog API to configure the Intel AMT > -Watchdog and to send heartbeats to it. The default timeout of the > +The Intel iAMT watchdog MEI driver uses the kernel watchdog API to configure > +the Intel AMT Watchdog and to send heartbeats to it. The default timeout of the > watchdog is 120 seconds. > > -If the Intel AMT Watchdog feature does not exist (i.e. the connection failed), > -the Intel MEI driver will disable the sending of heartbeats. > +If the Intel AMT is not enabled in the firmware then the watchdog client won't enumerate > +on the me client bus and watchdog devices won't be exposed. > > > Supported Chipsets > diff --git a/MAINTAINERS b/MAINTAINERS > index 9bff63cf326e..e655625c2c16 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5666,6 +5666,7 @@ S: Supported > F: include/uapi/linux/mei.h > F: include/linux/mei_cl_bus.h > F: drivers/misc/mei/* > +F: drivers/watchdog/mei_wdt.c > F: Documentation/misc-devices/mei/* > > INTEL MIC DRIVERS (mic) > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 1c427beffadd..8ac51d69785c 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -1154,6 +1154,21 @@ config SBC_EPX_C3_WATCHDOG > To compile this driver as a module, choose M here: the > module will be called sbc_epx_c3. > > +config INTEL_MEI_WDT > + tristate "Intel MEI iAMT Watchdog" > + depends on INTEL_MEI && X86 > + select WATCHDOG_CORE > + ---help--- > + A device driver for the Intel MEI iAMT watchdog. > + > + The Intel AMT Watchdog is an OS Health (Hang/Crash) watchdog. > + Whenever the OS hangs or crashes, iAMT will send an event > + to any subscriber to this event. The watchdog doesn't reset the > + the platform. > + > + To compile this driver as a module, choose M here: > + the module will be called mei_wdt. > + > # M32R Architecture > > # M68K Architecture > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 53d4827ddfe1..9069c9dd8aa8 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -123,6 +123,7 @@ obj-$(CONFIG_MACHZ_WDT) += machzwd.o > obj-$(CONFIG_SBC_EPX_C3_WATCHDOG) += sbc_epx_c3.o > obj-$(CONFIG_INTEL_SCU_WATCHDOG) += intel_scu_watchdog.o > obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o > +obj-$(CONFIG_INTEL_MEI_WDT) += mei_wdt.o > > # M32R Architecture > > diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c > new file mode 100644 > index 000000000000..5b28a1e95ac1 > --- /dev/null > +++ b/drivers/watchdog/mei_wdt.c > @@ -0,0 +1,481 @@ > +/* > + * Intel Management Engine Interface (Intel MEI) Linux driver > + * Copyright (c) 2015, Intel Corporation. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope 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. > + */ > + > +#include <linux/module.h> > +#include <linux/slab.h> > +#include <linux/interrupt.h> > +#include <linux/watchdog.h> > + > +#include <linux/uuid.h> > +#include <linux/mei_cl_bus.h> > + > +/* > + * iAMT Watchdog Device > + */ > +#define INTEL_AMT_WATCHDOG_ID "iamt_wdt" > + > +#define MEI_WDT_DEFAULT_TIMEOUT 120 /* seconds */ > +#define MEI_WDT_MIN_TIMEOUT 120 /* seconds */ > +#define MEI_WDT_MAX_TIMEOUT 65535 /* seconds */ > + > +/* Commands */ > +#define MEI_MANAGEMENT_CONTROL 0x02 > + > +/* MEI Management Control version number */ > +#define MEI_MC_VERSION_NUMBER 0x10 > + > +/* Sub Commands */ > +#define MEI_MC_START_WD_TIMER_REQ 0x13 > +#define MEI_MC_STOP_WD_TIMER_REQ 0x14 > + > +/** > + * enum mei_wdt_state - internal watchdog state > + * > + * @MEI_WDT_IDLE: wd is idle and not opened > + * @MEI_WDT_START: wd was opened, start was called > + * @MEI_WDT_RUNNING: wd is expecting keep alive pings > + * @MEI_WDT_STOPPING: wd is stopping and will move to IDLE > + */ > +enum mei_wdt_state { > + MEI_WDT_IDLE, > + MEI_WDT_START, > + MEI_WDT_RUNNING, > + MEI_WDT_STOPPING, > +}; > + > +struct mei_wdt; > + > +/** > + * struct mei_wdt_dev - watchdog device wrapper > + * > + * @wdd: watchdog device > + * @wdt: back pointer to mei_wdt driver > + * @refcnt: reference counter > + */ > +struct mei_wdt_dev { > + struct watchdog_device wdd; > + struct mei_wdt *wdt; > + struct kref refcnt; > +}; > + > +/** > + * struct mei_wdt - mei watchdog driver > + * @mwd: watchdog device wrapper > + * > + * @cldev: mei watchdog client device > + * @state: watchdog internal state > + * @timeout: watchdog current timeout > + */ > +struct mei_wdt { > + struct mei_wdt_dev *mwd; > + > + struct mei_cl_device *cldev; > + enum mei_wdt_state state; > + u16 timeout; > +}; > + > +/* > + * struct mei_mc_hdr - Management Control Command Header > + * > + * @command: Management Control (0x2) > + * @bytecount: Number of bytes in the message beyond this byte > + * @subcommand: Management Control Subcommand > + * @versionnumber: Management Control Version (0x10) > + */ > +struct mei_mc_hdr { > + u8 command; > + u8 bytecount; > + u8 subcommand; > + u8 versionnumber; > +}; > + > +/** > + * struct mei_wdt_start_request watchdog start/ping > + * > + * @hdr: Management Control Command Header > + * @timeout: timeout value > + * @reserved: reserved (legacy) > + */ > +struct mei_wdt_start_request { > + struct mei_mc_hdr hdr; > + u16 timeout; > + u8 reserved[17]; > +} __packed; > + > +/** > + * struct mei_wdt_stop_request - watchdog stop > + * > + * @hdr: Management Control Command Header > + */ > +struct mei_wdt_stop_request { > + struct mei_mc_hdr hdr; > +} __packed; > + > +/** > + * mei_wdt_ping - send wd start/ping command > + * > + * @wdt: mei watchdog device > + * > + * Return: 0 on success, > + * negative errno code on failure > + */ > +static int mei_wdt_ping(struct mei_wdt *wdt) > +{ > + struct mei_wdt_start_request req; > + const size_t req_len = sizeof(req); > + int ret; > + > + memset(&req, 0, req_len); > + req.hdr.command = MEI_MANAGEMENT_CONTROL; > + req.hdr.bytecount = req_len - offsetof(struct mei_mc_hdr, subcommand); > + req.hdr.subcommand = MEI_MC_START_WD_TIMER_REQ; > + req.hdr.versionnumber = MEI_MC_VERSION_NUMBER; > + req.timeout = wdt->timeout; > + > + ret = mei_cldev_send(wdt->cldev, (u8 *)&req, req_len); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +/** > + * mei_wdt_stop - send wd stop command > + * > + * @wdt: mei watchdog device > + * > + * Return: 0 on success, > + * negative errno code on failure > + */ > +static int mei_wdt_stop(struct mei_wdt *wdt) > +{ > + struct mei_wdt_stop_request req; > + const size_t req_len = sizeof(req); > + int ret; > + > + memset(&req, 0, req_len); > + req.hdr.command = MEI_MANAGEMENT_CONTROL; > + req.hdr.bytecount = req_len - offsetof(struct mei_mc_hdr, subcommand); > + req.hdr.subcommand = MEI_MC_STOP_WD_TIMER_REQ; > + req.hdr.versionnumber = MEI_MC_VERSION_NUMBER; > + > + ret = mei_cldev_send(wdt->cldev, (u8 *)&req, req_len); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +/** > + * mei_wdt_ops_start - wd start command from the watchdog core. > + * > + * @wdd: watchdog device > + * > + * Return: 0 on success or -ENODEV; > + */ > +static int mei_wdt_ops_start(struct watchdog_device *wdd) > +{ > + struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd); > + struct mei_wdt *wdt; > + > + if (!mwd) > + return -ENODEV; > + I don't find a code path where this can be NULL. I also checked later patches, but I just don't see it.
Can you clarify how this can happen ? If this is just for safety, it should go. We would _want_ the driver to crash unless this is a valid condition.
Several other similar checks below.
> + wdt = mwd->wdt; > + > + wdt->state = MEI_WDT_START; > + wdd->timeout = wdt->timeout; > + return 0; > +} > + > +/** > + * mei_wdt_ops_stop - wd stop command from the watchdog core. > + * > + * @wdd: watchdog device > + * > + * Return: 0 if success, negative errno code for failure > + */ > +static int mei_wdt_ops_stop(struct watchdog_device *wdd) > +{ > + struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd); > + struct mei_wdt *wdt; > + int ret; > + > + if (!mwd) > + return -ENODEV; > + Same here.
> + wdt = mwd->wdt; > + > + if (wdt->state != MEI_WDT_RUNNING) > + return 0; > + > + wdt->state = MEI_WDT_STOPPING; > + > + ret = mei_wdt_stop(wdt); > + if (ret) > + return ret; > + > + wdt->state = MEI_WDT_IDLE; > + Just wondering ... is MEI_WDT_STOPPING necessary ? At least from an ops perspective, this function is atomic (protected by a mutex in the watchdog core).
> + return 0; > +} > + > +/** > + * mei_wdt_ops_ping - wd ping command from the watchdog core. > + * > + * @wdd: watchdog device > + * > + * Return: 0 if success, negative errno code on failure > + */ > +static int mei_wdt_ops_ping(struct watchdog_device *wdd) > +{ > + struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd); > + struct mei_wdt *wdt; > + int ret; > + > + if (!mwd) > + return -ENODEV; > + And here.
> + wdt = mwd->wdt; > + > + if (wdt->state != MEI_WDT_START && wdt->state != MEI_WDT_RUNNING) > + return 0; > + > + ret = mei_wdt_ping(wdt); > + if (ret) > + return ret; > + > + wdt->state = MEI_WDT_RUNNING; > + > + return 0; > +} > + > +/** > + * mei_wdt_ops_set_timeout - wd set timeout command from the watchdog core. > + * > + * @wdd: watchdog device > + * @timeout: timeout value to set > + * > + * Return: 0 if success, negative errno code for failure > + */ > +static int mei_wdt_ops_set_timeout(struct watchdog_device *wdd, > + unsigned int timeout) > +{ > + > + struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd); > + struct mei_wdt *wdt; > + > + if (!mwd) > + return -ENODEV; > +
And here.
> + wdt = mwd->wdt; > + > + /* valid value is already checked by the caller */ > + wdt->timeout = timeout; > + wdd->timeout = timeout; > + > + return 0; > +} > + > +static void mei_wdt_release(struct kref *ref) > +{ > + struct mei_wdt_dev *mwd = container_of(ref, struct mei_wdt_dev, refcnt); > + > + kfree(mwd); > +} > + > +static void mei_wdt_ops_ref(struct watchdog_device *wdd) > +{ > + struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd); > + > + kref_get(&mwd->refcnt); > +} > + > +static void mei_wdt_ops_unref(struct watchdog_device *wdd) > +{ > + struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd); > + > + kref_put(&mwd->refcnt, mei_wdt_release); > +} > + > +static const struct watchdog_ops wd_ops = { > + .owner = THIS_MODULE, > + .start = mei_wdt_ops_start, > + .stop = mei_wdt_ops_stop, > + .ping = mei_wdt_ops_ping, > + .set_timeout = mei_wdt_ops_set_timeout, > + .ref = mei_wdt_ops_ref, > + .unref = mei_wdt_ops_unref, > +}; > + > +static struct watchdog_info wd_info = { > + .identity = INTEL_AMT_WATCHDOG_ID, > + .options = WDIOF_KEEPALIVEPING | > + WDIOF_SETTIMEOUT | > + WDIOF_ALARMONLY, > +}; > + > +/** > + * mei_wdt_unregister - unregister from the watchdog subsystem > + * > + * @wdt: mei watchdog device > + */ > +static void mei_wdt_unregister(struct mei_wdt *wdt) > +{ > + struct mei_wdt_dev *mwd = wdt->mwd; > + > + if (!mwd) > + return; > + And here.
> + watchdog_unregister_device(&mwd->wdd); > + wdt->mwd = NULL; > + wdt->state = MEI_WDT_IDLE; > + kref_put(&mwd->refcnt, mei_wdt_release); > +} > + > +/** > + * mei_wdt_register - register with the watchdog subsystem > + * > + * @wdt: mei watchdog device > + * > + * Return: 0 if success, negative errno code for failure > + */ > +static int mei_wdt_register(struct mei_wdt *wdt) > +{ > + struct mei_wdt_dev *mwd; > + struct device *dev; > + int ret; > + > + if (!wdt || !wdt->cldev) > + return -EINVAL; > + And here.
> + dev = &wdt->cldev->dev; > + > + mwd = kzalloc(sizeof(struct mei_wdt_dev), GFP_KERNEL); > + if (!mwd) > + return -ENOMEM; > + > + mwd->wdt = wdt; > + mwd->wdd.info = &wd_info; > + mwd->wdd.ops = &wd_ops; > + mwd->wdd.parent = dev; > + mwd->wdd.timeout = MEI_WDT_DEFAULT_TIMEOUT; > + mwd->wdd.min_timeout = MEI_WDT_MIN_TIMEOUT; > + mwd->wdd.max_timeout = MEI_WDT_MAX_TIMEOUT; > + kref_init(&mwd->refcnt); > + > + watchdog_set_drvdata(&mwd->wdd, mwd); > + ret = watchdog_register_device(&mwd->wdd); > + if (ret) { > + dev_err(dev, "unable to register watchdog device = %d.\n", ret); > + kref_put(&mwd->refcnt, mei_wdt_release); > + return ret; > + } > + > + wdt->mwd = mwd; > + return 0; > +} > + > +static int mei_wdt_probe(struct mei_cl_device *cldev, > + const struct mei_cl_device_id *id) > +{ > + struct mei_wdt *wdt; > + int ret; > + > + wdt = kzalloc(sizeof(struct mei_wdt), GFP_KERNEL); > + if (!wdt) > + return -ENOMEM; > + > + wdt->timeout = MEI_WDT_DEFAULT_TIMEOUT; > + wdt->state = MEI_WDT_IDLE; > + wdt->cldev = cldev; > + mei_cldev_set_drvdata(cldev, wdt); > + > + ret = mei_cldev_enable(cldev); > + if (ret < 0) { > + dev_err(&cldev->dev, "Could not enable cl device\n"); > + goto err_out; > + } > + > + wd_info.firmware_version = mei_cldev_ver(cldev); > + > + ret = mei_wdt_register(wdt); > + if (ret) > + goto err_disable; > + > + return 0; > + > +err_disable: > + mei_cldev_disable(cldev); > + > +err_out: > + kfree(wdt); > + > + return ret; > +} > + > +static int mei_wdt_remove(struct mei_cl_device *cldev) > +{ > + struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev); > + > + mei_wdt_unregister(wdt); > + > + mei_cldev_disable(cldev); > + > + kfree(wdt); > + > + return 0; > +} > + > +#define MEI_UUID_WD UUID_LE(0x05B79A6F, 0x4628, 0x4D7F, \ > + 0x89, 0x9D, 0xA9, 0x15, 0x14, 0xCB, 0x32, 0xAB) > + > +static struct mei_cl_device_id mei_wdt_tbl[] = { > + { .uuid = MEI_UUID_WD, .version = 0x1}, > + /* required last entry */ > + { } > +}; > +MODULE_DEVICE_TABLE(mei, mei_wdt_tbl); > + > +static struct mei_cl_driver mei_wdt_driver = { > + .id_table = mei_wdt_tbl, > + .name = KBUILD_MODNAME, > + > + .probe = mei_wdt_probe, > + .remove = mei_wdt_remove, > +}; > + > +static int __init mei_wdt_init(void) > +{ > + int ret; > + > + ret = mei_cldev_driver_register(&mei_wdt_driver); > + if (ret) { > + pr_err(KBUILD_MODNAME ": module registration failed\n"); > + return ret; > + } > + return 0; > +} > + > +static void __exit mei_wdt_exit(void) > +{ > + mei_cldev_driver_unregister(&mei_wdt_driver); > +} > + > +module_init(mei_wdt_init); > +module_exit(mei_wdt_exit); > + > +MODULE_AUTHOR("Intel Corporation"); > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("Device driver for Intel MEI iAMT watchdog"); >
| |