[char-misc-next v2 0/7] mei: create proper iAMT watchdog driver

[char-misc-next v2 0/7] mei: create proper iAMT watchdog driver

[Tomas Winkler]

Instead of integrating the iAMT watchdog within the mei core driver
we will create a watchdog device on the mei client bus and
create a proper watchdog driver for it.

V2: 
1. The watchdog device is no longer dynamically allocated in separate structure
2. Export device internal status via debugfs
3. Address few comments from Guenter
4. Reworked de/registration

The patches should apply and compile on both watchdog-next and char-misc-next
trees/branches.
I would prefer this will go via char-misc-next as this is the tree
we work against and conflicts should be trivial if at any.

Alexander Usyskin (3):
  mei: wd: drop the watchdog code from the core mei driver
  watchdog: mei_wdt: register wd device only if required
  watchdog: mei_wdt: re-register device on event

Tomas Winkler (4):
  mei: drop nfc leftovers from the mei driver
  watchdog: mei_wdt: implement MEI iAMT watchdog driver
  watchdog: mei_wdt: add status debugfs entry
  mei: bus: whitelist the watchdog client

 Documentation/misc-devices/mei/mei.txt |  12 +-
 MAINTAINERS                            |   1 +
 drivers/misc/mei/Kconfig               |   6 +-
 drivers/misc/mei/Makefile              |   1 -
 drivers/misc/mei/bus-fixup.c           |  29 ++
 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             |  72 +---
 drivers/misc/mei/wd.c                  | 391 -------------------
 drivers/watchdog/Kconfig               |  15 +
 drivers/watchdog/Makefile              |   1 +
 drivers/watchdog/mei_wdt.c             | 692 +++++++++++++++++++++++++++++++++
 14 files changed, 753 insertions(+), 508 deletions(-)
 delete mode 100644 drivers/misc/mei/wd.c
 create mode 100644 drivers/watchdog/mei_wdt.c

-- 
2.4.3

[char-misc-next v2 1/7] mei: drop nfc leftovers from the mei driver

