lkml.org 
[lkml]   [2015]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [char-misc-next 3/6] mei: wd: implement MEI iAMT watchdog driver
    Date
    On Mon, 2015-11-30 at 08:55 -0800, Guenter Roeck wrote:
    > On 11/26/2015 04:31 AM, Tomas Winkler wrote:
    > > From: Alexander Usyskin <alexander.usyskin@intel.com>
    > >
    > > 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>
    > > ---
    > > Documentation/misc-devices/mei/mei.txt | 12 +-
    > > MAINTAINERS | 1 +
    > > drivers/watchdog/Kconfig | 15 ++
    > > drivers/watchdog/Makefile | 1 +
    > > drivers/watchdog/mei_wdt.c | 432
    > > +++++++++++++++++++++++++++++++++
    > > 5 files changed, 455 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 050d0e77a2cf..cf0a51518f4a 100644
    > > --- a/MAINTAINERS
    > > +++ b/MAINTAINERS
    > > @@ -5664,6 +5664,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 7a8a6c6952e9..ec584714829d 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..149b29f341cf
    > > --- /dev/null
    > > +++ b/drivers/watchdog/mei_wdt.c
    > > @@ -0,0 +1,432 @@
    > > +/*
    > > + * 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 */
    >
    > Is the large minimum timeout on purpose ?
    > Just asking, since it is quite unusual.

    The recommendation is to ping each 90 sec and set the value to 120.
    I think there is no HW limitation, but since this is only a monitoring
    watchdog this value is sufficient for the use case.

    >
    > > +#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
    > > + *
    > > + * @cldev: mei watchdog client device
    > > + * @mwd: watchdog device wrapper
    > > + * @state: watchdog internal state
    > > + * @timeout: watchdog current timeout
    > > + */
    > > +struct mei_wdt {
    > > + struct mei_cl_device *cldev;
    > > + struct mei_wdt_dev *mwd;
    > > + enum mei_wdt_state state;
    > > + u16 timeout;
    > > +};
    >
    > Any special reason for having two data structures instead of one ?
    > You could just move the variables from struct mei_wdt_dev into
    > struct mei_wdt, no ?

    Yes, on newer platform mei_wdt_dev might be not present in case the the
    device is not provisioned. This came to action in the following
    patches.

    > > +
    > > +struct mei_wdt_hdr {
    > > + u8 command;
    > > + u8 bytecount;
    > > + u8 subcommand;
    > > + u8 versionnumber;
    > > +};
    > > +
    > > +struct mei_wdt_start_request {
    > > + struct mei_wdt_hdr hdr;
    > > + u16 timeout;
    > > + u8 reserved[17];
    > > +} __packed;
    > > +
    > > +struct mei_wdt_stop_request {
    > > + struct mei_wdt_hdr hdr;
    > > +} __packed;
    > > +
    > > +/**
    > > + * mei_wdt_ping - send wd start command
    > > + *
    > > + * @wdt: mei watchdog device
    > > + *
    > > + * Return: number of bytes sent 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);
    > > +
    > > + memset(&req, 0, req_len);
    > > + req.hdr.command = MEI_MANAGEMENT_CONTROL;
    > > + req.hdr.bytecount = req_len - offsetof(struct mei_wdt_hdr,
    > > subcommand);
    > > + req.hdr.subcommand = MEI_MC_START_WD_TIMER_REQ;
    > > + req.hdr.versionnumber = MEI_MC_VERSION_NUMBER;
    > > + req.timeout = wdt->timeout;
    > > +
    > > + return mei_cldev_send(wdt->cldev, (u8 *)&req, req_len);
    > > +}
    > > +
    > > +/**
    > > + * mei_wdt_stop - send wd stop command
    > > + *
    > > + * @wdt: mei watchdog device
    > > + *
    > > + * Return: number of bytes sent 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);
    > > +
    > > + memset(&req, 0, req_len);
    > > + req.hdr.command = MEI_MANAGEMENT_CONTROL;
    > > + req.hdr.bytecount = req_len - offsetof(struct mei_wdt_hdr,
    > > subcommand);
    > > + req.hdr.subcommand = MEI_MC_STOP_WD_TIMER_REQ;
    > > + req.hdr.versionnumber = MEI_MC_VERSION_NUMBER;
    > > +
    > > + return mei_cldev_send(wdt->cldev, (u8 *)&req, req_len);
    > > +}
    > > +
    > > +/**
    > > + * 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);
    > > +
    > > + if (!mwd)
    > > + return -ENODEV;
    >
    > This can only happen because you call watchdog_set_drvdata() after
    > watchdog device registration. If that happens, the system is in
    > really bad shape.

    mei_wdt_dev can destroyed during
    driver operation if the device is unprovisioned, but still you the
    condition should not happen unless we have a bug. We can put WARN_ON()
    there.

    >
    > I would suggest to move the call to watchdog_set_drvdata() ahead
    > of watchdog_register_device() and drop those checks.
    >
    > > +
    > > + mwd->wdt->state = MEI_WDT_START;
    > > + wdd->timeout = mwd->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;
    > > +
    > > + wdt = mwd->wdt;
    > > +
    > > + if (wdt->state != MEI_WDT_RUNNING)
    > > + return 0;
    > > +
    > > + wdt->state = MEI_WDT_STOPPING;
    > > +
    > > + ret = mei_wdt_stop(wdt);
    > > + if (ret < 0)
    > > + return ret;
    > > +
    > > + wdt->state = MEI_WDT_IDLE;
    > > +
    > > + 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;
    > > +
    > > + wdt = mwd->wdt;
    > > +
    > > + if (wdt->state != MEI_WDT_START &&
    > > + wdt->state != MEI_WDT_RUNNING)
    >
    > Unnecessary continuation line ?
    Looks more readable to me. but we can also straight the condition.
    >
    > > + return 0;
    > > +
    > > + ret = mei_wdt_ping(wdt);
    > > + if (ret < 0)
    > > + 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;
    > > +
    > > + wdt = mwd->wdt;
    > > +
    > > + /* valid value is already checked by the caller */
    > > + wdt->timeout = timeout;
    > > + wdd->timeout = timeout;
    >
    > One of those seems unnecessary. Why keep the timeout twice ?

    We need two as wdd may not exists and we still need to send ping to
    detect if the device is provisioned.

    > > +
    > > + 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,
    > > +};
    > > +
    > > +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;
    > > +
    > > + 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);
    > > +
    > > + 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;
    > > + watchdog_set_drvdata(&mwd->wdd, mwd);
    > > + return 0;
    > > +}
    > > +
    > > +static void mei_wdt_unregister(struct mei_wdt *wdt)
    > > +{
    > > + if (!wdt->mwd)
    > > + return;
    > > +
    > > + watchdog_unregister_device(&wdt->mwd->wdd);
    > > + kref_put(&wdt->mwd->refcnt, mei_wdt_release);
    > > + wdt->mwd = NULL;
    > > +}
    >
    > Seems to me that using two separate data structures instead of one
    > adds a lot of complexity.

    It might be reduced but I'm not sure it can be significantly simpler.
    It the reference counter will be part of watchdog_device it would be
    simpler.

    > > +
    > > +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_cldev_disable(cldev);
    > > +
    > > + mei_wdt_unregister(wdt);
    > > +
    > > + 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 = MEI_CL_VERSION_ANY},
    > > + /* 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");
    > >
    >
    \
     
     \ /
      Last update: 2015-12-01 13:21    [W:4.664 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site