* [char-misc-next v3 1/8] mei: drop nfc leftovers from the mei driver
2015-12-21 23:17 [char-misc-next v3 0/8] mei: create proper iAMT watchdog driver Tomas Winkler
@ 2015-12-21 23:17 ` Tomas Winkler
2015-12-21 23:17 ` [char-misc-next v3 2/8] mei: wd: drop the watchdog code from the core " Tomas Winkler
` (6 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Tomas Winkler @ 2015-12-21 23:17 UTC (permalink / raw)
To: Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Alexander Usyskin, Guenter Roeck, linux-watchdog, linux-kernel,
Tomas Winkler
We left few function prototypes in the header file after
moving nfc logic to bus.
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2/V3: resend
drivers/misc/mei/mei_dev.h | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/drivers/misc/mei/mei_dev.h b/drivers/misc/mei/mei_dev.h
index 4250555d5e72..b54d9d9cacea 100644
--- a/drivers/misc/mei/mei_dev.h
+++ b/drivers/misc/mei/mei_dev.h
@@ -649,17 +649,6 @@ int mei_amthif_irq_read_msg(struct mei_cl *cl,
struct mei_cl_cb *complete_list);
int mei_amthif_irq_read(struct mei_device *dev, s32 *slots);
-/*
- * NFC functions
- */
-int mei_nfc_host_init(struct mei_device *dev, struct mei_me_client *me_cl);
-void mei_nfc_host_exit(struct mei_device *dev);
-
-/*
- * NFC Client UUID
- */
-extern const uuid_le mei_nfc_guid;
-
int mei_wd_send(struct mei_device *dev);
int mei_wd_stop(struct mei_device *dev);
int mei_wd_host_init(struct mei_device *dev, struct mei_me_client *me_cl);
--
2.4.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* [char-misc-next v3 2/8] mei: wd: drop the watchdog code from the core mei driver
2015-12-21 23:17 [char-misc-next v3 0/8] mei: create proper iAMT watchdog driver Tomas Winkler
2015-12-21 23:17 ` [char-misc-next v3 1/8] mei: drop nfc leftovers from the mei driver Tomas Winkler
@ 2015-12-21 23:17 ` Tomas Winkler
2015-12-21 23:17 ` [char-misc-next v3 3/8] watchdog: mei_wdt: implement MEI iAMT watchdog driver Tomas Winkler
` (5 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Tomas Winkler @ 2015-12-21 23:17 UTC (permalink / raw)
To: Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Alexander Usyskin, Guenter Roeck, linux-watchdog, linux-kernel,
Tomas Winkler
From: Alexander Usyskin <alexander.usyskin@intel.com>
Instead of integrating the iAMT watchdog in the mei core driver
we will create a watchdog device on the mei client bus and
create a driver for it.
This patch removes the watchdog code from the mei core driver.
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2/V3: resend
drivers/misc/mei/Kconfig | 6 +-
drivers/misc/mei/Makefile | 1 -
drivers/misc/mei/client.c | 12 +-
drivers/misc/mei/client.h | 4 -
drivers/misc/mei/init.c | 10 +-
drivers/misc/mei/interrupt.c | 15 --
drivers/misc/mei/mei_dev.h | 61 +------
drivers/misc/mei/wd.c | 391 -------------------------------------------
8 files changed, 9 insertions(+), 491 deletions(-)
delete mode 100644 drivers/misc/mei/wd.c
diff --git a/drivers/misc/mei/Kconfig b/drivers/misc/mei/Kconfig
index d23384dde73b..c49e1d2269af 100644
--- a/drivers/misc/mei/Kconfig
+++ b/drivers/misc/mei/Kconfig
@@ -1,6 +1,6 @@
config INTEL_MEI
tristate "Intel Management Engine Interface"
- depends on X86 && PCI && WATCHDOG_CORE
+ depends on X86 && PCI
help
The Intel Management Engine (Intel ME) provides Manageability,
Security and Media services for system containing Intel chipsets.
@@ -12,7 +12,7 @@ config INTEL_MEI
config INTEL_MEI_ME
tristate "ME Enabled Intel Chipsets"
select INTEL_MEI
- depends on X86 && PCI && WATCHDOG_CORE
+ depends on X86 && PCI
help
MEI support for ME Enabled Intel chipsets.
@@ -37,7 +37,7 @@ config INTEL_MEI_ME
config INTEL_MEI_TXE
tristate "Intel Trusted Execution Environment with ME Interface"
select INTEL_MEI
- depends on X86 && PCI && WATCHDOG_CORE
+ depends on X86 && PCI
help
MEI Support for Trusted Execution Environment device on Intel SoCs
diff --git a/drivers/misc/mei/Makefile b/drivers/misc/mei/Makefile
index 01447ca21c26..59e6b0aede34 100644
--- a/drivers/misc/mei/Makefile
+++ b/drivers/misc/mei/Makefile
@@ -9,7 +9,6 @@ mei-objs += interrupt.o
mei-objs += client.o
mei-objs += main.o
mei-objs += amthif.o
-mei-objs += wd.o
mei-objs += bus.o
mei-objs += bus-fixup.o
mei-$(CONFIG_DEBUG_FS) += debugfs.o
diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
index 72e32615acd9..e069fcaed7aa 100644
--- a/drivers/misc/mei/client.c
+++ b/drivers/misc/mei/client.c
@@ -648,7 +648,7 @@ int mei_cl_unlink(struct mei_cl *cl)
if (!cl)
return 0;
- /* wd and amthif might not be initialized */
+ /* amthif might not be initialized */
if (!cl->dev)
return 0;
@@ -679,17 +679,11 @@ void mei_host_client_init(struct work_struct *work)
mutex_lock(&dev->device_lock);
-
me_cl = mei_me_cl_by_uuid(dev, &mei_amthif_guid);
if (me_cl)
mei_amthif_host_init(dev, me_cl);
mei_me_cl_put(me_cl);
- me_cl = mei_me_cl_by_uuid(dev, &mei_wd_guid);
- if (me_cl)
- mei_wd_host_init(dev, me_cl);
- mei_me_cl_put(me_cl);
-
dev->dev_state = MEI_DEV_ENABLED;
dev->reset_count = 0;
mutex_unlock(&dev->device_lock);
@@ -1153,7 +1147,7 @@ err:
*
* Return: 1 if mei_flow_ctrl_creds >0, 0 - otherwise.
*/
-int mei_cl_flow_ctrl_creds(struct mei_cl *cl)
+static int mei_cl_flow_ctrl_creds(struct mei_cl *cl)
{
int rets;
@@ -1186,7 +1180,7 @@ int mei_cl_flow_ctrl_creds(struct mei_cl *cl)
* 0 on success
* -EINVAL when ctrl credits are <= 0
*/
-int mei_cl_flow_ctrl_reduce(struct mei_cl *cl)
+static int mei_cl_flow_ctrl_reduce(struct mei_cl *cl)
{
if (WARN_ON(!cl || !cl->me_cl))
return -EINVAL;
diff --git a/drivers/misc/mei/client.h b/drivers/misc/mei/client.h
index 04e1aa39243f..2e90a25f896e 100644
--- a/drivers/misc/mei/client.h
+++ b/drivers/misc/mei/client.h
@@ -18,7 +18,6 @@
#define _MEI_CLIENT_H_
#include <linux/types.h>
-#include <linux/watchdog.h>
#include <linux/poll.h>
#include <linux/mei.h>
@@ -120,9 +119,6 @@ struct mei_cl_cb *mei_cl_alloc_cb(struct mei_cl *cl, size_t length,
enum mei_cb_file_ops type, struct file *fp);
int mei_cl_flush_queues(struct mei_cl *cl, const struct file *fp);
-int mei_cl_flow_ctrl_creds(struct mei_cl *cl);
-
-int mei_cl_flow_ctrl_reduce(struct mei_cl *cl);
/*
* MEI input output function prototype
*/
diff --git a/drivers/misc/mei/init.c b/drivers/misc/mei/init.c
index 3edafc8d3ad4..46a4302e5524 100644
--- a/drivers/misc/mei/init.c
+++ b/drivers/misc/mei/init.c
@@ -156,8 +156,7 @@ int mei_reset(struct mei_device *dev)
mei_cl_all_wakeup(dev);
/* remove entry if already in list */
- dev_dbg(dev->dev, "remove iamthif and wd from the file list.\n");
- mei_cl_unlink(&dev->wd_cl);
+ dev_dbg(dev->dev, "remove iamthif from the file list.\n");
mei_cl_unlink(&dev->iamthif_cl);
mei_amthif_reset_params(dev);
}
@@ -165,7 +164,6 @@ int mei_reset(struct mei_device *dev)
mei_hbm_reset(dev);
dev->rd_msg_hdr = 0;
- dev->wd_pending = false;
if (ret) {
dev_err(dev->dev, "hw_reset failed ret = %d\n", ret);
@@ -335,16 +333,12 @@ void mei_stop(struct mei_device *dev)
mutex_lock(&dev->device_lock);
- mei_wd_stop(dev);
-
dev->dev_state = MEI_DEV_POWER_DOWN;
mei_reset(dev);
/* move device to disabled state unconditionally */
dev->dev_state = MEI_DEV_DISABLED;
mutex_unlock(&dev->device_lock);
-
- mei_watchdog_unregister(dev);
}
EXPORT_SYMBOL_GPL(mei_stop);
@@ -394,7 +388,6 @@ void mei_device_init(struct mei_device *dev,
init_waitqueue_head(&dev->wait_hw_ready);
init_waitqueue_head(&dev->wait_pg);
init_waitqueue_head(&dev->wait_hbm_start);
- init_waitqueue_head(&dev->wait_stop_wd);
dev->dev_state = MEI_DEV_INITIALIZING;
dev->reset_count = 0;
@@ -407,7 +400,6 @@ void mei_device_init(struct mei_device *dev,
INIT_WORK(&dev->init_work, mei_host_client_init);
INIT_WORK(&dev->reset_work, mei_reset_work);
- INIT_LIST_HEAD(&dev->wd_cl.link);
INIT_LIST_HEAD(&dev->iamthif_cl.link);
mei_io_list_init(&dev->amthif_cmd_list);
mei_io_list_init(&dev->amthif_rd_complete_list);
diff --git a/drivers/misc/mei/interrupt.c b/drivers/misc/mei/interrupt.c
index 64b568a0268d..6340dee33052 100644
--- a/drivers/misc/mei/interrupt.c
+++ b/drivers/misc/mei/interrupt.c
@@ -360,21 +360,6 @@ int mei_irq_write_handler(struct mei_device *dev, struct mei_cl_cb *cmpl_list)
list_move_tail(&cb->list, &cmpl_list->list);
}
- if (dev->wd_state == MEI_WD_STOPPING) {
- dev->wd_state = MEI_WD_IDLE;
- wake_up(&dev->wait_stop_wd);
- }
-
- if (mei_cl_is_connected(&dev->wd_cl)) {
- if (dev->wd_pending &&
- mei_cl_flow_ctrl_creds(&dev->wd_cl) > 0) {
- ret = mei_wd_send(dev);
- if (ret)
- return ret;
- dev->wd_pending = false;
- }
- }
-
/* complete control write list CB */
dev_dbg(dev->dev, "complete control write list cb.\n");
list_for_each_entry_safe(cb, next, &dev->ctrl_wr_list.list, list) {
diff --git a/drivers/misc/mei/mei_dev.h b/drivers/misc/mei/mei_dev.h
index b54d9d9cacea..da613268480c 100644
--- a/drivers/misc/mei/mei_dev.h
+++ b/drivers/misc/mei/mei_dev.h
@@ -18,7 +18,7 @@
#define _MEI_DEV_H_
#include <linux/types.h>
-#include <linux/watchdog.h>
+#include <linux/cdev.h>
#include <linux/poll.h>
#include <linux/mei.h>
#include <linux/mei_cl_bus.h>
@@ -26,33 +26,13 @@
#include "hw.h"
#include "hbm.h"
-/*
- * watch dog definition
- */
-#define MEI_WD_HDR_SIZE 4
-#define MEI_WD_STOP_MSG_SIZE MEI_WD_HDR_SIZE
-#define MEI_WD_START_MSG_SIZE (MEI_WD_HDR_SIZE + 16)
-
-#define MEI_WD_DEFAULT_TIMEOUT 120 /* seconds */
-#define MEI_WD_MIN_TIMEOUT 120 /* seconds */
-#define MEI_WD_MAX_TIMEOUT 65535 /* seconds */
-
-#define MEI_WD_STOP_TIMEOUT 10 /* msecs */
-
-#define MEI_WD_STATE_INDEPENDENCE_MSG_SENT (1 << 0)
-
-#define MEI_RD_MSG_BUF_SIZE (128 * sizeof(u32))
-
/*
* AMTHI Client UUID
*/
extern const uuid_le mei_amthif_guid;
-/*
- * Watchdog Client UUID
- */
-extern const uuid_le mei_wd_guid;
+#define MEI_RD_MSG_BUF_SIZE (128 * sizeof(u32))
/*
* Number of Maximum MEI Clients
@@ -78,7 +58,6 @@ extern const uuid_le mei_wd_guid;
*/
#define MEI_HOST_CLIENT_ID_ANY (-1)
#define MEI_HBM_HOST_CLIENT_ID 0 /* not used, just for documentation */
-#define MEI_WD_HOST_CLIENT_ID 1
#define MEI_IAMTHIF_HOST_CLIENT_ID 2
@@ -123,12 +102,6 @@ enum mei_file_transaction_states {
MEI_READ_COMPLETE
};
-enum mei_wd_states {
- MEI_WD_IDLE,
- MEI_WD_RUNNING,
- MEI_WD_STOPPING,
-};
-
/**
* enum mei_cb_file_ops - file operation associated with the callback
* @MEI_FOP_READ: read
@@ -404,7 +377,6 @@ const char *mei_pg_state_str(enum mei_pg_state state);
* @wait_hw_ready : wait queue for receive HW ready message form FW
* @wait_pg : wait queue for receive PG message from FW
* @wait_hbm_start : wait queue for receive HBM start message from FW
- * @wait_stop_wd : wait queue for receive WD stop message from FW
*
* @reset_count : number of consecutive resets
* @dev_state : device state
@@ -435,12 +407,6 @@ const char *mei_pg_state_str(enum mei_pg_state state);
*
* @allow_fixed_address: allow user space to connect a fixed client
*
- * @wd_cl : watchdog client
- * @wd_state : watchdog client state
- * @wd_pending : watchdog command is pending
- * @wd_timeout : watchdog expiration timeout
- * @wd_data : watchdog message buffer
- *
* @amthif_cmd_list : amthif list for cmd waiting
* @amthif_rd_complete_list : amthif list for reading completed cmd data
* @iamthif_file_object : file for current amthif operation
@@ -486,7 +452,6 @@ struct mei_device {
wait_queue_head_t wait_hw_ready;
wait_queue_head_t wait_pg;
wait_queue_head_t wait_hbm_start;
- wait_queue_head_t wait_stop_wd;
/*
* mei device states
@@ -531,13 +496,6 @@ struct mei_device {
bool allow_fixed_address;
- struct mei_cl wd_cl;
- enum mei_wd_states wd_state;
- bool wd_pending;
- u16 wd_timeout;
- unsigned char wd_data[MEI_WD_START_MSG_SIZE];
-
-
/* amthif list for cmd waiting */
struct mei_cl_cb amthif_cmd_list;
/* driver managed amthif list for reading completed amthif cmd data */
@@ -649,21 +607,6 @@ int mei_amthif_irq_read_msg(struct mei_cl *cl,
struct mei_cl_cb *complete_list);
int mei_amthif_irq_read(struct mei_device *dev, s32 *slots);
-int mei_wd_send(struct mei_device *dev);
-int mei_wd_stop(struct mei_device *dev);
-int mei_wd_host_init(struct mei_device *dev, struct mei_me_client *me_cl);
-/*
- * mei_watchdog_register - Registering watchdog interface
- * once we got connection to the WD Client
- * @dev: mei device
- */
-int mei_watchdog_register(struct mei_device *dev);
-/*
- * mei_watchdog_unregister - Unregistering watchdog interface
- * @dev: mei device
- */
-void mei_watchdog_unregister(struct mei_device *dev);
-
/*
* Register Access Function
*/
diff --git a/drivers/misc/mei/wd.c b/drivers/misc/mei/wd.c
deleted file mode 100644
index b346638833b0..000000000000
--- a/drivers/misc/mei/wd.c
+++ /dev/null
@@ -1,391 +0,0 @@
-/*
- *
- * Intel Management Engine Interface (Intel MEI) Linux driver
- * Copyright (c) 2003-2012, 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/kernel.h>
-#include <linux/module.h>
-#include <linux/moduleparam.h>
-#include <linux/device.h>
-#include <linux/sched.h>
-#include <linux/watchdog.h>
-
-#include <linux/mei.h>
-
-#include "mei_dev.h"
-#include "hbm.h"
-#include "client.h"
-
-static const u8 mei_start_wd_params[] = { 0x02, 0x12, 0x13, 0x10 };
-static const u8 mei_stop_wd_params[] = { 0x02, 0x02, 0x14, 0x10 };
-
-/*
- * AMT Watchdog Device
- */
-#define INTEL_AMT_WATCHDOG_ID "INTCAMT"
-
-/* UUIDs for AMT F/W clients */
-const uuid_le mei_wd_guid = UUID_LE(0x05B79A6F, 0x4628, 0x4D7F, 0x89,
- 0x9D, 0xA9, 0x15, 0x14, 0xCB,
- 0x32, 0xAB);
-
-static void mei_wd_set_start_timeout(struct mei_device *dev, u16 timeout)
-{
- dev_dbg(dev->dev, "wd: set timeout=%d.\n", timeout);
- memcpy(dev->wd_data, mei_start_wd_params, MEI_WD_HDR_SIZE);
- memcpy(dev->wd_data + MEI_WD_HDR_SIZE, &timeout, sizeof(u16));
-}
-
-/**
- * mei_wd_host_init - connect to the watchdog client
- *
- * @dev: the device structure
- * @me_cl: me client
- *
- * Return: -ENOTTY if wd client cannot be found
- * -EIO if write has failed
- * 0 on success
- */
-int mei_wd_host_init(struct mei_device *dev, struct mei_me_client *me_cl)
-{
- struct mei_cl *cl = &dev->wd_cl;
- int ret;
-
- mei_cl_init(cl, dev);
-
- dev->wd_timeout = MEI_WD_DEFAULT_TIMEOUT;
- dev->wd_state = MEI_WD_IDLE;
-
- ret = mei_cl_link(cl, MEI_WD_HOST_CLIENT_ID);
- if (ret < 0) {
- dev_info(dev->dev, "wd: failed link client\n");
- return ret;
- }
-
- ret = mei_cl_connect(cl, me_cl, NULL);
- if (ret) {
- dev_err(dev->dev, "wd: failed to connect = %d\n", ret);
- mei_cl_unlink(cl);
- return ret;
- }
-
- ret = mei_watchdog_register(dev);
- if (ret) {
- mei_cl_disconnect(cl);
- mei_cl_unlink(cl);
- }
- return ret;
-}
-
-/**
- * mei_wd_send - sends watch dog message to fw.
- *
- * @dev: the device structure
- *
- * Return: 0 if success,
- * -EIO when message send fails
- * -EINVAL when invalid message is to be sent
- * -ENODEV on flow control failure
- */
-int mei_wd_send(struct mei_device *dev)
-{
- struct mei_cl *cl = &dev->wd_cl;
- struct mei_msg_hdr hdr;
- int ret;
-
- hdr.host_addr = cl->host_client_id;
- hdr.me_addr = mei_cl_me_id(cl);
- hdr.msg_complete = 1;
- hdr.reserved = 0;
- hdr.internal = 0;
-
- if (!memcmp(dev->wd_data, mei_start_wd_params, MEI_WD_HDR_SIZE))
- hdr.length = MEI_WD_START_MSG_SIZE;
- else if (!memcmp(dev->wd_data, mei_stop_wd_params, MEI_WD_HDR_SIZE))
- hdr.length = MEI_WD_STOP_MSG_SIZE;
- else {
- dev_err(dev->dev, "wd: invalid message is to be sent, aborting\n");
- return -EINVAL;
- }
-
- ret = mei_write_message(dev, &hdr, dev->wd_data);
- if (ret) {
- dev_err(dev->dev, "wd: write message failed\n");
- return ret;
- }
-
- ret = mei_cl_flow_ctrl_reduce(cl);
- if (ret) {
- dev_err(dev->dev, "wd: flow_ctrl_reduce failed.\n");
- return ret;
- }
-
- return 0;
-}
-
-/**
- * mei_wd_stop - sends watchdog stop message to fw.
- *
- * @dev: the device structure
- *
- * Return: 0 if success
- * on error:
- * -EIO when message send fails
- * -EINVAL when invalid message is to be sent
- * -ETIME on message timeout
- */
-int mei_wd_stop(struct mei_device *dev)
-{
- struct mei_cl *cl = &dev->wd_cl;
- int ret;
-
- if (!mei_cl_is_connected(cl) ||
- dev->wd_state != MEI_WD_RUNNING)
- return 0;
-
- memcpy(dev->wd_data, mei_stop_wd_params, MEI_WD_STOP_MSG_SIZE);
-
- dev->wd_state = MEI_WD_STOPPING;
-
- ret = mei_cl_flow_ctrl_creds(cl);
- if (ret < 0)
- goto err;
-
- if (ret && mei_hbuf_acquire(dev)) {
- ret = mei_wd_send(dev);
- if (ret)
- goto err;
- dev->wd_pending = false;
- } else {
- dev->wd_pending = true;
- }
-
- mutex_unlock(&dev->device_lock);
-
- ret = wait_event_timeout(dev->wait_stop_wd,
- dev->wd_state == MEI_WD_IDLE,
- msecs_to_jiffies(MEI_WD_STOP_TIMEOUT));
- mutex_lock(&dev->device_lock);
- if (dev->wd_state != MEI_WD_IDLE) {
- /* timeout */
- ret = -ETIME;
- dev_warn(dev->dev, "wd: stop failed to complete ret=%d\n", ret);
- goto err;
- }
- dev_dbg(dev->dev, "wd: stop completed after %u msec\n",
- MEI_WD_STOP_TIMEOUT - jiffies_to_msecs(ret));
- return 0;
-err:
- return ret;
-}
-
-/**
- * mei_wd_ops_start - wd start command from the watchdog core.
- *
- * @wd_dev: watchdog device struct
- *
- * Return: 0 if success, negative errno code for failure
- */
-static int mei_wd_ops_start(struct watchdog_device *wd_dev)
-{
- struct mei_device *dev;
- struct mei_cl *cl;
- int err = -ENODEV;
-
- dev = watchdog_get_drvdata(wd_dev);
- if (!dev)
- return -ENODEV;
-
- cl = &dev->wd_cl;
-
- mutex_lock(&dev->device_lock);
-
- if (dev->dev_state != MEI_DEV_ENABLED) {
- dev_dbg(dev->dev, "wd: dev_state != MEI_DEV_ENABLED dev_state = %s\n",
- mei_dev_state_str(dev->dev_state));
- goto end_unlock;
- }
-
- if (!mei_cl_is_connected(cl)) {
- cl_dbg(dev, cl, "MEI Driver is not connected to Watchdog Client\n");
- goto end_unlock;
- }
-
- mei_wd_set_start_timeout(dev, dev->wd_timeout);
-
- err = 0;
-end_unlock:
- mutex_unlock(&dev->device_lock);
- return err;
-}
-
-/**
- * mei_wd_ops_stop - wd stop command from the watchdog core.
- *
- * @wd_dev: watchdog device struct
- *
- * Return: 0 if success, negative errno code for failure
- */
-static int mei_wd_ops_stop(struct watchdog_device *wd_dev)
-{
- struct mei_device *dev;
-
- dev = watchdog_get_drvdata(wd_dev);
- if (!dev)
- return -ENODEV;
-
- mutex_lock(&dev->device_lock);
- mei_wd_stop(dev);
- mutex_unlock(&dev->device_lock);
-
- return 0;
-}
-
-/**
- * mei_wd_ops_ping - wd ping command from the watchdog core.
- *
- * @wd_dev: watchdog device struct
- *
- * Return: 0 if success, negative errno code for failure
- */
-static int mei_wd_ops_ping(struct watchdog_device *wd_dev)
-{
- struct mei_device *dev;
- struct mei_cl *cl;
- int ret;
-
- dev = watchdog_get_drvdata(wd_dev);
- if (!dev)
- return -ENODEV;
-
- cl = &dev->wd_cl;
-
- mutex_lock(&dev->device_lock);
-
- if (!mei_cl_is_connected(cl)) {
- cl_err(dev, cl, "wd: not connected.\n");
- ret = -ENODEV;
- goto end;
- }
-
- dev->wd_state = MEI_WD_RUNNING;
-
- ret = mei_cl_flow_ctrl_creds(cl);
- if (ret < 0)
- goto end;
-
- /* Check if we can send the ping to HW*/
- if (ret && mei_hbuf_acquire(dev)) {
- dev_dbg(dev->dev, "wd: sending ping\n");
-
- ret = mei_wd_send(dev);
- if (ret)
- goto end;
- dev->wd_pending = false;
- } else {
- dev->wd_pending = true;
- }
-
-end:
- mutex_unlock(&dev->device_lock);
- return ret;
-}
-
-/**
- * mei_wd_ops_set_timeout - wd set timeout command from the watchdog core.
- *
- * @wd_dev: watchdog device struct
- * @timeout: timeout value to set
- *
- * Return: 0 if success, negative errno code for failure
- */
-static int mei_wd_ops_set_timeout(struct watchdog_device *wd_dev,
- unsigned int timeout)
-{
- struct mei_device *dev;
-
- dev = watchdog_get_drvdata(wd_dev);
- if (!dev)
- return -ENODEV;
-
- /* Check Timeout value */
- if (timeout < MEI_WD_MIN_TIMEOUT || timeout > MEI_WD_MAX_TIMEOUT)
- return -EINVAL;
-
- mutex_lock(&dev->device_lock);
-
- dev->wd_timeout = timeout;
- wd_dev->timeout = timeout;
- mei_wd_set_start_timeout(dev, dev->wd_timeout);
-
- mutex_unlock(&dev->device_lock);
-
- return 0;
-}
-
-/*
- * Watchdog Device structs
- */
-static const struct watchdog_ops wd_ops = {
- .owner = THIS_MODULE,
- .start = mei_wd_ops_start,
- .stop = mei_wd_ops_stop,
- .ping = mei_wd_ops_ping,
- .set_timeout = mei_wd_ops_set_timeout,
-};
-static const struct watchdog_info wd_info = {
- .identity = INTEL_AMT_WATCHDOG_ID,
- .options = WDIOF_KEEPALIVEPING |
- WDIOF_SETTIMEOUT |
- WDIOF_ALARMONLY,
-};
-
-static struct watchdog_device amt_wd_dev = {
- .info = &wd_info,
- .ops = &wd_ops,
- .timeout = MEI_WD_DEFAULT_TIMEOUT,
- .min_timeout = MEI_WD_MIN_TIMEOUT,
- .max_timeout = MEI_WD_MAX_TIMEOUT,
-};
-
-
-int mei_watchdog_register(struct mei_device *dev)
-{
-
- int ret;
-
- amt_wd_dev.parent = dev->dev;
- /* unlock to perserve correct locking order */
- mutex_unlock(&dev->device_lock);
- ret = watchdog_register_device(&amt_wd_dev);
- mutex_lock(&dev->device_lock);
- if (ret) {
- dev_err(dev->dev, "wd: unable to register watchdog device = %d.\n",
- ret);
- return ret;
- }
-
- dev_dbg(dev->dev, "wd: successfully register watchdog interface.\n");
- watchdog_set_drvdata(&amt_wd_dev, dev);
- return 0;
-}
-
-void mei_watchdog_unregister(struct mei_device *dev)
-{
- if (watchdog_get_drvdata(&amt_wd_dev) == NULL)
- return;
-
- watchdog_set_drvdata(&amt_wd_dev, NULL);
- watchdog_unregister_device(&amt_wd_dev);
-}
-
--
2.4.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* [char-misc-next v3 3/8] watchdog: mei_wdt: implement MEI iAMT watchdog driver
2015-12-21 23:17 [char-misc-next v3 0/8] mei: create proper iAMT watchdog driver Tomas Winkler
2015-12-21 23:17 ` [char-misc-next v3 1/8] mei: drop nfc leftovers from the mei driver Tomas Winkler
2015-12-21 23:17 ` [char-misc-next v3 2/8] mei: wd: drop the watchdog code from the core " Tomas Winkler
@ 2015-12-21 23:17 ` Tomas Winkler
2015-12-22 4:58 ` Guenter Roeck
2015-12-21 23:17 ` [char-misc-next v3 4/8] watchdog: mei_wdt: add status debugfs entry Tomas Winkler
` (4 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Tomas Winkler @ 2015-12-21 23:17 UTC (permalink / raw)
To: Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Alexander Usyskin, Guenter Roeck, linux-watchdog, linux-kernel,
Tomas Winkler
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;
+
+ 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;
+
+ 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;
+
+ 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)
+ 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;
+
+ 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;
+
+ 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;
+
+ 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");
--
2.4.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [char-misc-next v3 3/8] watchdog: mei_wdt: implement MEI iAMT watchdog driver
2015-12-21 23:17 ` [char-misc-next v3 3/8] watchdog: mei_wdt: implement MEI iAMT watchdog driver Tomas Winkler
@ 2015-12-22 4:58 ` Guenter Roeck
2015-12-22 7:19 ` Winkler, Tomas
0 siblings, 1 reply; 19+ messages in thread
From: Guenter Roeck @ 2015-12-22 4:58 UTC (permalink / raw)
To: Tomas Winkler, Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Alexander Usyskin, linux-watchdog, linux-kernel
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");
>
^ permalink raw reply [flat|nested] 19+ messages in thread* RE: [char-misc-next v3 3/8] watchdog: mei_wdt: implement MEI iAMT watchdog driver
2015-12-22 4:58 ` Guenter Roeck
@ 2015-12-22 7:19 ` Winkler, Tomas
2015-12-22 7:40 ` Guenter Roeck
0 siblings, 1 reply; 19+ messages in thread
From: Winkler, Tomas @ 2015-12-22 7:19 UTC (permalink / raw)
To: Guenter Roeck, Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Usyskin, Alexander, linux-watchdog, linux-kernel
>
> 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.
Yes, this can happen and as far I remember it does as we are unregistering the device while the watchdog core is still
holding on wdd. Also I prefer not crashing the driver and the whole kernel with it, this is API should check its parameters.
>
> > + 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");
> >
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [char-misc-next v3 3/8] watchdog: mei_wdt: implement MEI iAMT watchdog driver
2015-12-22 7:19 ` Winkler, Tomas
@ 2015-12-22 7:40 ` Guenter Roeck
2015-12-23 22:38 ` Winkler, Tomas
0 siblings, 1 reply; 19+ messages in thread
From: Guenter Roeck @ 2015-12-22 7:40 UTC (permalink / raw)
To: Winkler, Tomas, Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Usyskin, Alexander, linux-watchdog, linux-kernel
On 12/21/2015 11:19 PM, Winkler, Tomas wrote:
>
>>
>> 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.
>
> Yes, this can happen and as far I remember it does as we are unregistering the device while the watchdog core is still
> holding on wdd. Also I prefer not crashing the driver and the whole kernel with it, this is API should check its parameters.
>
After unregistering, the watchdog core doesn't call those functions anymore.
It could have happened earlier when you were manipulating the internal status
flags, but it should not happen anymore. The watchdog core must not call the
ops functions after the device was unregistered.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 19+ messages in thread* RE: [char-misc-next v3 3/8] watchdog: mei_wdt: implement MEI iAMT watchdog driver
2015-12-22 7:40 ` Guenter Roeck
@ 2015-12-23 22:38 ` Winkler, Tomas
2015-12-24 0:23 ` Guenter Roeck
0 siblings, 1 reply; 19+ messages in thread
From: Winkler, Tomas @ 2015-12-23 22:38 UTC (permalink / raw)
To: Guenter Roeck, Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Usyskin, Alexander, linux-watchdog, linux-kernel
>
> On 12/21/2015 11:19 PM, Winkler, Tomas wrote:
> >
> >>
> >> 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.
> >
> > Yes, this can happen and as far I remember it does as we are unregistering the
> device while the watchdog core is still
> > holding on wdd. Also I prefer not crashing the driver and the whole kernel with
> it, this is API should check its parameters.
> >
> After unregistering, the watchdog core doesn't call those functions anymore.
> It could have happened earlier when you were manipulating the internal status
> flags, but it should not happen anymore. The watchdog core must not call the
> ops functions after the device was unregistered.
I agree the code logic seems to function like what you've describing but I've positively hit that condition in some stress test, now I'm not sure in what code version so I will try to get that again for the current one.
Thanks
Tomas
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [char-misc-next v3 3/8] watchdog: mei_wdt: implement MEI iAMT watchdog driver
2015-12-23 22:38 ` Winkler, Tomas
@ 2015-12-24 0:23 ` Guenter Roeck
0 siblings, 0 replies; 19+ messages in thread
From: Guenter Roeck @ 2015-12-24 0:23 UTC (permalink / raw)
To: Winkler, Tomas, Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Usyskin, Alexander, linux-watchdog, linux-kernel
On 12/23/2015 02:38 PM, Winkler, Tomas wrote:
>>
>> On 12/21/2015 11:19 PM, Winkler, Tomas wrote:
>>>
>>>>
>>>> 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.
>>>
>>> Yes, this can happen and as far I remember it does as we are unregistering the
>> device while the watchdog core is still
>>> holding on wdd. Also I prefer not crashing the driver and the whole kernel with
>> it, this is API should check its parameters.
>>>
>> After unregistering, the watchdog core doesn't call those functions anymore.
>> It could have happened earlier when you were manipulating the internal status
>> flags, but it should not happen anymore. The watchdog core must not call the
>> ops functions after the device was unregistered.
>
> I agree the code logic seems to function like what you've describing but I've positively hit that condition in some stress test, now I'm not sure in what code version so I will try to get that again for the current one.
That would be great. I am concerned that, if the situation is seen, we would
paint over some bug, maybe even in the infrastructure. If so we'll really
need to understand how to get there, since other drivers could be affected
as well.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 19+ messages in thread
* [char-misc-next v3 4/8] watchdog: mei_wdt: add status debugfs entry
2015-12-21 23:17 [char-misc-next v3 0/8] mei: create proper iAMT watchdog driver Tomas Winkler
` (2 preceding siblings ...)
2015-12-21 23:17 ` [char-misc-next v3 3/8] watchdog: mei_wdt: implement MEI iAMT watchdog driver Tomas Winkler
@ 2015-12-21 23:17 ` Tomas Winkler
2015-12-22 5:30 ` Guenter Roeck
2015-12-21 23:17 ` [char-misc-next v3 5/8] mei: bus: whitelist the watchdog client Tomas Winkler
` (3 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Tomas Winkler @ 2015-12-21 23:17 UTC (permalink / raw)
To: Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Alexander Usyskin, Guenter Roeck, linux-watchdog, linux-kernel,
Tomas Winkler
Add entry for dumping current watchdog internal state
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2: new in the series
V3: rebase
drivers/watchdog/mei_wdt.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 88 insertions(+)
diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
index 5b28a1e95ac1..ab9aec218d69 100644
--- a/drivers/watchdog/mei_wdt.c
+++ b/drivers/watchdog/mei_wdt.c
@@ -15,6 +15,7 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/interrupt.h>
+#include <linux/debugfs.h>
#include <linux/watchdog.h>
#include <linux/uuid.h>
@@ -54,6 +55,24 @@ enum mei_wdt_state {
MEI_WDT_STOPPING,
};
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+static const char *mei_wdt_state_str(enum mei_wdt_state state)
+{
+ switch (state) {
+ case MEI_WDT_IDLE:
+ return "IDLE";
+ case MEI_WDT_START:
+ return "START";
+ case MEI_WDT_RUNNING:
+ return "RUNNING";
+ case MEI_WDT_STOPPING:
+ return "STOPPING";
+ default:
+ return "unknown";
+ }
+}
+#endif /* CONFIG_DEBUG_FS */
+
struct mei_wdt;
/**
@@ -76,6 +95,8 @@ struct mei_wdt_dev {
* @cldev: mei watchdog client device
* @state: watchdog internal state
* @timeout: watchdog current timeout
+ *
+ * @dbgfs_dir: debugfs dir entry
*/
struct mei_wdt {
struct mei_wdt_dev *mwd;
@@ -83,6 +104,10 @@ struct mei_wdt {
struct mei_cl_device *cldev;
enum mei_wdt_state state;
u16 timeout;
+
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+ struct dentry *dbgfs_dir;
+#endif /* CONFIG_DEBUG_FS */
};
/*
@@ -387,6 +412,65 @@ static int mei_wdt_register(struct mei_wdt *wdt)
return 0;
}
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+
+static ssize_t mei_dbgfs_read_state(struct file *file, char __user *ubuf,
+ size_t cnt, loff_t *ppos)
+{
+ struct mei_wdt *wdt = file->private_data;
+ const size_t bufsz = 32;
+ char buf[32];
+ ssize_t pos = 0;
+
+ pos += scnprintf(buf + pos, bufsz - pos, "state: %s\n",
+ mei_wdt_state_str(wdt->state));
+
+ return simple_read_from_buffer(ubuf, cnt, ppos, buf, pos);
+}
+
+static const struct file_operations dbgfs_fops_state = {
+ .open = simple_open,
+ .read = mei_dbgfs_read_state,
+ .llseek = generic_file_llseek,
+};
+
+static void dbgfs_unregister(struct mei_wdt *wdt)
+{
+ if (!wdt->dbgfs_dir)
+ return;
+ debugfs_remove_recursive(wdt->dbgfs_dir);
+ wdt->dbgfs_dir = NULL;
+}
+
+static int dbgfs_register(struct mei_wdt *wdt)
+{
+ struct dentry *dir, *f;
+
+ dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
+ if (!dir)
+ return -ENOMEM;
+
+ wdt->dbgfs_dir = dir;
+ f = debugfs_create_file("state", S_IRUSR, dir, wdt, &dbgfs_fops_state);
+ if (!f)
+ goto err;
+
+ return 0;
+err:
+ dbgfs_unregister(wdt);
+ return -ENODEV;
+}
+
+#else
+
+static inline void dbgfs_unregister(struct mei_wdt *wdt) {}
+
+static inline int dbgfs_register(struct mei_wdt *wdt)
+{
+ return 0;
+}
+#endif /* CONFIG_DEBUG_FS */
+
static int mei_wdt_probe(struct mei_cl_device *cldev,
const struct mei_cl_device_id *id)
{
@@ -414,6 +498,8 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
if (ret)
goto err_disable;
+ dbgfs_register(wdt);
+
return 0;
err_disable:
@@ -433,6 +519,8 @@ static int mei_wdt_remove(struct mei_cl_device *cldev)
mei_cldev_disable(cldev);
+ dbgfs_unregister(wdt);
+
kfree(wdt);
return 0;
--
2.4.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [char-misc-next v3 4/8] watchdog: mei_wdt: add status debugfs entry
2015-12-21 23:17 ` [char-misc-next v3 4/8] watchdog: mei_wdt: add status debugfs entry Tomas Winkler
@ 2015-12-22 5:30 ` Guenter Roeck
2015-12-23 22:48 ` Winkler, Tomas
0 siblings, 1 reply; 19+ messages in thread
From: Guenter Roeck @ 2015-12-22 5:30 UTC (permalink / raw)
To: Tomas Winkler, Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Alexander Usyskin, linux-watchdog, linux-kernel
On 12/21/2015 03:17 PM, Tomas Winkler wrote:
> Add entry for dumping current watchdog internal state
>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
> V2: new in the series
> V3: rebase
> drivers/watchdog/mei_wdt.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 88 insertions(+)
>
> diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
> index 5b28a1e95ac1..ab9aec218d69 100644
> --- a/drivers/watchdog/mei_wdt.c
> +++ b/drivers/watchdog/mei_wdt.c
> @@ -15,6 +15,7 @@
> #include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/interrupt.h>
> +#include <linux/debugfs.h>
> #include <linux/watchdog.h>
>
> #include <linux/uuid.h>
> @@ -54,6 +55,24 @@ enum mei_wdt_state {
> MEI_WDT_STOPPING,
> };
>
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +static const char *mei_wdt_state_str(enum mei_wdt_state state)
> +{
> + switch (state) {
> + case MEI_WDT_IDLE:
> + return "IDLE";
> + case MEI_WDT_START:
> + return "START";
> + case MEI_WDT_RUNNING:
> + return "RUNNING";
> + case MEI_WDT_STOPPING:
> + return "STOPPING";
> + default:
> + return "unknown";
> + }
> +}
> +#endif /* CONFIG_DEBUG_FS */
> +
I still don't understand why this code has to be here instead of
further below (at <----> mark).
> struct mei_wdt;
>
> /**
> @@ -76,6 +95,8 @@ struct mei_wdt_dev {
> * @cldev: mei watchdog client device
> * @state: watchdog internal state
> * @timeout: watchdog current timeout
> + *
> + * @dbgfs_dir: debugfs dir entry
> */
> struct mei_wdt {
> struct mei_wdt_dev *mwd;
> @@ -83,6 +104,10 @@ struct mei_wdt {
> struct mei_cl_device *cldev;
> enum mei_wdt_state state;
> u16 timeout;
> +
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> + struct dentry *dbgfs_dir;
> +#endif /* CONFIG_DEBUG_FS */
> };
>
> /*
> @@ -387,6 +412,65 @@ static int mei_wdt_register(struct mei_wdt *wdt)
> return 0;
> }
>
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +
<---->
> +static ssize_t mei_dbgfs_read_state(struct file *file, char __user *ubuf,
> + size_t cnt, loff_t *ppos)
> +{
> + struct mei_wdt *wdt = file->private_data;
> + const size_t bufsz = 32;
> + char buf[32];
> + ssize_t pos = 0;
> +
> + pos += scnprintf(buf + pos, bufsz - pos, "state: %s\n",
> + mei_wdt_state_str(wdt->state));
> +
Seems to me that "pos = ..." would accomplish exactly the same
without having to pre-initialize pos. I also don't understand the use of
"+ pos" and "- pos" in the parameter field. pos is 0, isn't it ?
When would it ever be non-0 ?
pos = scnprintf(buf, sizeof(buf), "state: %s\n", mei_wdt_state_str(wdt->state));
What am I missing here ?
> + return simple_read_from_buffer(ubuf, cnt, ppos, buf, pos);
> +}
> +
> +static const struct file_operations dbgfs_fops_state = {
> + .open = simple_open,
> + .read = mei_dbgfs_read_state,
> + .llseek = generic_file_llseek,
> +};
> +
> +static void dbgfs_unregister(struct mei_wdt *wdt)
> +{
> + if (!wdt->dbgfs_dir)
> + return;
> + debugfs_remove_recursive(wdt->dbgfs_dir);
debugfs_remove_recursive() checks if the parameter is NULL,
so it is not necessary to check if it is NULL before the call.
> + wdt->dbgfs_dir = NULL;
> +}
> +
> +static int dbgfs_register(struct mei_wdt *wdt)
> +{
> + struct dentry *dir, *f;
> +
> + dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
> + if (!dir)
> + return -ENOMEM;
> +
> + wdt->dbgfs_dir = dir;
> + f = debugfs_create_file("state", S_IRUSR, dir, wdt, &dbgfs_fops_state);
> + if (!f)
> + goto err;
> +
> + return 0;
> +err:
> + dbgfs_unregister(wdt);
> + return -ENODEV;
The error value is ignored by the caller - why bother returning an error in the first place ?
> +}
> +
> +#else
> +
> +static inline void dbgfs_unregister(struct mei_wdt *wdt) {}
> +
> +static inline int dbgfs_register(struct mei_wdt *wdt)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_DEBUG_FS */
> +
> static int mei_wdt_probe(struct mei_cl_device *cldev,
> const struct mei_cl_device_id *id)
> {
> @@ -414,6 +498,8 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
> if (ret)
> goto err_disable;
>
> + dbgfs_register(wdt);
> +
> return 0;
>
> err_disable:
> @@ -433,6 +519,8 @@ static int mei_wdt_remove(struct mei_cl_device *cldev)
>
> mei_cldev_disable(cldev);
>
> + dbgfs_unregister(wdt);
> +
> kfree(wdt);
>
> return 0;
>
^ permalink raw reply [flat|nested] 19+ messages in thread* RE: [char-misc-next v3 4/8] watchdog: mei_wdt: add status debugfs entry
2015-12-22 5:30 ` Guenter Roeck
@ 2015-12-23 22:48 ` Winkler, Tomas
2015-12-24 0:25 ` Guenter Roeck
0 siblings, 1 reply; 19+ messages in thread
From: Winkler, Tomas @ 2015-12-23 22:48 UTC (permalink / raw)
To: Guenter Roeck, Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Usyskin, Alexander, linux-watchdog, linux-kernel
>
> On 12/21/2015 03:17 PM, Tomas Winkler wrote:
> > Add entry for dumping current watchdog internal state
> >
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> > V2: new in the series
> > V3: rebase
> > drivers/watchdog/mei_wdt.c | 88
> ++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 88 insertions(+)
> >
> > diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
> > index 5b28a1e95ac1..ab9aec218d69 100644
> > --- a/drivers/watchdog/mei_wdt.c
> > +++ b/drivers/watchdog/mei_wdt.c
> > @@ -15,6 +15,7 @@
> > #include <linux/module.h>
> > #include <linux/slab.h>
> > #include <linux/interrupt.h>
> > +#include <linux/debugfs.h>
> > #include <linux/watchdog.h>
> >
> > #include <linux/uuid.h>
> > @@ -54,6 +55,24 @@ enum mei_wdt_state {
> > MEI_WDT_STOPPING,
> > };
> >
> > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> > +static const char *mei_wdt_state_str(enum mei_wdt_state state)
> > +{
> > + switch (state) {
> > + case MEI_WDT_IDLE:
> > + return "IDLE";
> > + case MEI_WDT_START:
> > + return "START";
> > + case MEI_WDT_RUNNING:
> > + return "RUNNING";
> > + case MEI_WDT_STOPPING:
> > + return "STOPPING";
> > + default:
> > + return "unknown";
> > + }
> > +}
> > +#endif /* CONFIG_DEBUG_FS */
> > +
> I still don't understand why this code has to be here instead of
> further below (at <----> mark).
Once it follow closely after enum definition, second in the next patch the
Ifdef is removed since we use the function in debug output and not only in debugfs.
>
> > struct mei_wdt;
> >
> > /**
> > @@ -76,6 +95,8 @@ struct mei_wdt_dev {
> > * @cldev: mei watchdog client device
> > * @state: watchdog internal state
> > * @timeout: watchdog current timeout
> > + *
> > + * @dbgfs_dir: debugfs dir entry
> > */
> > struct mei_wdt {
> > struct mei_wdt_dev *mwd;
> > @@ -83,6 +104,10 @@ struct mei_wdt {
> > struct mei_cl_device *cldev;
> > enum mei_wdt_state state;
> > u16 timeout;
> > +
> > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> > + struct dentry *dbgfs_dir;
> > +#endif /* CONFIG_DEBUG_FS */
> > };
> >
> > /*
> > @@ -387,6 +412,65 @@ static int mei_wdt_register(struct mei_wdt *wdt)
> > return 0;
> > }
> >
> > +#if IS_ENABLED(CONFIG_DEBUG_FS)
> > +
>
> <---->
>
> > +static ssize_t mei_dbgfs_read_state(struct file *file, char __user *ubuf,
> > + size_t cnt, loff_t *ppos)
> > +{
> > + struct mei_wdt *wdt = file->private_data;
> > + const size_t bufsz = 32;
> > + char buf[32];
> > + ssize_t pos = 0;
> > +
> > + pos += scnprintf(buf + pos, bufsz - pos, "state: %s\n",
> > + mei_wdt_state_str(wdt->state));
> > +
> Seems to me that "pos = ..." would accomplish exactly the same
> without having to pre-initialize pos. I also don't understand the use of
> "+ pos" and "- pos" in the parameter field. pos is 0, isn't it ?
> When would it ever be non-0 ?
>
> pos = scnprintf(buf, sizeof(buf), "state: %s\n", mei_wdt_state_str(wdt-
> >state));
>
> What am I missing here ?
Not you are not missing anything, it's just an idiom taken from all my debugfs function with multiline output.
>
> > + return simple_read_from_buffer(ubuf, cnt, ppos, buf, pos);
> > +}
> > +
> > +static const struct file_operations dbgfs_fops_state = {
> > + .open = simple_open,
> > + .read = mei_dbgfs_read_state,
> > + .llseek = generic_file_llseek,
> > +};
> > +
> > +static void dbgfs_unregister(struct mei_wdt *wdt)
> > +{
> > + if (!wdt->dbgfs_dir)
> > + return;
> > + debugfs_remove_recursive(wdt->dbgfs_dir);
>
> debugfs_remove_recursive() checks if the parameter is NULL,
> so it is not necessary to check if it is NULL before the call.
Correct, I can be fixed.
>
> > + wdt->dbgfs_dir = NULL;
> > +}
> > +
> > +static int dbgfs_register(struct mei_wdt *wdt)
> > +{
> > + struct dentry *dir, *f;
> > +
> > + dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
> > + if (!dir)
> > + return -ENOMEM;
> > +
> > + wdt->dbgfs_dir = dir;
> > + f = debugfs_create_file("state", S_IRUSR, dir, wdt, &dbgfs_fops_state);
> > + if (!f)
> > + goto err;
> > +
> > + return 0;
> > +err:
> > + dbgfs_unregister(wdt);
> > + return -ENODEV;
>
> The error value is ignored by the caller - why bother returning an error in the first
> place ?
A function doesn't take responsibility on how it used.
>
> > +}
> > +
> > +#else
> > +
> > +static inline void dbgfs_unregister(struct mei_wdt *wdt) {}
> > +
> > +static inline int dbgfs_register(struct mei_wdt *wdt)
> > +{
> > + return 0;
> > +}
> > +#endif /* CONFIG_DEBUG_FS */
> > +
> > static int mei_wdt_probe(struct mei_cl_device *cldev,
> > const struct mei_cl_device_id *id)
> > {
> > @@ -414,6 +498,8 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
> > if (ret)
> > goto err_disable;
> >
> > + dbgfs_register(wdt);
> > +
> > return 0;
> >
> > err_disable:
> > @@ -433,6 +519,8 @@ static int mei_wdt_remove(struct mei_cl_device *cldev)
> >
> > mei_cldev_disable(cldev);
> >
> > + dbgfs_unregister(wdt);
> > +
> > kfree(wdt);
> >
> > return 0;
> >
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [char-misc-next v3 4/8] watchdog: mei_wdt: add status debugfs entry
2015-12-23 22:48 ` Winkler, Tomas
@ 2015-12-24 0:25 ` Guenter Roeck
0 siblings, 0 replies; 19+ messages in thread
From: Guenter Roeck @ 2015-12-24 0:25 UTC (permalink / raw)
To: Winkler, Tomas, Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Usyskin, Alexander, linux-watchdog, linux-kernel
On 12/23/2015 02:48 PM, Winkler, Tomas wrote:
>>
>> On 12/21/2015 03:17 PM, Tomas Winkler wrote:
>>> Add entry for dumping current watchdog internal state
>>>
>>> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
>>> ---
>>> V2: new in the series
>>> V3: rebase
>>> drivers/watchdog/mei_wdt.c | 88
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 88 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
>>> index 5b28a1e95ac1..ab9aec218d69 100644
>>> --- a/drivers/watchdog/mei_wdt.c
>>> +++ b/drivers/watchdog/mei_wdt.c
>>> @@ -15,6 +15,7 @@
>>> #include <linux/module.h>
>>> #include <linux/slab.h>
>>> #include <linux/interrupt.h>
>>> +#include <linux/debugfs.h>
>>> #include <linux/watchdog.h>
>>>
>>> #include <linux/uuid.h>
>>> @@ -54,6 +55,24 @@ enum mei_wdt_state {
>>> MEI_WDT_STOPPING,
>>> };
>>>
>>> +#if IS_ENABLED(CONFIG_DEBUG_FS)
>>> +static const char *mei_wdt_state_str(enum mei_wdt_state state)
>>> +{
>>> + switch (state) {
>>> + case MEI_WDT_IDLE:
>>> + return "IDLE";
>>> + case MEI_WDT_START:
>>> + return "START";
>>> + case MEI_WDT_RUNNING:
>>> + return "RUNNING";
>>> + case MEI_WDT_STOPPING:
>>> + return "STOPPING";
>>> + default:
>>> + return "unknown";
>>> + }
>>> +}
>>> +#endif /* CONFIG_DEBUG_FS */
>>> +
>> I still don't understand why this code has to be here instead of
>> further below (at <----> mark).
> Once it follow closely after enum definition, second in the next patch the
> Ifdef is removed since we use the function in debug output and not only in debugfs.
>
>>
>>> struct mei_wdt;
>>>
>>> /**
>>> @@ -76,6 +95,8 @@ struct mei_wdt_dev {
>>> * @cldev: mei watchdog client device
>>> * @state: watchdog internal state
>>> * @timeout: watchdog current timeout
>>> + *
>>> + * @dbgfs_dir: debugfs dir entry
>>> */
>>> struct mei_wdt {
>>> struct mei_wdt_dev *mwd;
>>> @@ -83,6 +104,10 @@ struct mei_wdt {
>>> struct mei_cl_device *cldev;
>>> enum mei_wdt_state state;
>>> u16 timeout;
>>> +
>>> +#if IS_ENABLED(CONFIG_DEBUG_FS)
>>> + struct dentry *dbgfs_dir;
>>> +#endif /* CONFIG_DEBUG_FS */
>>> };
>>>
>>> /*
>>> @@ -387,6 +412,65 @@ static int mei_wdt_register(struct mei_wdt *wdt)
>>> return 0;
>>> }
>>>
>>> +#if IS_ENABLED(CONFIG_DEBUG_FS)
>>> +
>>
>> <---->
>>
>>> +static ssize_t mei_dbgfs_read_state(struct file *file, char __user *ubuf,
>>> + size_t cnt, loff_t *ppos)
>>> +{
>>> + struct mei_wdt *wdt = file->private_data;
>>> + const size_t bufsz = 32;
>>> + char buf[32];
>>> + ssize_t pos = 0;
>>> +
>>> + pos += scnprintf(buf + pos, bufsz - pos, "state: %s\n",
>>> + mei_wdt_state_str(wdt->state));
>>> +
>> Seems to me that "pos = ..." would accomplish exactly the same
>> without having to pre-initialize pos. I also don't understand the use of
>> "+ pos" and "- pos" in the parameter field. pos is 0, isn't it ?
>> When would it ever be non-0 ?
>>
>> pos = scnprintf(buf, sizeof(buf), "state: %s\n", mei_wdt_state_str(wdt-
>>> state));
>>
>> What am I missing here ?
> Not you are not missing anything, it's just an idiom taken from all my debugfs function with multiline output.
I don't think that is a good reason for using the more complex code here.
>>
>>> + return simple_read_from_buffer(ubuf, cnt, ppos, buf, pos);
>>> +}
>>> +
>>> +static const struct file_operations dbgfs_fops_state = {
>>> + .open = simple_open,
>>> + .read = mei_dbgfs_read_state,
>>> + .llseek = generic_file_llseek,
>>> +};
>>> +
>>> +static void dbgfs_unregister(struct mei_wdt *wdt)
>>> +{
>>> + if (!wdt->dbgfs_dir)
>>> + return;
>>> + debugfs_remove_recursive(wdt->dbgfs_dir);
>>
>> debugfs_remove_recursive() checks if the parameter is NULL,
>> so it is not necessary to check if it is NULL before the call.
> Correct, I can be fixed.
>>
>>> + wdt->dbgfs_dir = NULL;
>>> +}
>>> +
>>> +static int dbgfs_register(struct mei_wdt *wdt)
>>> +{
>>> + struct dentry *dir, *f;
>>> +
>>> + dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
>>> + if (!dir)
>>> + return -ENOMEM;
>>> +
>>> + wdt->dbgfs_dir = dir;
>>> + f = debugfs_create_file("state", S_IRUSR, dir, wdt, &dbgfs_fops_state);
>>> + if (!f)
>>> + goto err;
>>> +
>>> + return 0;
>>> +err:
>>> + dbgfs_unregister(wdt);
>>> + return -ENODEV;
>>
>> The error value is ignored by the caller - why bother returning an error in the first
>> place ?
> A function doesn't take responsibility on how it used.
For an exported function I would agree, but not in a static function.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 19+ messages in thread
* [char-misc-next v3 5/8] mei: bus: whitelist the watchdog client
2015-12-21 23:17 [char-misc-next v3 0/8] mei: create proper iAMT watchdog driver Tomas Winkler
` (3 preceding siblings ...)
2015-12-21 23:17 ` [char-misc-next v3 4/8] watchdog: mei_wdt: add status debugfs entry Tomas Winkler
@ 2015-12-21 23:17 ` Tomas Winkler
2015-12-21 23:17 ` [char-misc-next v3 6/8] watchdog: mei_wdt: register wd device only if required Tomas Winkler
` (2 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Tomas Winkler @ 2015-12-21 23:17 UTC (permalink / raw)
To: Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Alexander Usyskin, Guenter Roeck, linux-watchdog, linux-kernel,
Tomas Winkler
The iAMT WD client has to be whitelisted sice it has two connections
and is filtered out by number_of_connections fixup.
Also the API has changed for BDW and SKL but firmware haven't updated
the protocol version.
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
V2/V3: resend
drivers/misc/mei/bus-fixup.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/drivers/misc/mei/bus-fixup.c b/drivers/misc/mei/bus-fixup.c
index b2d2a6ea576c..b87323f4bb14 100644
--- a/drivers/misc/mei/bus-fixup.c
+++ b/drivers/misc/mei/bus-fixup.c
@@ -35,6 +35,9 @@ static const uuid_le mei_nfc_info_guid = MEI_UUID_NFC_INFO;
#define MEI_UUID_NFC_HCI UUID_LE(0x0bb17a78, 0x2a8e, 0x4c50, \
0x94, 0xd4, 0x50, 0x26, 0x67, 0x23, 0x77, 0x5c)
+#define MEI_UUID_WD UUID_LE(0x05B79A6F, 0x4628, 0x4D7F, \
+ 0x89, 0x9D, 0xA9, 0x15, 0x14, 0xCB, 0x32, 0xAB)
+
#define MEI_UUID_ANY NULL_UUID_LE
/**
@@ -66,6 +69,31 @@ static void blacklist(struct mei_cl_device *cldev)
cldev->do_match = 0;
}
+/**
+ * mei_wd - wd client on the bus, change protocol version
+ * as the API has changed.
+ *
+ * @cldev: me clients device
+ */
+#if IS_ENABLED(CONFIG_INTEL_MEI_ME)
+#include <linux/pci.h>
+#include "hw-me-regs.h"
+static void mei_wd(struct mei_cl_device *cldev)
+{
+ struct pci_dev *pdev = to_pci_dev(cldev->dev.parent);
+
+ dev_dbg(&cldev->dev, "running hook %s\n", __func__);
+ if (pdev->device == MEI_DEV_ID_WPT_LP ||
+ pdev->device == MEI_DEV_ID_SPT ||
+ pdev->device == MEI_DEV_ID_SPT_H)
+ cldev->me_cl->props.protocol_version = 0x2;
+
+ cldev->do_match = 1;
+}
+#else
+static inline void mei_wd(struct mei_cl_device *cldev) {}
+#endif /* CONFIG_INTEL_MEI_ME */
+
struct mei_nfc_cmd {
u8 command;
u8 status;
@@ -280,6 +308,7 @@ static struct mei_fixup {
MEI_FIXUP(MEI_UUID_ANY, number_of_connections),
MEI_FIXUP(MEI_UUID_NFC_INFO, blacklist),
MEI_FIXUP(MEI_UUID_NFC_HCI, mei_nfc),
+ MEI_FIXUP(MEI_UUID_WD, mei_wd),
};
/**
--
2.4.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* [char-misc-next v3 6/8] watchdog: mei_wdt: register wd device only if required
2015-12-21 23:17 [char-misc-next v3 0/8] mei: create proper iAMT watchdog driver Tomas Winkler
` (4 preceding siblings ...)
2015-12-21 23:17 ` [char-misc-next v3 5/8] mei: bus: whitelist the watchdog client Tomas Winkler
@ 2015-12-21 23:17 ` Tomas Winkler
2015-12-22 5:18 ` Guenter Roeck
2015-12-21 23:18 ` [char-misc-next v3 7/8] watchdog: mei_wdt: add activation debugfs entry Tomas Winkler
2015-12-21 23:18 ` [char-misc-next v3 8/8] watchdog: mei_wdt: re-register device on event Tomas Winkler
7 siblings, 1 reply; 19+ messages in thread
From: Tomas Winkler @ 2015-12-21 23:17 UTC (permalink / raw)
To: Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Alexander Usyskin, Guenter Roeck, linux-watchdog, linux-kernel,
Tomas Winkler
From: Alexander Usyskin <alexander.usyskin@intel.com>
For Intel Broadwell and newer platforms, the ME device can inform
the host whether the watchdog functionality is activated or not.
If the watchdog functionality is not activated then the watchdog interface
can be not registered and eliminate unnecessary pings and hence lower the
power consumption by avoiding waking up the device.
The feature can be deactivated also without reboot
in that case the watchdog device should be unregistered at runtime.
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2: rework unregistration
V3: rebase; implement unregistraion also at runtime
drivers/watchdog/mei_wdt.c | 183 +++++++++++++++++++++++++++++++++++++++++----
1 file changed, 169 insertions(+), 14 deletions(-)
diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
index ab9aec218d69..3cd80aa75db1 100644
--- a/drivers/watchdog/mei_wdt.c
+++ b/drivers/watchdog/mei_wdt.c
@@ -16,6 +16,7 @@
#include <linux/slab.h>
#include <linux/interrupt.h>
#include <linux/debugfs.h>
+#include <linux/completion.h>
#include <linux/watchdog.h>
#include <linux/uuid.h>
@@ -38,24 +39,30 @@
/* Sub Commands */
#define MEI_MC_START_WD_TIMER_REQ 0x13
+#define MEI_MC_START_WD_TIMER_RES 0x83
+#define MEI_WDT_STATUS_SUCCESS 0
+#define MEI_WDT_WDSTATE_NOT_REQUIRED 0x1
#define MEI_MC_STOP_WD_TIMER_REQ 0x14
/**
* enum mei_wdt_state - internal watchdog state
*
+ * @MEI_WDT_PROBE: wd in probing stage
* @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
+ * @MEI_WDT_NOT_REQUIRED: wd device is not required
*/
enum mei_wdt_state {
+ MEI_WDT_PROBE,
MEI_WDT_IDLE,
MEI_WDT_START,
MEI_WDT_RUNNING,
MEI_WDT_STOPPING,
+ MEI_WDT_NOT_REQUIRED,
};
-#if IS_ENABLED(CONFIG_DEBUG_FS)
static const char *mei_wdt_state_str(enum mei_wdt_state state)
{
switch (state) {
@@ -71,7 +78,6 @@ static const char *mei_wdt_state_str(enum mei_wdt_state state)
return "unknown";
}
}
-#endif /* CONFIG_DEBUG_FS */
struct mei_wdt;
@@ -94,6 +100,10 @@ struct mei_wdt_dev {
*
* @cldev: mei watchdog client device
* @state: watchdog internal state
+ * @resp_required: ping required response
+ * @response: ping response completion
+ * @unregister: unregister worker
+ * @reg_lock: watchdog device registration lock
* @timeout: watchdog current timeout
*
* @dbgfs_dir: debugfs dir entry
@@ -103,6 +113,10 @@ struct mei_wdt {
struct mei_cl_device *cldev;
enum mei_wdt_state state;
+ bool resp_required;
+ struct completion response;
+ struct work_struct unregister;
+ struct mutex reg_lock;
u16 timeout;
#if IS_ENABLED(CONFIG_DEBUG_FS)
@@ -139,6 +153,19 @@ struct mei_wdt_start_request {
} __packed;
/**
+ * struct mei_wdt_start_response watchdog start/ping response
+ *
+ * @hdr: Management Control Command Header
+ * @status: operation status
+ * @wdstate: watchdog status bit mask
+ */
+struct mei_wdt_start_response {
+ struct mei_mc_hdr hdr;
+ u8 status;
+ u8 wdstate;
+} __packed;
+
+/**
* struct mei_wdt_stop_request - watchdog stop
*
* @hdr: Management Control Command Header
@@ -277,13 +304,18 @@ static int mei_wdt_ops_ping(struct watchdog_device *wdd)
if (wdt->state != MEI_WDT_START && wdt->state != MEI_WDT_RUNNING)
return 0;
+ if (wdt->resp_required)
+ init_completion(&wdt->response);
+
+ wdt->state = MEI_WDT_RUNNING;
ret = mei_wdt_ping(wdt);
if (ret)
return ret;
- wdt->state = MEI_WDT_RUNNING;
+ if (wdt->resp_required)
+ ret = wait_for_completion_killable(&wdt->response);
- return 0;
+ return ret;
}
/**
@@ -358,15 +390,22 @@ static struct watchdog_info wd_info = {
*/
static void mei_wdt_unregister(struct mei_wdt *wdt)
{
- struct mei_wdt_dev *mwd = wdt->mwd;
+ struct mei_wdt_dev *mwd;
- if (!mwd)
- return;
+ mutex_lock(&wdt->reg_lock);
+
+ if (!wdt->mwd)
+ goto out;
+
+ mwd = wdt->mwd;
watchdog_unregister_device(&mwd->wdd);
+
wdt->mwd = NULL;
- wdt->state = MEI_WDT_IDLE;
kref_put(&mwd->refcnt, mei_wdt_release);
+
+out:
+ mutex_unlock(&wdt->reg_lock);
}
/**
@@ -387,9 +426,13 @@ static int mei_wdt_register(struct mei_wdt *wdt)
dev = &wdt->cldev->dev;
+ mutex_lock(&wdt->reg_lock);
+
mwd = kzalloc(sizeof(struct mei_wdt_dev), GFP_KERNEL);
- if (!mwd)
- return -ENOMEM;
+ if (!mwd) {
+ ret = -ENOMEM;
+ goto out;
+ }
mwd->wdt = wdt;
mwd->wdd.info = &wd_info;
@@ -405,13 +448,104 @@ static int mei_wdt_register(struct mei_wdt *wdt)
if (ret) {
dev_err(dev, "unable to register watchdog device = %d.\n", ret);
kref_put(&mwd->refcnt, mei_wdt_release);
- return ret;
+ goto out;
}
wdt->mwd = mwd;
+out:
+ mutex_unlock(&wdt->reg_lock);
return 0;
}
+static void mei_wdt_unregister_work(struct work_struct *work)
+{
+ struct mei_wdt *wdt = container_of(work, struct mei_wdt, unregister);
+
+ mei_wdt_unregister(wdt);
+}
+
+/**
+ * mei_wdt_event_rx - callback for data receive
+ *
+ * @cldev: bus device
+ */
+static void mei_wdt_event_rx(struct mei_cl_device *cldev)
+{
+ struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
+ struct mei_wdt_start_response res;
+ const size_t res_len = sizeof(res);
+ int ret;
+
+ ret = mei_cldev_recv(wdt->cldev, (u8 *)&res, res_len);
+ if (ret < 0) {
+ dev_err(&cldev->dev, "failure in recv %d\n", ret);
+ return;
+ }
+
+ /* Empty response can be sent on stop */
+ if (ret == 0)
+ return;
+
+ if (ret < sizeof(struct mei_mc_hdr)) {
+ dev_err(&cldev->dev, "recv small data %d\n", ret);
+ return;
+ }
+
+ if (res.hdr.command != MEI_MANAGEMENT_CONTROL ||
+ res.hdr.versionnumber != MEI_MC_VERSION_NUMBER) {
+ dev_err(&cldev->dev, "wrong command received\n");
+ return;
+ }
+
+ if (res.hdr.subcommand != MEI_MC_START_WD_TIMER_RES) {
+ dev_warn(&cldev->dev, "unsupported command %d :%s[%d]\n",
+ res.hdr.subcommand,
+ mei_wdt_state_str(wdt->state),
+ wdt->state);
+ return;
+ }
+
+ if (wdt->state == MEI_WDT_RUNNING) {
+ if (res.wdstate & MEI_WDT_WDSTATE_NOT_REQUIRED) {
+ wdt->state = MEI_WDT_NOT_REQUIRED;
+ schedule_work(&wdt->unregister);
+ }
+ goto out;
+ }
+
+ if (wdt->state == MEI_WDT_PROBE) {
+ if (res.wdstate & MEI_WDT_WDSTATE_NOT_REQUIRED) {
+ wdt->state = MEI_WDT_NOT_REQUIRED;
+ } else {
+ /* stop the ping register watchdog device */
+ mei_wdt_stop(wdt);
+ mei_wdt_register(wdt);
+ }
+ return;
+ }
+
+ dev_warn(&cldev->dev, "not in correct state %s[%d]\n",
+ mei_wdt_state_str(wdt->state), wdt->state);
+
+out:
+ if (!completion_done(&wdt->response))
+ complete(&wdt->response);
+}
+
+/**
+ * mei_wdt_event - callback for event receive
+ *
+ * @cldev: bus device
+ * @events: event mask
+ * @context: callback context
+ */
+static void mei_wdt_event(struct mei_cl_device *cldev,
+ u32 events, void *context)
+{
+ if (events & BIT(MEI_CL_EVENT_RX))
+ mei_wdt_event_rx(cldev);
+}
+
#if IS_ENABLED(CONFIG_DEBUG_FS)
static ssize_t mei_dbgfs_read_state(struct file *file, char __user *ubuf,
@@ -482,8 +616,13 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
return -ENOMEM;
wdt->timeout = MEI_WDT_DEFAULT_TIMEOUT;
- wdt->state = MEI_WDT_IDLE;
+ wdt->state = MEI_WDT_PROBE;
wdt->cldev = cldev;
+ wdt->resp_required = mei_cldev_ver(cldev) > 0x1;
+ mutex_init(&wdt->reg_lock);
+ init_completion(&wdt->response);
+ INIT_WORK(&wdt->unregister, mei_wdt_unregister_work);
+
mei_cldev_set_drvdata(cldev, wdt);
ret = mei_cldev_enable(cldev);
@@ -492,9 +631,19 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
goto err_out;
}
+ ret = mei_cldev_register_event_cb(wdt->cldev, BIT(MEI_CL_EVENT_RX),
+ mei_wdt_event, NULL);
+ if (ret) {
+ dev_err(&cldev->dev, "Could not register event ret=%d\n", ret);
+ goto err_disable;
+ }
+
wd_info.firmware_version = mei_cldev_ver(cldev);
- ret = mei_wdt_register(wdt);
+ if (wdt->resp_required)
+ ret = mei_wdt_ping(wdt);
+ else
+ ret = mei_wdt_register(wdt);
if (ret)
goto err_disable;
@@ -515,6 +664,12 @@ static int mei_wdt_remove(struct mei_cl_device *cldev)
{
struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
+ /* Free the caller in case of fw initiated or unexpected reset */
+ if (!completion_done(&wdt->response))
+ complete(&wdt->response);
+
+ cancel_work_sync(&wdt->unregister);
+
mei_wdt_unregister(wdt);
mei_cldev_disable(cldev);
@@ -530,7 +685,7 @@ static int mei_wdt_remove(struct mei_cl_device *cldev)
0x89, 0x9D, 0xA9, 0x15, 0x14, 0xCB, 0x32, 0xAB)
static struct mei_cl_device_id mei_wdt_tbl[] = {
- { .uuid = MEI_UUID_WD, .version = 0x1},
+ { .uuid = MEI_UUID_WD, .version = MEI_CL_VERSION_ANY },
/* required last entry */
{ }
};
--
2.4.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [char-misc-next v3 6/8] watchdog: mei_wdt: register wd device only if required
2015-12-21 23:17 ` [char-misc-next v3 6/8] watchdog: mei_wdt: register wd device only if required Tomas Winkler
@ 2015-12-22 5:18 ` Guenter Roeck
2015-12-23 23:01 ` Winkler, Tomas
0 siblings, 1 reply; 19+ messages in thread
From: Guenter Roeck @ 2015-12-22 5:18 UTC (permalink / raw)
To: Tomas Winkler, Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Alexander Usyskin, linux-watchdog, linux-kernel
On 12/21/2015 03:17 PM, Tomas Winkler wrote:
> From: Alexander Usyskin <alexander.usyskin@intel.com>
>
> For Intel Broadwell and newer platforms, the ME device can inform
> the host whether the watchdog functionality is activated or not.
> If the watchdog functionality is not activated then the watchdog interface
> can be not registered and eliminate unnecessary pings and hence lower the
> power consumption by avoiding waking up the device.
> The feature can be deactivated also without reboot
> in that case the watchdog device should be unregistered at runtime.
>
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
> V2: rework unregistration
> V3: rebase; implement unregistraion also at runtime
>
> drivers/watchdog/mei_wdt.c | 183 +++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 169 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
> index ab9aec218d69..3cd80aa75db1 100644
> --- a/drivers/watchdog/mei_wdt.c
> +++ b/drivers/watchdog/mei_wdt.c
> @@ -16,6 +16,7 @@
> #include <linux/slab.h>
> #include <linux/interrupt.h>
> #include <linux/debugfs.h>
> +#include <linux/completion.h>
> #include <linux/watchdog.h>
>
> #include <linux/uuid.h>
> @@ -38,24 +39,30 @@
>
> /* Sub Commands */
> #define MEI_MC_START_WD_TIMER_REQ 0x13
> +#define MEI_MC_START_WD_TIMER_RES 0x83
> +#define MEI_WDT_STATUS_SUCCESS 0
> +#define MEI_WDT_WDSTATE_NOT_REQUIRED 0x1
> #define MEI_MC_STOP_WD_TIMER_REQ 0x14
>
> /**
> * enum mei_wdt_state - internal watchdog state
> *
> + * @MEI_WDT_PROBE: wd in probing stage
> * @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
> + * @MEI_WDT_NOT_REQUIRED: wd device is not required
> */
> enum mei_wdt_state {
> + MEI_WDT_PROBE,
> MEI_WDT_IDLE,
> MEI_WDT_START,
> MEI_WDT_RUNNING,
> MEI_WDT_STOPPING,
> + MEI_WDT_NOT_REQUIRED,
> };
>
> -#if IS_ENABLED(CONFIG_DEBUG_FS)
> static const char *mei_wdt_state_str(enum mei_wdt_state state)
> {
> switch (state) {
> @@ -71,7 +78,6 @@ static const char *mei_wdt_state_str(enum mei_wdt_state state)
> return "unknown";
> }
> }
> -#endif /* CONFIG_DEBUG_FS */
>
> struct mei_wdt;
>
> @@ -94,6 +100,10 @@ struct mei_wdt_dev {
> *
> * @cldev: mei watchdog client device
> * @state: watchdog internal state
> + * @resp_required: ping required response
> + * @response: ping response completion
> + * @unregister: unregister worker
> + * @reg_lock: watchdog device registration lock
> * @timeout: watchdog current timeout
> *
> * @dbgfs_dir: debugfs dir entry
> @@ -103,6 +113,10 @@ struct mei_wdt {
>
> struct mei_cl_device *cldev;
> enum mei_wdt_state state;
> + bool resp_required;
> + struct completion response;
> + struct work_struct unregister;
> + struct mutex reg_lock;
> u16 timeout;
>
> #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -139,6 +153,19 @@ struct mei_wdt_start_request {
> } __packed;
>
> /**
> + * struct mei_wdt_start_response watchdog start/ping response
> + *
> + * @hdr: Management Control Command Header
> + * @status: operation status
> + * @wdstate: watchdog status bit mask
> + */
> +struct mei_wdt_start_response {
> + struct mei_mc_hdr hdr;
> + u8 status;
> + u8 wdstate;
> +} __packed;
> +
> +/**
> * struct mei_wdt_stop_request - watchdog stop
> *
> * @hdr: Management Control Command Header
> @@ -277,13 +304,18 @@ static int mei_wdt_ops_ping(struct watchdog_device *wdd)
> if (wdt->state != MEI_WDT_START && wdt->state != MEI_WDT_RUNNING)
> return 0;
>
> + if (wdt->resp_required)
> + init_completion(&wdt->response);
> +
> + wdt->state = MEI_WDT_RUNNING;
> ret = mei_wdt_ping(wdt);
> if (ret)
> return ret;
>
> - wdt->state = MEI_WDT_RUNNING;
The state is now set to RUNNING even if the ping failed.
Is that on purpose ?
> + if (wdt->resp_required)
> + ret = wait_for_completion_killable(&wdt->response);
>
> - return 0;
> + return ret;
> }
>
> /**
> @@ -358,15 +390,22 @@ static struct watchdog_info wd_info = {
> */
> static void mei_wdt_unregister(struct mei_wdt *wdt)
> {
> - struct mei_wdt_dev *mwd = wdt->mwd;
> + struct mei_wdt_dev *mwd;
>
> - if (!mwd)
> - return;
> + mutex_lock(&wdt->reg_lock);
> +
> + if (!wdt->mwd)
> + goto out;
> +
Is this because the error on registration was ignored ?
Trying to understand the rationale for this check.
> + mwd = wdt->mwd;
>
> watchdog_unregister_device(&mwd->wdd);
> +
It would be better to make such changes in an earlier patch and avoid the
whitespace change here.
> wdt->mwd = NULL;
> - wdt->state = MEI_WDT_IDLE;
> kref_put(&mwd->refcnt, mei_wdt_release);
> +
> +out:
> + mutex_unlock(&wdt->reg_lock);
> }
>
> /**
> @@ -387,9 +426,13 @@ static int mei_wdt_register(struct mei_wdt *wdt)
>
> dev = &wdt->cldev->dev;
>
> + mutex_lock(&wdt->reg_lock);
> +
> mwd = kzalloc(sizeof(struct mei_wdt_dev), GFP_KERNEL);
> - if (!mwd)
> - return -ENOMEM;
> + if (!mwd) {
> + ret = -ENOMEM;
> + goto out;
> + }
>
> mwd->wdt = wdt;
> mwd->wdd.info = &wd_info;
> @@ -405,13 +448,104 @@ static int mei_wdt_register(struct mei_wdt *wdt)
> if (ret) {
> dev_err(dev, "unable to register watchdog device = %d.\n", ret);
> kref_put(&mwd->refcnt, mei_wdt_release);
> - return ret;
> + goto out;
> }
>
> wdt->mwd = mwd;
> +out:
> + mutex_unlock(&wdt->reg_lock);
> return 0;
Do you want to return ret here ? Otherwise some of the code above does
not really make sense (ie setting ret).
> }
>
> +static void mei_wdt_unregister_work(struct work_struct *work)
> +{
> + struct mei_wdt *wdt = container_of(work, struct mei_wdt, unregister);
> +
> + mei_wdt_unregister(wdt);
> +}
> +
> +/**
> + * mei_wdt_event_rx - callback for data receive
> + *
> + * @cldev: bus device
> + */
> +static void mei_wdt_event_rx(struct mei_cl_device *cldev)
> +{
> + struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
> + struct mei_wdt_start_response res;
> + const size_t res_len = sizeof(res);
> + int ret;
> +
> + ret = mei_cldev_recv(wdt->cldev, (u8 *)&res, res_len);
> + if (ret < 0) {
> + dev_err(&cldev->dev, "failure in recv %d\n", ret);
Not objecting, just concerned. Can those error messages
result in filling up the kernel log if the mei hardware has a problem ?
From an operational perspective, is it acceptable to ignore the errors ?
> + return;
> + }
> +
> + /* Empty response can be sent on stop */
> + if (ret == 0)
> + return;
> +
> + if (ret < sizeof(struct mei_mc_hdr)) {
> + dev_err(&cldev->dev, "recv small data %d\n", ret);
> + return;
> + }
> +
> + if (res.hdr.command != MEI_MANAGEMENT_CONTROL ||
> + res.hdr.versionnumber != MEI_MC_VERSION_NUMBER) {
> + dev_err(&cldev->dev, "wrong command received\n");
> + return;
> + }
> +
> + if (res.hdr.subcommand != MEI_MC_START_WD_TIMER_RES) {
> + dev_warn(&cldev->dev, "unsupported command %d :%s[%d]\n",
> + res.hdr.subcommand,
> + mei_wdt_state_str(wdt->state),
> + wdt->state);
> + return;
> + }
> +
> + if (wdt->state == MEI_WDT_RUNNING) {
> + if (res.wdstate & MEI_WDT_WDSTATE_NOT_REQUIRED) {
> + wdt->state = MEI_WDT_NOT_REQUIRED;
> + schedule_work(&wdt->unregister);
> + }
> + goto out;
> + }
> +
> + if (wdt->state == MEI_WDT_PROBE) {
> + if (res.wdstate & MEI_WDT_WDSTATE_NOT_REQUIRED) {
> + wdt->state = MEI_WDT_NOT_REQUIRED;
> + } else {
> + /* stop the ping register watchdog device */
Should this be "stop the watchdog" ?
> + mei_wdt_stop(wdt);
> + mei_wdt_register(wdt);
> + }
> + return;
> + }
> +
> + dev_warn(&cldev->dev, "not in correct state %s[%d]\n",
> + mei_wdt_state_str(wdt->state), wdt->state);
> +
> +out:
> + if (!completion_done(&wdt->response))
> + complete(&wdt->response);
> +}
> +
> +/**
> + * mei_wdt_event - callback for event receive
> + *
> + * @cldev: bus device
> + * @events: event mask
> + * @context: callback context
> + */
> +static void mei_wdt_event(struct mei_cl_device *cldev,
> + u32 events, void *context)
> +{
> + if (events & BIT(MEI_CL_EVENT_RX))
> + mei_wdt_event_rx(cldev);
> +}
> +
> #if IS_ENABLED(CONFIG_DEBUG_FS)
>
> static ssize_t mei_dbgfs_read_state(struct file *file, char __user *ubuf,
> @@ -482,8 +616,13 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
> return -ENOMEM;
>
> wdt->timeout = MEI_WDT_DEFAULT_TIMEOUT;
> - wdt->state = MEI_WDT_IDLE;
> + wdt->state = MEI_WDT_PROBE;
> wdt->cldev = cldev;
> + wdt->resp_required = mei_cldev_ver(cldev) > 0x1;
> + mutex_init(&wdt->reg_lock);
> + init_completion(&wdt->response);
> + INIT_WORK(&wdt->unregister, mei_wdt_unregister_work);
> +
> mei_cldev_set_drvdata(cldev, wdt);
>
> ret = mei_cldev_enable(cldev);
> @@ -492,9 +631,19 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
> goto err_out;
> }
>
> + ret = mei_cldev_register_event_cb(wdt->cldev, BIT(MEI_CL_EVENT_RX),
> + mei_wdt_event, NULL);
> + if (ret) {
> + dev_err(&cldev->dev, "Could not register event ret=%d\n", ret);
> + goto err_disable;
> + }
> +
Can there be an event before the call to mei_wdt_register() ?
If so, it that case handled correctly ?
> wd_info.firmware_version = mei_cldev_ver(cldev);
>
> - ret = mei_wdt_register(wdt);
> + if (wdt->resp_required)
> + ret = mei_wdt_ping(wdt);
> + else
> + ret = mei_wdt_register(wdt);
> if (ret)
> goto err_disable;
>
> @@ -515,6 +664,12 @@ static int mei_wdt_remove(struct mei_cl_device *cldev)
> {
> struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
>
> + /* Free the caller in case of fw initiated or unexpected reset */
> + if (!completion_done(&wdt->response))
> + complete(&wdt->response);
> +
> + cancel_work_sync(&wdt->unregister);
> +
> mei_wdt_unregister(wdt);
>
can there be an event callback after mei_wdt_unregister() but before mei_cldev_disable() ?
If so, is the situation handled or would it result in a race condition ?
[ I think it is ok in both cases, just trying to make sure I didn't miss anything ]
> mei_cldev_disable(cldev);
> @@ -530,7 +685,7 @@ static int mei_wdt_remove(struct mei_cl_device *cldev)
> 0x89, 0x9D, 0xA9, 0x15, 0x14, 0xCB, 0x32, 0xAB)
>
> static struct mei_cl_device_id mei_wdt_tbl[] = {
> - { .uuid = MEI_UUID_WD, .version = 0x1},
> + { .uuid = MEI_UUID_WD, .version = MEI_CL_VERSION_ANY },
> /* required last entry */
> { }
> };
>
^ permalink raw reply [flat|nested] 19+ messages in thread* RE: [char-misc-next v3 6/8] watchdog: mei_wdt: register wd device only if required
2015-12-22 5:18 ` Guenter Roeck
@ 2015-12-23 23:01 ` Winkler, Tomas
0 siblings, 0 replies; 19+ messages in thread
From: Winkler, Tomas @ 2015-12-23 23:01 UTC (permalink / raw)
To: Guenter Roeck, Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Usyskin, Alexander, linux-watchdog, linux-kernel
next v3 6/8] watchdog: mei_wdt: register wd device only
> if required
>
> On 12/21/2015 03:17 PM, Tomas Winkler wrote:
> > From: Alexander Usyskin <alexander.usyskin@intel.com>
> >
> > For Intel Broadwell and newer platforms, the ME device can inform
> > the host whether the watchdog functionality is activated or not.
> > If the watchdog functionality is not activated then the watchdog interface
> > can be not registered and eliminate unnecessary pings and hence lower the
> > power consumption by avoiding waking up the device.
> > The feature can be deactivated also without reboot
> > in that case the watchdog device should be unregistered at runtime.
> >
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> > V2: rework unregistration
> > V3: rebase; implement unregistraion also at runtime
> >
> > drivers/watchdog/mei_wdt.c | 183
> +++++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 169 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
> > index ab9aec218d69..3cd80aa75db1 100644
> > --- a/drivers/watchdog/mei_wdt.c
> > +++ b/drivers/watchdog/mei_wdt.c
> > @@ -16,6 +16,7 @@
> > #include <linux/slab.h>
> > #include <linux/interrupt.h>
> > #include <linux/debugfs.h>
> > +#include <linux/completion.h>
> > #include <linux/watchdog.h>
> >
> > #include <linux/uuid.h>
> > @@ -38,24 +39,30 @@
> >
> > /* Sub Commands */
> > #define MEI_MC_START_WD_TIMER_REQ 0x13
> > +#define MEI_MC_START_WD_TIMER_RES 0x83
> > +#define MEI_WDT_STATUS_SUCCESS 0
> > +#define MEI_WDT_WDSTATE_NOT_REQUIRED 0x1
> > #define MEI_MC_STOP_WD_TIMER_REQ 0x14
> >
> > /**
> > * enum mei_wdt_state - internal watchdog state
> > *
> > + * @MEI_WDT_PROBE: wd in probing stage
> > * @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
> > + * @MEI_WDT_NOT_REQUIRED: wd device is not required
> > */
> > enum mei_wdt_state {
> > + MEI_WDT_PROBE,
> > MEI_WDT_IDLE,
> > MEI_WDT_START,
> > MEI_WDT_RUNNING,
> > MEI_WDT_STOPPING,
> > + MEI_WDT_NOT_REQUIRED,
> > };
> >
> > -#if IS_ENABLED(CONFIG_DEBUG_FS)
> > static const char *mei_wdt_state_str(enum mei_wdt_state state)
> > {
> > switch (state) {
> > @@ -71,7 +78,6 @@ static const char *mei_wdt_state_str(enum
> mei_wdt_state state)
> > return "unknown";
> > }
> > }
> > -#endif /* CONFIG_DEBUG_FS */
> >
> > struct mei_wdt;
> >
> > @@ -94,6 +100,10 @@ struct mei_wdt_dev {
> > *
> > * @cldev: mei watchdog client device
> > * @state: watchdog internal state
> > + * @resp_required: ping required response
> > + * @response: ping response completion
> > + * @unregister: unregister worker
> > + * @reg_lock: watchdog device registration lock
> > * @timeout: watchdog current timeout
> > *
> > * @dbgfs_dir: debugfs dir entry
> > @@ -103,6 +113,10 @@ struct mei_wdt {
> >
> > struct mei_cl_device *cldev;
> > enum mei_wdt_state state;
> > + bool resp_required;
> > + struct completion response;
> > + struct work_struct unregister;
> > + struct mutex reg_lock;
> > u16 timeout;
> >
> > #if IS_ENABLED(CONFIG_DEBUG_FS)
> > @@ -139,6 +153,19 @@ struct mei_wdt_start_request {
> > } __packed;
> >
> > /**
> > + * struct mei_wdt_start_response watchdog start/ping response
> > + *
> > + * @hdr: Management Control Command Header
> > + * @status: operation status
> > + * @wdstate: watchdog status bit mask
> > + */
> > +struct mei_wdt_start_response {
> > + struct mei_mc_hdr hdr;
> > + u8 status;
> > + u8 wdstate;
> > +} __packed;
> > +
> > +/**
> > * struct mei_wdt_stop_request - watchdog stop
> > *
> > * @hdr: Management Control Command Header
> > @@ -277,13 +304,18 @@ static int mei_wdt_ops_ping(struct watchdog_device
> *wdd)
> > if (wdt->state != MEI_WDT_START && wdt->state !=
> MEI_WDT_RUNNING)
> > return 0;
> >
> > + if (wdt->resp_required)
> > + init_completion(&wdt->response);
> > +
> > + wdt->state = MEI_WDT_RUNNING;
> > ret = mei_wdt_ping(wdt);
> > if (ret)
> > return ret;
> >
> > - wdt->state = MEI_WDT_RUNNING;
>
> The state is now set to RUNNING even if the ping failed.
> Is that on purpose ?
Yes, the state is checked in the response handler.
>
> > + if (wdt->resp_required)
> > + ret = wait_for_completion_killable(&wdt->response);
> >
> > - return 0;
> > + return ret;
> > }
> >
> > /**
> > @@ -358,15 +390,22 @@ static struct watchdog_info wd_info = {
> > */
> > static void mei_wdt_unregister(struct mei_wdt *wdt)
> > {
> > - struct mei_wdt_dev *mwd = wdt->mwd;
> > + struct mei_wdt_dev *mwd;
> >
> > - if (!mwd)
> > - return;
> > + mutex_lock(&wdt->reg_lock);
> > +
> > + if (!wdt->mwd)
> > + goto out;
> > +
>
> Is this because the error on registration was ignored ?
> Trying to understand the rationale for this check.
Protection against double call, mostly from testing hooks which are not part of the series.
>
> > + mwd = wdt->mwd;
> >
> > watchdog_unregister_device(&mwd->wdd);
> > +
>
> It would be better to make such changes in an earlier patch and avoid the
> whitespace change here.
>
> > wdt->mwd = NULL;
> > - wdt->state = MEI_WDT_IDLE;
> > kref_put(&mwd->refcnt, mei_wdt_release);
> > +
> > +out:
> > + mutex_unlock(&wdt->reg_lock);
> > }
> >
> > /**
> > @@ -387,9 +426,13 @@ static int mei_wdt_register(struct mei_wdt *wdt)
> >
> > dev = &wdt->cldev->dev;
> >
> > + mutex_lock(&wdt->reg_lock);
> > +
> > mwd = kzalloc(sizeof(struct mei_wdt_dev), GFP_KERNEL);
> > - if (!mwd)
> > - return -ENOMEM;
> > + if (!mwd) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> >
> > mwd->wdt = wdt;
> > mwd->wdd.info = &wd_info;
> > @@ -405,13 +448,104 @@ static int mei_wdt_register(struct mei_wdt *wdt)
> > if (ret) {
> > dev_err(dev, "unable to register watchdog device = %d.\n", ret);
> > kref_put(&mwd->refcnt, mei_wdt_release);
> > - return ret;
> > + goto out;
> > }
> >
> > wdt->mwd = mwd;
> > +out:
> > + mutex_unlock(&wdt->reg_lock);
> > return 0;
>
> Do you want to return ret here ? Otherwise some of the code above does
> not really make sense (ie setting ret).
Thanks, good catch.
> > }
> >
> > +static void mei_wdt_unregister_work(struct work_struct *work)
> > +{
> > + struct mei_wdt *wdt = container_of(work, struct mei_wdt, unregister);
> > +
> > + mei_wdt_unregister(wdt);
> > +}
> > +
> > +/**
> > + * mei_wdt_event_rx - callback for data receive
> > + *
> > + * @cldev: bus device
> > + */
> > +static void mei_wdt_event_rx(struct mei_cl_device *cldev)
> > +{
> > + struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
> > + struct mei_wdt_start_response res;
> > + const size_t res_len = sizeof(res);
> > + int ret;
> > +
> > + ret = mei_cldev_recv(wdt->cldev, (u8 *)&res, res_len);
> > + if (ret < 0) {
> > + dev_err(&cldev->dev, "failure in recv %d\n", ret);
>
> Not objecting, just concerned. Can those error messages
> result in filling up the kernel log if the mei hardware has a problem ?
I hope no, the HW will reset eventually.
> From an operational perspective, is it acceptable to ignore the errors ?
Not really, we want to know when it happens and from what flow.
> > + return;
> > + }
> > +
> > + /* Empty response can be sent on stop */
> > + if (ret == 0)
> > + return;
> > +
> > + if (ret < sizeof(struct mei_mc_hdr)) {
> > + dev_err(&cldev->dev, "recv small data %d\n", ret);
> > + return;
> > + }
> > +
> > + if (res.hdr.command != MEI_MANAGEMENT_CONTROL ||
> > + res.hdr.versionnumber != MEI_MC_VERSION_NUMBER) {
> > + dev_err(&cldev->dev, "wrong command received\n");
> > + return;
> > + }
> > +
> > + if (res.hdr.subcommand != MEI_MC_START_WD_TIMER_RES) {
> > + dev_warn(&cldev->dev, "unsupported command %d :%s[%d]\n",
> > + res.hdr.subcommand,
> > + mei_wdt_state_str(wdt->state),
> > + wdt->state);
> > + return;
> > + }
> > +
> > + if (wdt->state == MEI_WDT_RUNNING) {
> > + if (res.wdstate & MEI_WDT_WDSTATE_NOT_REQUIRED) {
> > + wdt->state = MEI_WDT_NOT_REQUIRED;
> > + schedule_work(&wdt->unregister);
> > + }
> > + goto out;
> > + }
> > +
> > + if (wdt->state == MEI_WDT_PROBE) {
> > + if (res.wdstate & MEI_WDT_WDSTATE_NOT_REQUIRED) {
> > + wdt->state = MEI_WDT_NOT_REQUIRED;
> > + } else {
> > + /* stop the ping register watchdog device */
>
> Should this be "stop the watchdog" ?
Maybe this needs rephrasing
>
> > + mei_wdt_stop(wdt);
> > + mei_wdt_register(wdt);
> > + }
> > + return;
> > + }
> > +
> > + dev_warn(&cldev->dev, "not in correct state %s[%d]\n",
> > + mei_wdt_state_str(wdt->state), wdt->state);
> > +
> > +out:
> > + if (!completion_done(&wdt->response))
> > + complete(&wdt->response);
> > +}
> > +
> > +/**
> > + * mei_wdt_event - callback for event receive
> > + *
> > + * @cldev: bus device
> > + * @events: event mask
> > + * @context: callback context
> > + */
> > +static void mei_wdt_event(struct mei_cl_device *cldev,
> > + u32 events, void *context)
> > +{
> > + if (events & BIT(MEI_CL_EVENT_RX))
> > + mei_wdt_event_rx(cldev);
> > +}
> > +
> > #if IS_ENABLED(CONFIG_DEBUG_FS)
> >
> > static ssize_t mei_dbgfs_read_state(struct file *file, char __user *ubuf,
> > @@ -482,8 +616,13 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
> > return -ENOMEM;
> >
> > wdt->timeout = MEI_WDT_DEFAULT_TIMEOUT;
> > - wdt->state = MEI_WDT_IDLE;
> > + wdt->state = MEI_WDT_PROBE;
> > wdt->cldev = cldev;
> > + wdt->resp_required = mei_cldev_ver(cldev) > 0x1;
> > + mutex_init(&wdt->reg_lock);
> > + init_completion(&wdt->response);
> > + INIT_WORK(&wdt->unregister, mei_wdt_unregister_work);
> > +
> > mei_cldev_set_drvdata(cldev, wdt);
> >
> > ret = mei_cldev_enable(cldev);
> > @@ -492,9 +631,19 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
> > goto err_out;
> > }
> >
> > + ret = mei_cldev_register_event_cb(wdt->cldev, BIT(MEI_CL_EVENT_RX),
> > + mei_wdt_event, NULL);
> > + if (ret) {
> > + dev_err(&cldev->dev, "Could not register event ret=%d\n", ret);
> > + goto err_disable;
> > + }
> > +
> Can there be an event before the call to mei_wdt_register() ?
> If so, it that case handled correctly ?
Yes, there is an first internal ping that checks whether the feature is enabled
>
> > wd_info.firmware_version = mei_cldev_ver(cldev);
> >
> > - ret = mei_wdt_register(wdt);
> > + if (wdt->resp_required)
> > + ret = mei_wdt_ping(wdt);
> > + else
> > + ret = mei_wdt_register(wdt);
> > if (ret)
> > goto err_disable;
> >
> > @@ -515,6 +664,12 @@ static int mei_wdt_remove(struct mei_cl_device
> *cldev)
> > {
> > struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
> >
> > + /* Free the caller in case of fw initiated or unexpected reset */
> > + if (!completion_done(&wdt->response))
> > + complete(&wdt->response);
> > +
> > + cancel_work_sync(&wdt->unregister);
> > +
> > mei_wdt_unregister(wdt);
> >
> can there be an event callback after mei_wdt_unregister() but before
> mei_cldev_disable() ?
> If so, is the situation handled or would it result in a race condition ?
Not really, unless there is some glitch in the FW
> [ I think it is ok in both cases, just trying to make sure I didn't miss anything ]
>
> > mei_cldev_disable(cldev);
> > @@ -530,7 +685,7 @@ static int mei_wdt_remove(struct mei_cl_device *cldev)
> > 0x89, 0x9D, 0xA9, 0x15, 0x14, 0xCB, 0x32, 0xAB)
> >
> > static struct mei_cl_device_id mei_wdt_tbl[] = {
> > - { .uuid = MEI_UUID_WD, .version = 0x1},
> > + { .uuid = MEI_UUID_WD, .version = MEI_CL_VERSION_ANY },
> > /* required last entry */
> > { }
> > };
> >
^ permalink raw reply [flat|nested] 19+ messages in thread
* [char-misc-next v3 7/8] watchdog: mei_wdt: add activation debugfs entry
2015-12-21 23:17 [char-misc-next v3 0/8] mei: create proper iAMT watchdog driver Tomas Winkler
` (5 preceding siblings ...)
2015-12-21 23:17 ` [char-misc-next v3 6/8] watchdog: mei_wdt: register wd device only if required Tomas Winkler
@ 2015-12-21 23:18 ` Tomas Winkler
2015-12-21 23:18 ` [char-misc-next v3 8/8] watchdog: mei_wdt: re-register device on event Tomas Winkler
7 siblings, 0 replies; 19+ messages in thread
From: Tomas Winkler @ 2015-12-21 23:18 UTC (permalink / raw)
To: Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Alexander Usyskin, Guenter Roeck, linux-watchdog, linux-kernel,
Tomas Winkler
Add entry for displaying whether the device has activated or
deactivated watchdog fw application.
cat <debugfs>/mei_wdt/activation
activated | deactivated
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V3: new in the series
drivers/watchdog/mei_wdt.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
index 3cd80aa75db1..88511ef68b67 100644
--- a/drivers/watchdog/mei_wdt.c
+++ b/drivers/watchdog/mei_wdt.c
@@ -548,6 +548,28 @@ static void mei_wdt_event(struct mei_cl_device *cldev,
#if IS_ENABLED(CONFIG_DEBUG_FS)
+static ssize_t mei_dbgfs_read_activation(struct file *file, char __user *ubuf,
+ size_t cnt, loff_t *ppos)
+{
+ struct mei_wdt *wdt = file->private_data;
+ const size_t bufsz = 32;
+ char buf[bufsz];
+ ssize_t pos = 0;
+
+ mutex_lock(&wdt->reg_lock);
+ pos += scnprintf(buf + pos, bufsz - pos, "event: %s\n",
+ wdt->mwd ? "activated" : "deactivated");
+ mutex_unlock(&wdt->reg_lock);
+
+ return simple_read_from_buffer(ubuf, cnt, ppos, buf, pos);
+}
+
+static const struct file_operations dbgfs_fops_activation = {
+ .open = simple_open,
+ .read = mei_dbgfs_read_activation,
+ .llseek = generic_file_llseek,
+};
+
static ssize_t mei_dbgfs_read_state(struct file *file, char __user *ubuf,
size_t cnt, loff_t *ppos)
{
@@ -589,6 +611,11 @@ static int dbgfs_register(struct mei_wdt *wdt)
if (!f)
goto err;
+ f = debugfs_create_file("activation", S_IRUSR,
+ dir, wdt, &dbgfs_fops_activation);
+ if (!f)
+ goto err;
+
return 0;
err:
dbgfs_unregister(wdt);
--
2.4.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* [char-misc-next v3 8/8] watchdog: mei_wdt: re-register device on event
2015-12-21 23:17 [char-misc-next v3 0/8] mei: create proper iAMT watchdog driver Tomas Winkler
` (6 preceding siblings ...)
2015-12-21 23:18 ` [char-misc-next v3 7/8] watchdog: mei_wdt: add activation debugfs entry Tomas Winkler
@ 2015-12-21 23:18 ` Tomas Winkler
7 siblings, 0 replies; 19+ messages in thread
From: Tomas Winkler @ 2015-12-21 23:18 UTC (permalink / raw)
To: Greg Kroah-Hartman, Wim Van Sebroeck
Cc: Alexander Usyskin, Guenter Roeck, linux-watchdog, linux-kernel,
Tomas Winkler
From: Alexander Usyskin <alexander.usyskin@intel.com>
For Intel SKL platform the ME device can inform the host via
asynchronous notification that the watchdog feature was activated
on the device. The activation doesn't require reboot.
In that case the driver registers the watchdog device with the kernel.
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2: rework un/registration in runtime
V2: rebase, runtime unregistration was moved to BDW patch.
drivers/watchdog/mei_wdt.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
index 88511ef68b67..93fb8ce80456 100644
--- a/drivers/watchdog/mei_wdt.c
+++ b/drivers/watchdog/mei_wdt.c
@@ -532,6 +532,21 @@ out:
complete(&wdt->response);
}
+/*
+ * mei_wdt_notify_event - callback for event notification
+ *
+ * @cldev: bus device
+ */
+static void mei_wdt_notify_event(struct mei_cl_device *cldev)
+{
+ struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
+
+ if (wdt->state != MEI_WDT_NOT_REQUIRED)
+ return;
+ wdt->state = MEI_WDT_IDLE;
+ mei_wdt_register(wdt);
+}
+
/**
* mei_wdt_event - callback for event receive
*
@@ -544,6 +559,9 @@ static void mei_wdt_event(struct mei_cl_device *cldev,
{
if (events & BIT(MEI_CL_EVENT_RX))
mei_wdt_event_rx(cldev);
+
+ if (events & BIT(MEI_CL_EVENT_NOTIF))
+ mei_wdt_notify_event(cldev);
}
#if IS_ENABLED(CONFIG_DEBUG_FS)
@@ -658,9 +676,13 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
goto err_out;
}
- ret = mei_cldev_register_event_cb(wdt->cldev, BIT(MEI_CL_EVENT_RX),
+ ret = mei_cldev_register_event_cb(wdt->cldev,
+ BIT(MEI_CL_EVENT_RX) |
+ BIT(MEI_CL_EVENT_NOTIF),
mei_wdt_event, NULL);
- if (ret) {
+
+ /* on legacy devices notification is not supported */
+ if (ret && ret != -EOPNOTSUPP) {
dev_err(&cldev->dev, "Could not register event ret=%d\n", ret);
goto err_disable;
}
--
2.4.3
^ permalink raw reply related [flat|nested] 19+ messages in thread