[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: 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

[char-misc-next v2 2/7] mei: wd: drop the watchdog code from the core mei driver

[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: 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

[char-misc-next v2 3/7] watchdog: mei_wdt: implement MEI iAMT watchdog driver

[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

 Documentation/misc-devices/mei/mei.txt |  12 +-
 MAINTAINERS                            |   1 +
 drivers/watchdog/Kconfig               |  15 ++
 drivers/watchdog/Makefile              |   1 +
 drivers/watchdog/mei_wdt.c             | 415 +++++++++++++++++++++++++++++++++
 5 files changed, 438 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..3c97deb70d90
--- /dev/null
+++ b/drivers/watchdog/mei_wdt.c
@@ -0,0 +1,415 @@
+/*
+ * 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 - mei watchdog driver
+ * @wdd: watchdog device
+ * @refcnt: watchdog device reference counter
+ *
+ * @state: watchdog internal state
+ * @cldev: mei watchdog client device
+ * @timeout: watchdog current timeout
+ */
+struct mei_wdt {
+	struct watchdog_device wdd;
+	struct kref refcnt;
+
+	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;
+}
+
+static void mei_wdt_release(struct kref *ref)
+{
+	struct mei_wdt *wdt = container_of(ref, struct mei_wdt, refcnt);
+
+	kfree(wdt);
+}
+
+/**
+ * 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 device *dev;
+	int ret;
+
+	dev = &wdt->cldev->dev;
+
+	watchdog_set_drvdata(&wdt->wdd, wdt);
+
+	ret = watchdog_register_device(&wdt->wdd);
+	if (ret) {
+		dev_err(dev, "unable to register watchdog device = %d.\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * mei_wdt_ops_start - wd start command from the watchdog core.
+ *
+ * @wdd: watchdog device
+ *
+ * Return: 0
+ */
+static int mei_wdt_ops_start(struct watchdog_device *wdd)
+{
+	struct mei_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	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 *wdt = watchdog_get_drvdata(wdd);
+	int ret;
+
+	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 *wdt = watchdog_get_drvdata(wdd);
+	int ret;
+
+	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 *wdt = watchdog_get_drvdata(wdd);
+
+	/* valid value is already checked by the caller */
+	wdt->timeout = timeout;
+	wdd->timeout = timeout;
+
+	return 0;
+}
+
+static void mei_wdt_ops_ref(struct watchdog_device *wdd)
+{
+	struct mei_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	kref_get(&wdt->refcnt);
+}
+
+static void mei_wdt_ops_unref(struct watchdog_device *wdd)
+{
+	struct mei_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	kref_put(&wdt->refcnt, mei_wdt_release);
+}
+
+static const struct watchdog_ops wd_ops = {
+	.owner       = THIS_MODULE,
+	.start       = mei_wdt_ops_start,
+	.stop        = mei_wdt_ops_stop,
+	.ping        = mei_wdt_ops_ping,
+	.set_timeout = mei_wdt_ops_set_timeout,
+	.ref         = mei_wdt_ops_ref,
+	.unref       = mei_wdt_ops_unref,
+};
+
+static struct watchdog_info wd_info = {
+	.identity = INTEL_AMT_WATCHDOG_ID,
+	.options  = WDIOF_KEEPALIVEPING |
+		    WDIOF_SETTIMEOUT |
+		    WDIOF_ALARMONLY,
+};
+
+static int mei_wdt_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;
+
+	kref_init(&wdt->refcnt);
+	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);
+
+	wdt->wdd.info = &wd_info;
+	wdt->wdd.ops = &wd_ops;
+	wdt->wdd.parent = &cldev->dev;
+	wdt->wdd.timeout = wdt->timeout;
+	wdt->wdd.min_timeout = MEI_WDT_MIN_TIMEOUT;
+	wdt->wdd.max_timeout = MEI_WDT_MAX_TIMEOUT;
+
+	ret = mei_wdt_register(wdt);
+	if (ret)
+		goto err_disable;
+
+	return 0;
+
+err_disable:
+	mei_cldev_disable(cldev);
+
+err_out:
+	kref_put(&wdt->refcnt, mei_wdt_release);
+
+	return ret;
+}
+
+static int mei_wdt_remove(struct mei_cl_device *cldev)
+{
+	struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
+
+	watchdog_unregister_device(&wdt->wdd);
+
+	kref_put(&wdt->refcnt, mei_wdt_release);
+
+	mei_cldev_disable(cldev);
+
+	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

Re: [char-misc-next v2 3/7] watchdog: mei_wdt: implement MEI iAMT watchdog driver

[Guenter Roeck]

On 12/17/2015 06:49 AM, 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>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> V2: The watchdog device is no longer dynamically allocated in separate structure
>
>   Documentation/misc-devices/mei/mei.txt |  12 +-
>   MAINTAINERS                            |   1 +
>   drivers/watchdog/Kconfig               |  15 ++
>   drivers/watchdog/Makefile              |   1 +
>   drivers/watchdog/mei_wdt.c             | 415 +++++++++++++++++++++++++++++++++
>   5 files changed, 438 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..3c97deb70d90
> --- /dev/null
> +++ b/drivers/watchdog/mei_wdt.c
> @@ -0,0 +1,415 @@
> +/*
> + * 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 - mei watchdog driver
> + * @wdd: watchdog device
> + * @refcnt: watchdog device reference counter
> + *
> + * @state: watchdog internal state
> + * @cldev: mei watchdog client device
> + * @timeout: watchdog current timeout
> + */
> +struct mei_wdt {
> +	struct watchdog_device wdd;
> +	struct kref refcnt;
> +
> +	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;
> +}
> +
> +static void mei_wdt_release(struct kref *ref)
> +{
> +	struct mei_wdt *wdt = container_of(ref, struct mei_wdt, refcnt);
> +
> +	kfree(wdt);
> +}
> +
> +/**
> + * 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 device *dev;
> +	int ret;
> +
> +	dev = &wdt->cldev->dev;
> +
> +	watchdog_set_drvdata(&wdt->wdd, wdt);
> +
> +	ret = watchdog_register_device(&wdt->wdd);
> +	if (ret) {
> +		dev_err(dev, "unable to register watchdog device = %d.\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * mei_wdt_ops_start - wd start command from the watchdog core.
> + *
> + * @wdd: watchdog device
> + *
> + * Return: 0
> + */
> +static int mei_wdt_ops_start(struct watchdog_device *wdd)
> +{
> +	struct mei_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	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 *wdt = watchdog_get_drvdata(wdd);
> +	int ret;
> +
> +	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 *wdt = watchdog_get_drvdata(wdd);
> +	int ret;
> +
> +	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 *wdt = watchdog_get_drvdata(wdd);
> +
> +	/* valid value is already checked by the caller */
> +	wdt->timeout = timeout;
> +	wdd->timeout = timeout;
> +
> +	return 0;
> +}
> +
> +static void mei_wdt_ops_ref(struct watchdog_device *wdd)
> +{
> +	struct mei_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	kref_get(&wdt->refcnt);
> +}
> +
> +static void mei_wdt_ops_unref(struct watchdog_device *wdd)
> +{
> +	struct mei_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	kref_put(&wdt->refcnt, mei_wdt_release);
> +}
> +
> +static const struct watchdog_ops wd_ops = {
> +	.owner       = THIS_MODULE,
> +	.start       = mei_wdt_ops_start,
> +	.stop        = mei_wdt_ops_stop,
> +	.ping        = mei_wdt_ops_ping,
> +	.set_timeout = mei_wdt_ops_set_timeout,
> +	.ref         = mei_wdt_ops_ref,
> +	.unref       = mei_wdt_ops_unref,
> +};
> +
> +static struct watchdog_info wd_info = {
> +	.identity = INTEL_AMT_WATCHDOG_ID,
> +	.options  = WDIOF_KEEPALIVEPING |
> +		    WDIOF_SETTIMEOUT |
> +		    WDIOF_ALARMONLY,
> +};
> +
> +static int mei_wdt_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;
> +
> +	kref_init(&wdt->refcnt);
> +	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);
> +
> +	wdt->wdd.info = &wd_info;
> +	wdt->wdd.ops = &wd_ops;
> +	wdt->wdd.parent = &cldev->dev;
> +	wdt->wdd.timeout = wdt->timeout;
> +	wdt->wdd.min_timeout = MEI_WDT_MIN_TIMEOUT;
> +	wdt->wdd.max_timeout = MEI_WDT_MAX_TIMEOUT;
> +
> +	ret = mei_wdt_register(wdt);
> +	if (ret)
> +		goto err_disable;
> +
> +	return 0;
> +
> +err_disable:
> +	mei_cldev_disable(cldev);
> +
> +err_out:
> +	kref_put(&wdt->refcnt, mei_wdt_release);
> +
> +	return ret;
> +}
> +
> +static int mei_wdt_remove(struct mei_cl_device *cldev)
> +{
> +	struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
> +
> +	watchdog_unregister_device(&wdt->wdd);
> +
> +	kref_put(&wdt->refcnt, mei_wdt_release);
> +
> +	mei_cldev_disable(cldev);
> +
> +	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");
>

[char-misc-next 4/7] watchdog: mei_wdt: add status debugfs entry

[Tomas Winkler]

Add entry for dumping current watchdog internal state

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/watchdog/mei_wdt.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
index 3c97deb70d90..a3f1c1613c32 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/watchdog.h>
+#include <linux/debugfs.h>
 
 #include <linux/uuid.h>
 #include <linux/mei_cl_bus.h>
@@ -54,6 +55,19 @@ 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 - mei watchdog driver
  * @wdd: watchdog device
@@ -62,6 +76,8 @@ enum mei_wdt_state {
  * @state: watchdog internal state
  * @cldev: mei watchdog client device
  * @timeout: watchdog current timeout
+ *
+ * @dbgfs_dir: debugfs dir entry
  */
 struct mei_wdt {
 	struct watchdog_device wdd;
@@ -70,6 +86,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 */
 };
 
 /*
@@ -296,6 +316,66 @@ static void mei_wdt_ops_unref(struct watchdog_device *wdd)
 	kref_put(&wdt->refcnt, mei_wdt_release);
 }
 
+#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_deregister(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_deregister(wdt);
+	return -ENODEV;
+}
+
+#else
+
+static inline void dbgfs_deregister(struct mei_wdt *wdt) {}
+
+static inline int dbgfs_register(struct mei_wdt *wdt)
+{
+	return 0;
+}
+#endif /* CONFIG_DEBUG_FS */
+
+
 static const struct watchdog_ops wd_ops = {
 	.owner       = THIS_MODULE,
 	.start       = mei_wdt_ops_start,
@@ -348,6 +428,8 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
 	if (ret)
 		goto err_disable;
 
+	dbgfs_register(wdt);
+
 	return 0;
 
 err_disable:
@@ -367,6 +449,8 @@ static int mei_wdt_remove(struct mei_cl_device *cldev)
 
 	kref_put(&wdt->refcnt, mei_wdt_release);
 
+	dbgfs_deregister(wdt);
+
 	mei_cldev_disable(cldev);
 
 	return 0;
-- 
2.4.3

Re: [char-misc-next 4/7] watchdog: mei_wdt: add status debugfs entry

[Guenter Roeck]

On 12/17/2015 06:49 AM, Tomas Winkler wrote:
> Add entry for dumping current watchdog internal state
>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>   drivers/watchdog/mei_wdt.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 84 insertions(+)
>
> diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
> index 3c97deb70d90..a3f1c1613c32 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/watchdog.h>
> +#include <linux/debugfs.h>
>
>   #include <linux/uuid.h>
>   #include <linux/mei_cl_bus.h>
> @@ -54,6 +55,19 @@ 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";

Doesn't this cause checkpatch warnings ?

> +	}
> +}
> +#endif /* CONFIG_DEBUG_FS */
> +

Can you move this conditional code into the other #ifdef block below ?

Thanks,
Guenter

>   /**
>    * struct mei_wdt - mei watchdog driver
>    * @wdd: watchdog device
> @@ -62,6 +76,8 @@ enum mei_wdt_state {
>    * @state: watchdog internal state
>    * @cldev: mei watchdog client device
>    * @timeout: watchdog current timeout
> + *
> + * @dbgfs_dir: debugfs dir entry
>    */
>   struct mei_wdt {
>   	struct watchdog_device wdd;
> @@ -70,6 +86,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 */
>   };
>
>   /*
> @@ -296,6 +316,66 @@ static void mei_wdt_ops_unref(struct watchdog_device *wdd)
>   	kref_put(&wdt->refcnt, mei_wdt_release);
>   }
>
> +#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_deregister(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_deregister(wdt);
> +	return -ENODEV;
> +}
> +
> +#else
> +
> +static inline void dbgfs_deregister(struct mei_wdt *wdt) {}
> +
> +static inline int dbgfs_register(struct mei_wdt *wdt)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_DEBUG_FS */
> +
> +
>   static const struct watchdog_ops wd_ops = {
>   	.owner       = THIS_MODULE,
>   	.start       = mei_wdt_ops_start,
> @@ -348,6 +428,8 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
>   	if (ret)
>   		goto err_disable;
>
> +	dbgfs_register(wdt);
> +
>   	return 0;
>
>   err_disable:
> @@ -367,6 +449,8 @@ static int mei_wdt_remove(struct mei_cl_device *cldev)
>
>   	kref_put(&wdt->refcnt, mei_wdt_release);
>
> +	dbgfs_deregister(wdt);
> +

This should probably come before the call to kref_put() since it uses wdt.

>   	mei_cldev_disable(cldev);
>
>   	return 0;
>

RE: [char-misc-next 4/7] watchdog: mei_wdt: add status debugfs entry

[Winkler, Tomas]

> On 12/17/2015 06:49 AM, Tomas Winkler wrote:
> > Add entry for dumping current watchdog internal state
> >
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> >   drivers/watchdog/mei_wdt.c | 84
> ++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 84 insertions(+)
> >
> > diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
> > index 3c97deb70d90..a3f1c1613c32 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/watchdog.h>
> > +#include <linux/debugfs.h>
> >
> >   #include <linux/uuid.h>
> >   #include <linux/mei_cl_bus.h>
> > @@ -54,6 +55,19 @@ 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";
> 
> Doesn't this cause checkpatch warnings ?
It doesn't and also checkpatch.pl  is just a help tool. 
> 
> > +	}
> > +}
> > +#endif /* CONFIG_DEBUG_FS */
> > +
> 
> Can you move this conditional code into the other #ifdef block below ?

I'm removing the ifdef in the following patch so this is just for this path to not to spill warning the compilation w/o CONFIG_DEBUG_FS set.

	.start       = mei_wdt_ops_start,
> > @@ -348,6 +428,8 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
> >   	if (ret)
> >   		goto err_disable;
> >
> > +	dbgfs_register(wdt);
> > +
> >   	return 0;
> >
> >   err_disable:
> > @@ -367,6 +449,8 @@ static int mei_wdt_remove(struct mei_cl_device *cldev)
> >
> >   	kref_put(&wdt->refcnt, mei_wdt_release);
> >
> > +	dbgfs_deregister(wdt);
> > +
> 
> This should probably come before the call to kref_put() since it uses wdt.

Good catch, will resend.
Tomas

 

Re: [char-misc-next 4/7] watchdog: mei_wdt: add status debugfs entry

[Guenter Roeck]

On 12/20/2015 01:44 AM, Winkler, Tomas wrote:
>
>> On 12/17/2015 06:49 AM, Tomas Winkler wrote:
>>> Add entry for dumping current watchdog internal state
>>>
>>> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
>>> ---
>>>    drivers/watchdog/mei_wdt.c | 84
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 84 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
>>> index 3c97deb70d90..a3f1c1613c32 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/watchdog.h>
>>> +#include <linux/debugfs.h>
>>>
>>>    #include <linux/uuid.h>
>>>    #include <linux/mei_cl_bus.h>
>>> @@ -54,6 +55,19 @@ 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";
>>
>> Doesn't this cause checkpatch warnings ?
> It doesn't and also checkpatch.pl  is just a help tool.

I should have said "Doesn't this violate the single-statement-per-line"
coding style rule, but I guess then you'd argue that "case xxx:" is not
a statement.

Guenter

RE: [char-misc-next 4/7] watchdog: mei_wdt: add status debugfs entry

[Winkler, Tomas]

> >>> +#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";
> >>
> >> Doesn't this cause checkpatch warnings ?
> > It doesn't and also checkpatch.pl  is just a help tool.
> 
> I should have said "Doesn't this violate the single-statement-per-line"
> coding style rule, but I guess then you'd argue that "case xxx:" is not
> a statement.

I believe it is reasonably readable , but I don't have a strong feeling about it, if you do I can change it.

Tomas 

[char-misc-next v2 5/7] mei: bus: whitelist the watchdog client

[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: 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

[char-misc-next v2 6/7] watchdog: mei_wdt: register wd device only if required

[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.

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2: rework unregistration

 drivers/watchdog/mei_wdt.c | 180 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 167 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
index a3f1c1613c32..00d1b8e630b7 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/watchdog.h>
+#include <linux/completion.h>
 #include <linux/debugfs.h>
 
 #include <linux/uuid.h>
@@ -38,43 +39,54 @@
 
 /* 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) {
+	case MEI_WDT_PROBE: return "PROBE";
 	case MEI_WDT_IDLE: return "IDLE";
 	case MEI_WDT_START: return "START";
 	case MEI_WDT_RUNNING: return "RUNNING";
 	case MEI_WDT_STOPPING: return "STOPPING";
+	case MEI_WDT_NOT_REQUIRED: return "NOT_REQUIRED";
 	default: return "unknown";
 	}
 }
-#endif /* CONFIG_DEBUG_FS */
 
 /**
  * struct mei_wdt - mei watchdog driver
  * @wdd: watchdog device
  * @refcnt: watchdog device reference counter
+ * @reg_lock: watchdog device registration lock
+ * @is_registered: is watchdog device registered
  *
- * @state: watchdog internal state
  * @cldev: mei watchdog client device
+ * @state: watchdog internal state
+ * @resp_required: ping required response
+ * @response: ping response
  * @timeout: watchdog current timeout
  *
  * @dbgfs_dir: debugfs dir entry
@@ -82,9 +94,13 @@ static const char *mei_wdt_state_str(enum mei_wdt_state state)
 struct mei_wdt {
 	struct watchdog_device wdd;
 	struct kref refcnt;
+	struct mutex reg_lock;
+	bool   is_registered;
 
 	struct mei_cl_device *cldev;
 	enum mei_wdt_state state;
+	bool resp_required;
+	struct completion response;
 	u16 timeout;
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
@@ -121,6 +137,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
@@ -192,6 +221,23 @@ static void mei_wdt_release(struct kref *ref)
 }
 
 /**
+ * mei_wdt_unregister - unregister from the watchdog subsystem
+ *
+ * @wdt: mei watchdog device
+ */
+static void mei_wdt_unregister(struct mei_wdt *wdt)
+{
+
+	mutex_lock(&wdt->reg_lock);
+	if (wdt->is_registered) {
+		clear_bit(WDOG_ACTIVE, &wdt->wdd.status);
+		watchdog_unregister_device(&wdt->wdd);
+		wdt->is_registered = false;
+	}
+	mutex_unlock(&wdt->reg_lock);
+}
+
+/**
  * mei_wdt_register - register with the watchdog subsystem
  *
  * @wdt: mei watchdog device
@@ -205,15 +251,20 @@ static int mei_wdt_register(struct mei_wdt *wdt)
 
 	dev = &wdt->cldev->dev;
 
-	watchdog_set_drvdata(&wdt->wdd, wdt);
+	wdt->state = MEI_WDT_IDLE;
 
+	mutex_lock(&wdt->reg_lock);
+	clear_bit(WDOG_UNREGISTERED, &wdt->wdd.status);
+	watchdog_set_drvdata(&wdt->wdd, wdt);
 	ret = watchdog_register_device(&wdt->wdd);
 	if (ret) {
 		dev_err(dev, "unable to register watchdog device = %d.\n", ret);
-		return ret;
+		watchdog_set_drvdata(&wdt->wdd, NULL);
 	}
+	wdt->is_registered = true;
+	mutex_unlock(&wdt->reg_lock);
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -259,6 +310,83 @@ static int mei_wdt_ops_stop(struct watchdog_device *wdd)
 }
 
 /**
+ * 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)
+		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);
+}
+
+/**
  * mei_wdt_ops_ping - wd ping command from the watchdog core.
  *
  * @wdd: watchdog device
@@ -273,13 +401,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;
 }
 
 /**
@@ -405,8 +538,13 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
 
 	kref_init(&wdt->refcnt);
 	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);
+	wdt->is_registered = false;
+	init_completion(&wdt->response);
+
 	mei_cldev_set_drvdata(cldev, wdt);
 
 	ret = mei_cldev_enable(cldev);
@@ -415,6 +553,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),
+					  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);
 
 	wdt->wdd.info = &wd_info;
@@ -424,8 +569,13 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
 	wdt->wdd.min_timeout = MEI_WDT_MIN_TIMEOUT;
 	wdt->wdd.max_timeout = MEI_WDT_MAX_TIMEOUT;
 
-	ret = mei_wdt_register(wdt);
-	if (ret)
+	/* register after ping response */
+	if (wdt->resp_required)
+		ret = mei_wdt_ping(wdt);
+	else
+		ret = mei_wdt_register(wdt);
+
+	if (ret < 0)
 		goto err_disable;
 
 	dbgfs_register(wdt);
@@ -445,7 +595,11 @@ static int mei_wdt_remove(struct mei_cl_device *cldev)
 {
 	struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
 
-	watchdog_unregister_device(&wdt->wdd);
+	/* Free the caller in case of fw initiated or unexpected reset */
+	if (!completion_done(&wdt->response))
+		complete(&wdt->response);
+
+	mei_wdt_unregister(wdt);
 
 	kref_put(&wdt->refcnt, mei_wdt_release);
 
@@ -460,7 +614,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

Re: [char-misc-next v2 6/7] watchdog: mei_wdt: register wd device only if required

[Guenter Roeck]

On 12/17/2015 06:49 AM, 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.
>
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
> V2: rework unregistration
>
>   drivers/watchdog/mei_wdt.c | 180 +++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 167 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
> index a3f1c1613c32..00d1b8e630b7 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/watchdog.h>
> +#include <linux/completion.h>
>   #include <linux/debugfs.h>
>
>   #include <linux/uuid.h>
> @@ -38,43 +39,54 @@
>
>   /* 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) {
> +	case MEI_WDT_PROBE: return "PROBE";
>   	case MEI_WDT_IDLE: return "IDLE";
>   	case MEI_WDT_START: return "START";
>   	case MEI_WDT_RUNNING: return "RUNNING";
>   	case MEI_WDT_STOPPING: return "STOPPING";
> +	case MEI_WDT_NOT_REQUIRED: return "NOT_REQUIRED";
>   	default: return "unknown";
>   	}
>   }
> -#endif /* CONFIG_DEBUG_FS */
>
>   /**
>    * struct mei_wdt - mei watchdog driver
>    * @wdd: watchdog device
>    * @refcnt: watchdog device reference counter
> + * @reg_lock: watchdog device registration lock
> + * @is_registered: is watchdog device registered
>    *
> - * @state: watchdog internal state
>    * @cldev: mei watchdog client device
> + * @state: watchdog internal state
> + * @resp_required: ping required response
> + * @response: ping response
>    * @timeout: watchdog current timeout
>    *
>    * @dbgfs_dir: debugfs dir entry
> @@ -82,9 +94,13 @@ static const char *mei_wdt_state_str(enum mei_wdt_state state)
>   struct mei_wdt {
>   	struct watchdog_device wdd;
>   	struct kref refcnt;
> +	struct mutex reg_lock;
> +	bool   is_registered;
>
>   	struct mei_cl_device *cldev;
>   	enum mei_wdt_state state;
> +	bool resp_required;
> +	struct completion response;
>   	u16 timeout;
>
>   #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -121,6 +137,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
> @@ -192,6 +221,23 @@ static void mei_wdt_release(struct kref *ref)
>   }
>
>   /**
> + * mei_wdt_unregister - unregister from the watchdog subsystem
> + *
> + * @wdt: mei watchdog device
> + */
> +static void mei_wdt_unregister(struct mei_wdt *wdt)
> +{
> +
> +	mutex_lock(&wdt->reg_lock);
> +	if (wdt->is_registered) {
> +		clear_bit(WDOG_ACTIVE, &wdt->wdd.status);

WDOG_ACTIVE is commonly handled by the core. Why does it have to be touched here ?

I am a bit concerned about the entire sequence.

If the following happens

- watchdog character device is opened
- watchdog goes away
- watchdog reappears
- watchdog character device is accessed

the code may access the driver through an old instance of the watchdog
character device. The behavior in this situation is pretty much undefined.
It may work, but only by luck, not by design.

> +		watchdog_unregister_device(&wdt->wdd);
> +		wdt->is_registered = false;
> +	}
> +	mutex_unlock(&wdt->reg_lock);
> +}
> +
> +/**
>    * mei_wdt_register - register with the watchdog subsystem
>    *
>    * @wdt: mei watchdog device
> @@ -205,15 +251,20 @@ static int mei_wdt_register(struct mei_wdt *wdt)
>
>   	dev = &wdt->cldev->dev;
>
> -	watchdog_set_drvdata(&wdt->wdd, wdt);
> +	wdt->state = MEI_WDT_IDLE;
>
> +	mutex_lock(&wdt->reg_lock);
> +	clear_bit(WDOG_UNREGISTERED, &wdt->wdd.status);

Please don't touch watchdog core internal states - this flag will likely
go away soon.

> +	watchdog_set_drvdata(&wdt->wdd, wdt);
>   	ret = watchdog_register_device(&wdt->wdd);
>   	if (ret) {
>   		dev_err(dev, "unable to register watchdog device = %d.\n", ret);
> -		return ret;
> +		watchdog_set_drvdata(&wdt->wdd, NULL);
>   	}
> +	wdt->is_registered = true;
> +	mutex_unlock(&wdt->reg_lock);
>
> -	return 0;
> +	return ret;
>   }
>
>   /**
> @@ -259,6 +310,83 @@ static int mei_wdt_ops_stop(struct watchdog_device *wdd)
>   }
>
>   /**
> + * 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)
> +		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);
> +}
> +
> +/**
>    * mei_wdt_ops_ping - wd ping command from the watchdog core.
>    *
>    * @wdd: watchdog device
> @@ -273,13 +401,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;
>   }
>
>   /**
> @@ -405,8 +538,13 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
>
>   	kref_init(&wdt->refcnt);
>   	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);
> +	wdt->is_registered = false;
> +	init_completion(&wdt->response);
> +
>   	mei_cldev_set_drvdata(cldev, wdt);
>
>   	ret = mei_cldev_enable(cldev);
> @@ -415,6 +553,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),
> +					  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);
>
>   	wdt->wdd.info = &wd_info;
> @@ -424,8 +569,13 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
>   	wdt->wdd.min_timeout = MEI_WDT_MIN_TIMEOUT;
>   	wdt->wdd.max_timeout = MEI_WDT_MAX_TIMEOUT;
>
> -	ret = mei_wdt_register(wdt);
> -	if (ret)
> +	/* register after ping response */
> +	if (wdt->resp_required)
> +		ret = mei_wdt_ping(wdt);
> +	else
> +		ret = mei_wdt_register(wdt);
> +
> +	if (ret < 0)
>   		goto err_disable;
>
>   	dbgfs_register(wdt);
> @@ -445,7 +595,11 @@ static int mei_wdt_remove(struct mei_cl_device *cldev)
>   {
>   	struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
>
> -	watchdog_unregister_device(&wdt->wdd);
> +	/* Free the caller in case of fw initiated or unexpected reset */
> +	if (!completion_done(&wdt->response))
> +		complete(&wdt->response);
> +
> +	mei_wdt_unregister(wdt);
>
>   	kref_put(&wdt->refcnt, mei_wdt_release);
>
> @@ -460,7 +614,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 */
>   	{ }
>   };
>

RE: [char-misc-next v2 6/7] watchdog: mei_wdt: register wd device only if required

[Winkler, Tomas]

> 
> On 12/17/2015 06:49 AM, 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.
> >
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> > V2: rework unregistration
> >
> >   drivers/watchdog/mei_wdt.c | 180
> +++++++++++++++++++++++++++++++++++++++++----
> >   1 file changed, 167 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
> > index a3f1c1613c32..00d1b8e630b7 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/watchdog.h>
> > +#include <linux/completion.h>
> >   #include <linux/debugfs.h>
> >
> >   #include <linux/uuid.h>
> > @@ -38,43 +39,54 @@
> >
> >   /* 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) {
> > +	case MEI_WDT_PROBE: return "PROBE";
> >   	case MEI_WDT_IDLE: return "IDLE";
> >   	case MEI_WDT_START: return "START";
> >   	case MEI_WDT_RUNNING: return "RUNNING";
> >   	case MEI_WDT_STOPPING: return "STOPPING";
> > +	case MEI_WDT_NOT_REQUIRED: return "NOT_REQUIRED";
> >   	default: return "unknown";
> >   	}
> >   }
> > -#endif /* CONFIG_DEBUG_FS */
> >
> >   /**
> >    * struct mei_wdt - mei watchdog driver
> >    * @wdd: watchdog device
> >    * @refcnt: watchdog device reference counter
> > + * @reg_lock: watchdog device registration lock
> > + * @is_registered: is watchdog device registered
> >    *
> > - * @state: watchdog internal state
> >    * @cldev: mei watchdog client device
> > + * @state: watchdog internal state
> > + * @resp_required: ping required response
> > + * @response: ping response
> >    * @timeout: watchdog current timeout
> >    *
> >    * @dbgfs_dir: debugfs dir entry
> > @@ -82,9 +94,13 @@ static const char *mei_wdt_state_str(enum
> mei_wdt_state state)
> >   struct mei_wdt {
> >   	struct watchdog_device wdd;
> >   	struct kref refcnt;
> > +	struct mutex reg_lock;
> > +	bool   is_registered;
> >
> >   	struct mei_cl_device *cldev;
> >   	enum mei_wdt_state state;
> > +	bool resp_required;
> > +	struct completion response;
> >   	u16 timeout;
> >
> >   #if IS_ENABLED(CONFIG_DEBUG_FS)
> > @@ -121,6 +137,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
> > @@ -192,6 +221,23 @@ static void mei_wdt_release(struct kref *ref)
> >   }
> >
> >   /**
> > + * mei_wdt_unregister - unregister from the watchdog subsystem
> > + *
> > + * @wdt: mei watchdog device
> > + */
> > +static void mei_wdt_unregister(struct mei_wdt *wdt)
> > +{
> > +
> > +	mutex_lock(&wdt->reg_lock);
> > +	if (wdt->is_registered) {
> > +		clear_bit(WDOG_ACTIVE, &wdt->wdd.status);
> 
> WDOG_ACTIVE is commonly handled by the core. Why does it have to be
> touched here ?

This have to be cleaned otherwise we ops->start is won't be called on new open()

> I am a bit concerned about the entire sequence.
> 
> If the following happens
> 
> - watchdog character device is opened
> - watchdog goes away
> - watchdog reappears
> - watchdog character device is accessed


> the code may access the driver through an old instance of the watchdog
> character device. The behavior in this situation is pretty much undefined.
> It may work, but only by luck, not by design.

You won't get through as the file descriptor is stale, you have to open the device again.
This should work by design, you have pluggable devices.

> 
> > +		watchdog_unregister_device(&wdt->wdd);
> > +		wdt->is_registered = false;
> > +	}
> > +	mutex_unlock(&wdt->reg_lock);
> > +}
> > +
> > +/**
> >    * mei_wdt_register - register with the watchdog subsystem
> >    *
> >    * @wdt: mei watchdog device
> > @@ -205,15 +251,20 @@ static int mei_wdt_register(struct mei_wdt *wdt)
> >
> >   	dev = &wdt->cldev->dev;
> >
> > -	watchdog_set_drvdata(&wdt->wdd, wdt);
> > +	wdt->state = MEI_WDT_IDLE;
> >
> > +	mutex_lock(&wdt->reg_lock);
> > +	clear_bit(WDOG_UNREGISTERED, &wdt->wdd.status);
> 
> Please don't touch watchdog core internal states 

No choice here. This is a bit tricky bit. This is not cleared, by the core as fresh watchdog device upon registering 
I think in the overall design it would be cleaner if this flag would have positive logic. i.e.  WDOG_REGISTERED. 
The code would be simpler if I would be able just toggle this bit, but there is no event propagated to the user space on this change, so 
I prefer to remove the whole device as this is something already supported..

- this flag will likely
> go away soon

We will have to deal with that when this happens. 
Tomas

Re: [char-misc-next v2 6/7] watchdog: mei_wdt: register wd device only if required

[Guenter Roeck]

On 12/20/2015 04:23 AM, Winkler, Tomas wrote:
>>
>> On 12/17/2015 06:49 AM, 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.
>>>
>>> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
>>> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
>>> ---
>>> V2: rework unregistration
>>>
>>>    drivers/watchdog/mei_wdt.c | 180
>> +++++++++++++++++++++++++++++++++++++++++----
>>>    1 file changed, 167 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
>>> index a3f1c1613c32..00d1b8e630b7 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/watchdog.h>
>>> +#include <linux/completion.h>
>>>    #include <linux/debugfs.h>
>>>
>>>    #include <linux/uuid.h>
>>> @@ -38,43 +39,54 @@
>>>
>>>    /* 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) {
>>> +	case MEI_WDT_PROBE: return "PROBE";
>>>    	case MEI_WDT_IDLE: return "IDLE";
>>>    	case MEI_WDT_START: return "START";
>>>    	case MEI_WDT_RUNNING: return "RUNNING";
>>>    	case MEI_WDT_STOPPING: return "STOPPING";
>>> +	case MEI_WDT_NOT_REQUIRED: return "NOT_REQUIRED";
>>>    	default: return "unknown";
>>>    	}
>>>    }
>>> -#endif /* CONFIG_DEBUG_FS */
>>>
>>>    /**
>>>     * struct mei_wdt - mei watchdog driver
>>>     * @wdd: watchdog device
>>>     * @refcnt: watchdog device reference counter
>>> + * @reg_lock: watchdog device registration lock
>>> + * @is_registered: is watchdog device registered
>>>     *
>>> - * @state: watchdog internal state
>>>     * @cldev: mei watchdog client device
>>> + * @state: watchdog internal state
>>> + * @resp_required: ping required response
>>> + * @response: ping response
>>>     * @timeout: watchdog current timeout
>>>     *
>>>     * @dbgfs_dir: debugfs dir entry
>>> @@ -82,9 +94,13 @@ static const char *mei_wdt_state_str(enum
>> mei_wdt_state state)
>>>    struct mei_wdt {
>>>    	struct watchdog_device wdd;
>>>    	struct kref refcnt;
>>> +	struct mutex reg_lock;
>>> +	bool   is_registered;
>>>
>>>    	struct mei_cl_device *cldev;
>>>    	enum mei_wdt_state state;
>>> +	bool resp_required;
>>> +	struct completion response;
>>>    	u16 timeout;
>>>
>>>    #if IS_ENABLED(CONFIG_DEBUG_FS)
>>> @@ -121,6 +137,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
>>> @@ -192,6 +221,23 @@ static void mei_wdt_release(struct kref *ref)
>>>    }
>>>
>>>    /**
>>> + * mei_wdt_unregister - unregister from the watchdog subsystem
>>> + *
>>> + * @wdt: mei watchdog device
>>> + */
>>> +static void mei_wdt_unregister(struct mei_wdt *wdt)
>>> +{
>>> +
>>> +	mutex_lock(&wdt->reg_lock);
>>> +	if (wdt->is_registered) {
>>> +		clear_bit(WDOG_ACTIVE, &wdt->wdd.status);
>>
>> WDOG_ACTIVE is commonly handled by the core. Why does it have to be
>> touched here ?
>
> This have to be cleaned otherwise we ops->start is won't be called on new open()
>
>> I am a bit concerned about the entire sequence.
>>
>> If the following happens
>>
>> - watchdog character device is opened
>> - watchdog goes away
>> - watchdog reappears
>> - watchdog character device is accessed
>
>
>> the code may access the driver through an old instance of the watchdog
>> character device. The behavior in this situation is pretty much undefined.
>> It may work, but only by luck, not by design.
>
> You won't get through as the file descriptor is stale, you have to open the device again.
> This should work by design, you have pluggable devices.

It can definitely happen today without the third step, ie

- watchdog is unregistered
- watchdog character device is accessed

is possible, making the ref/unref operations necessary, and the reason for
introducing WDOG_UNREGISTERED in the first place. See commit e907df327252.

I actually tested that condition. It _can_ happen.

>
>>
>>> +		watchdog_unregister_device(&wdt->wdd);
>>> +		wdt->is_registered = false;
>>> +	}
>>> +	mutex_unlock(&wdt->reg_lock);
>>> +}
>>> +
>>> +/**
>>>     * mei_wdt_register - register with the watchdog subsystem
>>>     *
>>>     * @wdt: mei watchdog device
>>> @@ -205,15 +251,20 @@ static int mei_wdt_register(struct mei_wdt *wdt)
>>>
>>>    	dev = &wdt->cldev->dev;
>>>
>>> -	watchdog_set_drvdata(&wdt->wdd, wdt);
>>> +	wdt->state = MEI_WDT_IDLE;
>>>
>>> +	mutex_lock(&wdt->reg_lock);
>>> +	clear_bit(WDOG_UNREGISTERED, &wdt->wdd.status);
>>
>> Please don't touch watchdog core internal states
>
> No choice here. This is a bit tricky bit. This is not cleared, by the core as fresh watchdog device upon registering
> I think in the overall design it would be cleaner if this flag would have positive logic. i.e.  WDOG_REGISTERED.
> The code would be simpler if I would be able just toggle this bit, but there is no event propagated to the user space on this change, so
> I prefer to remove the whole device as this is something already supported..
>
> - this flag will likely
>> go away soon
>
> We will have to deal with that when this happens.

Consider the case where the watchdog was removed, but the character device
(or misc device) file descriptor is still open. Now you clear this flag.
User space accesses the "stale" file descriptor right after your "clear" operation.
Since the flag is no longer set, the watchdog core doesn't know anymore that the
watchdog device is unregistered, and calls the driver functions, even though
the watchdog is still unregistered.

I don't know what happens if the watchdog is re-registered using the same character
device data structure, ie after cdev_init() is called on it again. Maybe a file access
using the "old" file descriptor is then rejected by the kernel, and it doesn't find
its way into the driver. Maybe I am missing it, but I don't immediately see where
the kernel would detect that situation. Instead, it seems to me that the kernel
holds a reference to the file operations until the device is closed/released.
And how would that close operation happen unless the kernel actually calls the fops
release function ? After all, the close operation _does_ have to call the watchdog
core, since the watchdog core calls module_put() as well as kref_put(), and clears
WDOG_DEV_OPEN.

Did you actually test that case ? That would actually be interesting to know for sure.

Another interesting case occurs after you unregistered the watchdog device, but
the character device is still open. Your code clears WATCHDOG_ACTIVE, meaning
the watchdog core will believe that the watchdog isn't running and won't even check
for other flags such as WDOG_ALLOW_RELEASE, or call the stop function, but will
just close the watchdog device unconditionally. That may well work with your driver,
but again relies on internal functionality and may have unexpected side effects
(such as that the WDOG_ALLOW_RELEASE flag won't be cleared).

Yet another interesting case occurs if the character device is still open after
the watchdog was unregistererd and re-registered. In that case, WDOG_DEV_OPEN
will still be set, so it will be impossible to open the watchdog device until
the old instance is closed. How would that close actually happen ? Do you rely
on the kernel detecting that the fd is stale when user space tries to use it,
and on calling the release function at that time ?

Either case, with the current watchdog core, there is no guarantee that 'struct
watchdog_device' is no longer in use by the watchdog core until the unref function
has been called. Until then, the watchdog core still "owns" the structure, and
you can not re-use it.

I am working on a set of patches to remedy that situation. With those patches,
the watchdog core will no longer use struct watchdog_device after the call to
watchdog_unregister_device, and you will be able to re-use it immediately.
But we are not there yet.

Overall, it may be better to allocate a new instance of struct watchdog_device
whenever you re-register the watchdog, to avoid all those problems. Sorry that
I didn't realize that earlier.

Guenter

[char-misc-next v2 7/7] watchdog: mei_wdt: re-register device on event

[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.
In the same manner the feature can be deactivated without reboot
in that case the watchdog device should be unregistered.

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2: rework un/registration in runtime

 drivers/watchdog/mei_wdt.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
index 00d1b8e630b7..cd79920c473b 100644
--- a/drivers/watchdog/mei_wdt.c
+++ b/drivers/watchdog/mei_wdt.c
@@ -87,6 +87,7 @@ static const char *mei_wdt_state_str(enum mei_wdt_state state)
  * @state: watchdog internal state
  * @resp_required: ping required response
  * @response: ping response
+ * @unregister: unregister worker
  * @timeout: watchdog current timeout
  *
  * @dbgfs_dir: debugfs dir entry
@@ -101,6 +102,7 @@ struct mei_wdt {
 	enum mei_wdt_state state;
 	bool resp_required;
 	struct completion response;
+	struct work_struct unregister;
 	u16 timeout;
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
@@ -237,6 +239,13 @@ static void mei_wdt_unregister(struct mei_wdt *wdt)
 	mutex_unlock(&wdt->reg_lock);
 }
 
+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_register - register with the watchdog subsystem
  *
@@ -350,8 +359,13 @@ static void mei_wdt_event_rx(struct mei_cl_device *cldev)
 		return;
 	}
 
-	if (wdt->state == MEI_WDT_RUNNING)
+	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) {
@@ -372,6 +386,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
  *
@@ -384,6 +413,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);
 }
 
 /**
@@ -543,6 +575,7 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
 	wdt->resp_required = mei_cldev_ver(cldev) > 0x1;
 	mutex_init(&wdt->reg_lock);
 	wdt->is_registered = false;
+	INIT_WORK(&wdt->unregister, mei_wdt_unregister_work);
 	init_completion(&wdt->response);
 
 	mei_cldev_set_drvdata(cldev, wdt);
@@ -553,9 +586,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;
 	}
@@ -599,6 +636,8 @@ static int mei_wdt_remove(struct mei_cl_device *cldev)
 	if (!completion_done(&wdt->response))
 		complete(&wdt->response);
 
+	cancel_work_sync(&wdt->unregister);
+
 	mei_wdt_unregister(wdt);
 
 	kref_put(&wdt->refcnt, mei_wdt_release);
-- 
2.4.3

Re: [char-misc-next v2 7/7] watchdog: mei_wdt: re-register device on event

[Guenter Roeck]

On 12/17/2015 06:49 AM, Tomas Winkler wrote:
> 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.
> In the same manner the feature can be deactivated without reboot
> in that case the watchdog device should be unregistered.
>
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---

I am not really happy about the watchdog device appearing and disappearing
dynamically. This wreaks havoc with any standard watchdog application.

Isn't there a better way to handle this ? How about just registering the
watchdog device and return an error in the access functions if it is disabled ?

Thanks,
Guenter

> V2: rework un/registration in runtime
>
>   drivers/watchdog/mei_wdt.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
> index 00d1b8e630b7..cd79920c473b 100644
> --- a/drivers/watchdog/mei_wdt.c
> +++ b/drivers/watchdog/mei_wdt.c
> @@ -87,6 +87,7 @@ static const char *mei_wdt_state_str(enum mei_wdt_state state)
>    * @state: watchdog internal state
>    * @resp_required: ping required response
>    * @response: ping response
> + * @unregister: unregister worker
>    * @timeout: watchdog current timeout
>    *
>    * @dbgfs_dir: debugfs dir entry
> @@ -101,6 +102,7 @@ struct mei_wdt {
>   	enum mei_wdt_state state;
>   	bool resp_required;
>   	struct completion response;
> +	struct work_struct unregister;
>   	u16 timeout;
>
>   #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -237,6 +239,13 @@ static void mei_wdt_unregister(struct mei_wdt *wdt)
>   	mutex_unlock(&wdt->reg_lock);
>   }
>
> +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_register - register with the watchdog subsystem
>    *
> @@ -350,8 +359,13 @@ static void mei_wdt_event_rx(struct mei_cl_device *cldev)
>   		return;
>   	}
>
> -	if (wdt->state == MEI_WDT_RUNNING)
> +	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) {
> @@ -372,6 +386,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
>    *
> @@ -384,6 +413,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);
>   }
>
>   /**
> @@ -543,6 +575,7 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
>   	wdt->resp_required = mei_cldev_ver(cldev) > 0x1;
>   	mutex_init(&wdt->reg_lock);
>   	wdt->is_registered = false;
> +	INIT_WORK(&wdt->unregister, mei_wdt_unregister_work);
>   	init_completion(&wdt->response);
>
>   	mei_cldev_set_drvdata(cldev, wdt);
> @@ -553,9 +586,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;
>   	}
> @@ -599,6 +636,8 @@ static int mei_wdt_remove(struct mei_cl_device *cldev)
>   	if (!completion_done(&wdt->response))
>   		complete(&wdt->response);
>
> +	cancel_work_sync(&wdt->unregister);
> +
>   	mei_wdt_unregister(wdt);
>
>   	kref_put(&wdt->refcnt, mei_wdt_release);
>

Re: [char-misc-next v2 7/7] watchdog: mei_wdt: re-register device on event

[One Thousand Gnomes]

> I am not really happy about the watchdog device appearing and disappearing
> dynamically. This wreaks havoc with any standard watchdog application.

Any software that doesn't handle this has been broken for over fifteen
years. We have hotplug PCI and we have PCI watchdog card support. This
isn't a new behaviour to anyone outside the embedded single board space.

> Isn't there a better way to handle this ? How about just registering the
> watchdog device and return an error in the access functions if it is disabled ?

That breaks the existing behaviour of hot pluggable watchdog interfaces
and is different to just about any other device in the kernel. Today with
any desktop or server distribution you can already trivially arrange for
watchdog daemons to start at the point a watchdog is detected dynamically.

Alan

Re: [char-misc-next v2 7/7] watchdog: mei_wdt: re-register device on event

[Guenter Roeck]

On 12/18/2015 09:19 AM, One Thousand Gnomes wrote:
>> I am not really happy about the watchdog device appearing and disappearing
>> dynamically. This wreaks havoc with any standard watchdog application.
>
> Any software that doesn't handle this has been broken for over fifteen
> years. We have hotplug PCI and we have PCI watchdog card support. This
> isn't a new behaviour to anyone outside the embedded single board space.
>
>> Isn't there a better way to handle this ? How about just registering the
>> watchdog device and return an error in the access functions if it is disabled ?
>
> That breaks the existing behaviour of hot pluggable watchdog interfaces
> and is different to just about any other device in the kernel. Today with
> any desktop or server distribution you can already trivially arrange for
> watchdog daemons to start at the point a watchdog is detected dynamically.
>

Ok, you have a point. Wonder if any distributions are doing that, though.
Any idea ?

Guenter

Re: [char-misc-next v2 7/7] watchdog: mei_wdt: re-register device on event

[One Thousand Gnomes]

> > That breaks the existing behaviour of hot pluggable watchdog interfaces
> > and is different to just about any other device in the kernel. Today with
> > any desktop or server distribution you can already trivially arrange for
> > watchdog daemons to start at the point a watchdog is detected dynamically.
> >
> 
> Ok, you have a point. Wonder if any distributions are doing that, though.
> Any idea ?

I don't know for any of the out of the box mainstream ones.

There is a second problem if you register the watchdog but don't actually
have it enabled. Consider the case where a user has the mei watchdog
disabled and a more advanced watchdog card plugged in - in that case the
naïvely implemented existing watchdog app may bind to the non-functional
watchdog, not the correct one.

Alan

RE: [char-misc-next v2 7/7] watchdog: mei_wdt: re-register device on event

[Winkler, Tomas]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1271 bytes --]

> 
> > > That breaks the existing behaviour of hot pluggable watchdog interfaces
> > > and is different to just about any other device in the kernel. Today with
> > > any desktop or server distribution you can already trivially arrange for
> > > watchdog daemons to start at the point a watchdog is detected dynamically.
> > >
> >
> > Ok, you have a point. Wonder if any distributions are doing that, though.
> > Any idea ?
> 
> I don't know for any of the out of the box mainstream ones.
> 
> There is a second problem if you register the watchdog but don't actually
> have it enabled. Consider the case where a user has the mei watchdog
> disabled and a more advanced watchdog card plugged in - in that case the
> naïvely implemented existing watchdog app may bind to the non-functional
> watchdog, not the correct one.

That's correct, though  WDIOF_ALARMONLY or device identity has to be checked. 
And if mei is disabled one would probably hit ENODEV, still there will be no uevent to the user space
when the device is provisioned again, so still I prefer add/removing the watchdog device. 

Thanks
Tomas

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [char-misc-next v2 7/7] watchdog: mei_wdt: re-register device on event

[Winkler, Tomas]

> 
> > > That breaks the existing behaviour of hot pluggable watchdog interfaces
> > > and is different to just about any other device in the kernel. Today with
> > > any desktop or server distribution you can already trivially arrange for
> > > watchdog daemons to start at the point a watchdog is detected dynamically.
> > >
> >
> > Ok, you have a point. Wonder if any distributions are doing that, though.
> > Any idea ?
> 
> I don't know for any of the out of the box mainstream ones.
> 
> There is a second problem if you register the watchdog but don't actually
> have it enabled. Consider the case where a user has the mei watchdog
> disabled and a more advanced watchdog card plugged in - in that case the
> naïvely implemented existing watchdog app may bind to the non-functional
> watchdog, not the correct one.

That's correct, though  WDIOF_ALARMONLY or device identity has to be checked. 
And if mei is disabled one would probably hit ENODEV, still there will be no uevent to the user space
when the device is provisioned again, so still I prefer add/removing the watchdog device. 

Thanks
Tomas