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

[char-misc-next 0/6] 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.

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 (4):
  mei: wd: drop the watchdog code from the core mei driver
  mei: wd: implement MEI iAMT watchdog driver
  mei: wd: register wd device only if required
  mei: wd: re-register device on event

Tomas Winkler (2):
  mei: drop nfc leftovers from the mei driver
  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             | 571 +++++++++++++++++++++++++++++++++
 14 files changed, 632 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 1/6] 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>
---
 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 2/6] 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>
---
 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 3/6] mei: wd: implement MEI iAMT watchdog driver

[Tomas Winkler]

From: Alexander Usyskin <alexander.usyskin@intel.com>

Create a driver with the generic watchdog interface
for the MEI iAMT watchdog device.

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 Documentation/misc-devices/mei/mei.txt |  12 +-
 MAINTAINERS                            |   1 +
 drivers/watchdog/Kconfig               |  15 ++
 drivers/watchdog/Makefile              |   1 +
 drivers/watchdog/mei_wdt.c             | 432 +++++++++++++++++++++++++++++++++
 5 files changed, 455 insertions(+), 6 deletions(-)
 create mode 100644 drivers/watchdog/mei_wdt.c

diff --git a/Documentation/misc-devices/mei/mei.txt b/Documentation/misc-devices/mei/mei.txt
index 91c1fa34f48b..2b80a0cd621f 100644
--- a/Documentation/misc-devices/mei/mei.txt
+++ b/Documentation/misc-devices/mei/mei.txt
@@ -231,15 +231,15 @@ IT knows when a platform crashes even when there is a hard failure on the host.
 The Intel AMT Watchdog is composed of two parts:
 	1) Firmware feature - receives the heartbeats
 	   and sends an event when the heartbeats stop.
-	2) Intel MEI driver - connects to the watchdog feature, configures the
-	   watchdog and sends the heartbeats.
+	2) Intel MEI iAMT watchdog driver - connects to the watchdog feature,
+	   configures the watchdog and sends the heartbeats.
 
-The Intel MEI driver uses the kernel watchdog API to configure the Intel AMT
-Watchdog and to send heartbeats to it. The default timeout of the
+The Intel iAMT watchdog MEI driver uses the kernel watchdog API to configure
+the Intel AMT Watchdog and to send heartbeats to it. The default timeout of the
 watchdog is 120 seconds.
 
-If the Intel AMT Watchdog feature does not exist (i.e. the connection failed),
-the Intel MEI driver will disable the sending of heartbeats.
+If the Intel AMT is not enabled in the firmware then the watchdog client won't enumerate
+on the me client bus and watchdog devices won't be exposed.
 
 
 Supported Chipsets
diff --git a/MAINTAINERS b/MAINTAINERS
index 050d0e77a2cf..cf0a51518f4a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5664,6 +5664,7 @@ S:	Supported
 F:	include/uapi/linux/mei.h
 F:	include/linux/mei_cl_bus.h
 F:	drivers/misc/mei/*
+F:	drivers/watchdog/mei_wdt.c
 F:	Documentation/misc-devices/mei/*
 
 INTEL MIC DRIVERS (mic)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 7a8a6c6952e9..ec584714829d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1154,6 +1154,21 @@ config SBC_EPX_C3_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called sbc_epx_c3.
 
+config INTEL_MEI_WDT
+	tristate "Intel MEI iAMT Watchdog"
+	depends on INTEL_MEI && X86
+	select WATCHDOG_CORE
+	---help---
+	  A device driver for the Intel MEI iAMT watchdog.
+
+	  The Intel AMT Watchdog is an OS Health (Hang/Crash) watchdog.
+	  Whenever the OS hangs or crashes, iAMT will send an event
+	  to any subscriber to this event. The watchdog doesn't reset the
+	  the platform.
+
+	  To compile this driver as a module, choose M here:
+	  the module will be called mei_wdt.
+
 # M32R Architecture
 
 # M68K Architecture
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 53d4827ddfe1..9069c9dd8aa8 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -123,6 +123,7 @@ obj-$(CONFIG_MACHZ_WDT) += machzwd.o
 obj-$(CONFIG_SBC_EPX_C3_WATCHDOG) += sbc_epx_c3.o
 obj-$(CONFIG_INTEL_SCU_WATCHDOG) += intel_scu_watchdog.o
 obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
+obj-$(CONFIG_INTEL_MEI_WDT) += mei_wdt.o
 
 # M32R Architecture
 
diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
new file mode 100644
index 000000000000..149b29f341cf
--- /dev/null
+++ b/drivers/watchdog/mei_wdt.c
@@ -0,0 +1,432 @@
+/*
+ * Intel Management Engine Interface (Intel MEI) Linux driver
+ * Copyright (c) 2015, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/watchdog.h>
+
+#include <linux/uuid.h>
+#include <linux/mei_cl_bus.h>
+
+/*
+ * iAMT Watchdog Device
+ */
+#define INTEL_AMT_WATCHDOG_ID "iamt_wdt"
+
+#define MEI_WDT_DEFAULT_TIMEOUT   120  /* seconds */
+#define MEI_WDT_MIN_TIMEOUT       120  /* seconds */
+#define MEI_WDT_MAX_TIMEOUT     65535  /* seconds */
+
+/* Commands */
+#define MEI_MANAGEMENT_CONTROL 0x02
+
+/* MEI Management Control version number */
+#define MEI_MC_VERSION_NUMBER  0x10
+
+/* Sub Commands */
+#define MEI_MC_START_WD_TIMER_REQ  0x13
+#define MEI_MC_STOP_WD_TIMER_REQ   0x14
+
+/**
+ * enum mei_wdt_state - internal watchdog state
+ *
+ * @MEI_WDT_IDLE: wd is idle and not opened
+ * @MEI_WDT_START: wd was opened, start was called
+ * @MEI_WDT_RUNNING: wd is expecting keep alive pings
+ * @MEI_WDT_STOPPING: wd is stopping and will move to IDLE
+ */
+enum mei_wdt_state {
+	MEI_WDT_IDLE,
+	MEI_WDT_START,
+	MEI_WDT_RUNNING,
+	MEI_WDT_STOPPING,
+};
+
+struct mei_wdt;
+
+/**
+ * struct mei_wdt_dev - watchdog device wrapper
+ *
+ * @wdd: watchdog device
+ * @wdt: back pointer to mei_wdt driver
+ * @refcnt: reference counter
+ */
+struct mei_wdt_dev {
+	struct watchdog_device wdd;
+	struct mei_wdt *wdt;
+	struct kref refcnt;
+};
+
+/**
+ * struct mei_wdt - mei watchdog driver
+ *
+ * @cldev: mei watchdog client device
+ * @mwd: watchdog device wrapper
+ * @state: watchdog internal state
+ * @timeout: watchdog current timeout
+ */
+struct mei_wdt {
+	struct mei_cl_device *cldev;
+	struct mei_wdt_dev *mwd;
+	enum mei_wdt_state state;
+	u16 timeout;
+};
+
+struct mei_wdt_hdr {
+	u8 command;
+	u8 bytecount;
+	u8 subcommand;
+	u8 versionnumber;
+};
+
+struct mei_wdt_start_request {
+	struct mei_wdt_hdr hdr;
+	u16 timeout;
+	u8 reserved[17];
+} __packed;
+
+struct mei_wdt_stop_request {
+	struct mei_wdt_hdr hdr;
+} __packed;
+
+/**
+ * mei_wdt_ping - send wd start command
+ *
+ * @wdt: mei watchdog device
+ *
+ * Return: number of bytes sent on success,
+ *         negative errno code on failure
+ */
+static int mei_wdt_ping(struct mei_wdt *wdt)
+{
+	struct mei_wdt_start_request req;
+	const size_t req_len = sizeof(req);
+
+	memset(&req, 0, req_len);
+	req.hdr.command = MEI_MANAGEMENT_CONTROL;
+	req.hdr.bytecount = req_len - offsetof(struct mei_wdt_hdr, subcommand);
+	req.hdr.subcommand = MEI_MC_START_WD_TIMER_REQ;
+	req.hdr.versionnumber = MEI_MC_VERSION_NUMBER;
+	req.timeout = wdt->timeout;
+
+	return mei_cldev_send(wdt->cldev, (u8 *)&req, req_len);
+}
+
+/**
+ * mei_wdt_stop - send wd stop command
+ *
+ * @wdt: mei watchdog device
+ *
+ * Return: number of bytes sent on success,
+ *         negative errno code on failure
+ */
+static int mei_wdt_stop(struct mei_wdt *wdt)
+{
+	struct mei_wdt_stop_request req;
+	const size_t req_len = sizeof(req);
+
+	memset(&req, 0, req_len);
+	req.hdr.command = MEI_MANAGEMENT_CONTROL;
+	req.hdr.bytecount = req_len - offsetof(struct mei_wdt_hdr, subcommand);
+	req.hdr.subcommand = MEI_MC_STOP_WD_TIMER_REQ;
+	req.hdr.versionnumber = MEI_MC_VERSION_NUMBER;
+
+	return mei_cldev_send(wdt->cldev, (u8 *)&req, req_len);
+}
+
+/**
+ * mei_wdt_ops_start - wd start command from the watchdog core.
+ *
+ * @wdd: watchdog device
+ *
+ * Return: 0 on success or -ENODEV;
+ */
+static int mei_wdt_ops_start(struct watchdog_device *wdd)
+{
+	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
+
+	if (!mwd)
+		return -ENODEV;
+
+	mwd->wdt->state = MEI_WDT_START;
+	wdd->timeout = mwd->wdt->timeout;
+	return 0;
+}
+
+/**
+ * mei_wdt_ops_stop - wd stop command from the watchdog core.
+ *
+ * @wdd: watchdog device
+ *
+ * Return: 0 if success, negative errno code for failure
+ */
+static int mei_wdt_ops_stop(struct watchdog_device *wdd)
+{
+	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
+	struct mei_wdt *wdt;
+	int ret;
+
+	if (!mwd)
+		return -ENODEV;
+
+	wdt = mwd->wdt;
+
+	if (wdt->state != MEI_WDT_RUNNING)
+		return 0;
+
+	wdt->state = MEI_WDT_STOPPING;
+
+	ret = mei_wdt_stop(wdt);
+	if (ret < 0)
+		return ret;
+
+	wdt->state = MEI_WDT_IDLE;
+
+	return 0;
+}
+
+/**
+ * mei_wdt_ops_ping - wd ping command from the watchdog core.
+ *
+ * @wdd: watchdog device
+ *
+ * Return: 0 if success, negative errno code on failure
+ */
+static int mei_wdt_ops_ping(struct watchdog_device *wdd)
+{
+	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
+	struct mei_wdt *wdt;
+	int ret;
+
+	if (!mwd)
+		return -ENODEV;
+
+	wdt = mwd->wdt;
+
+	if (wdt->state != MEI_WDT_START &&
+	    wdt->state != MEI_WDT_RUNNING)
+		return 0;
+
+	ret = mei_wdt_ping(wdt);
+	if (ret < 0)
+		return ret;
+
+	wdt->state = MEI_WDT_RUNNING;
+
+	return 0;
+}
+
+/**
+ * mei_wdt_ops_set_timeout - wd set timeout command from the watchdog core.
+ *
+ * @wdd: watchdog device
+ * @timeout: timeout value to set
+ *
+ * Return: 0 if success, negative errno code for failure
+ */
+static int mei_wdt_ops_set_timeout(struct watchdog_device *wdd,
+				   unsigned int timeout)
+{
+	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
+	struct mei_wdt *wdt;
+
+	if (!mwd)
+		return -ENODEV;
+
+	wdt = mwd->wdt;
+
+	/* valid value is already checked by the caller */
+	wdt->timeout = timeout;
+	wdd->timeout = timeout;
+
+	return 0;
+}
+
+static void mei_wdt_release(struct kref *ref)
+{
+	struct mei_wdt_dev *mwd = container_of(ref, struct mei_wdt_dev, refcnt);
+
+	kfree(mwd);
+}
+
+static void mei_wdt_ops_ref(struct watchdog_device *wdd)
+{
+	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
+
+	kref_get(&mwd->refcnt);
+}
+
+static void mei_wdt_ops_unref(struct watchdog_device *wdd)
+{
+	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
+
+	kref_put(&mwd->refcnt, mei_wdt_release);
+}
+
+static const struct watchdog_ops wd_ops = {
+	.owner       = THIS_MODULE,
+	.start       = mei_wdt_ops_start,
+	.stop        = mei_wdt_ops_stop,
+	.ping        = mei_wdt_ops_ping,
+	.set_timeout = mei_wdt_ops_set_timeout,
+	.ref         = mei_wdt_ops_ref,
+	.unref       = mei_wdt_ops_unref,
+};
+
+static struct watchdog_info wd_info = {
+	.identity = INTEL_AMT_WATCHDOG_ID,
+	.options  = WDIOF_KEEPALIVEPING |
+		    WDIOF_SETTIMEOUT |
+		    WDIOF_ALARMONLY,
+};
+
+static int mei_wdt_register(struct mei_wdt *wdt)
+{
+	struct mei_wdt_dev *mwd;
+	struct device *dev;
+	int ret;
+
+	if (!wdt || !wdt->cldev)
+		return -EINVAL;
+
+	dev = &wdt->cldev->dev;
+
+	mwd = kzalloc(sizeof(struct mei_wdt_dev), GFP_KERNEL);
+	if (!mwd)
+		return -ENOMEM;
+
+	mwd->wdt = wdt;
+	mwd->wdd.info = &wd_info;
+	mwd->wdd.ops = &wd_ops;
+	mwd->wdd.parent = dev;
+	mwd->wdd.timeout = MEI_WDT_DEFAULT_TIMEOUT;
+	mwd->wdd.min_timeout = MEI_WDT_MIN_TIMEOUT;
+	mwd->wdd.max_timeout = MEI_WDT_MAX_TIMEOUT;
+	kref_init(&mwd->refcnt);
+
+	ret = watchdog_register_device(&mwd->wdd);
+	if (ret) {
+		dev_err(dev, "unable to register watchdog device = %d.\n", ret);
+		kref_put(&mwd->refcnt, mei_wdt_release);
+		return ret;
+	}
+
+	wdt->mwd = mwd;
+	watchdog_set_drvdata(&mwd->wdd, mwd);
+	return 0;
+}
+
+static void mei_wdt_unregister(struct mei_wdt *wdt)
+{
+	if (!wdt->mwd)
+		return;
+
+	watchdog_unregister_device(&wdt->mwd->wdd);
+	kref_put(&wdt->mwd->refcnt, mei_wdt_release);
+	wdt->mwd = NULL;
+}
+
+static int mei_wdt_probe(struct mei_cl_device *cldev,
+			 const struct mei_cl_device_id *id)
+{
+	struct mei_wdt *wdt;
+	int ret;
+
+	wdt = kzalloc(sizeof(struct mei_wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+
+	wdt->timeout = MEI_WDT_DEFAULT_TIMEOUT;
+	wdt->state = MEI_WDT_IDLE;
+	wdt->cldev = cldev;
+	mei_cldev_set_drvdata(cldev, wdt);
+
+	ret = mei_cldev_enable(cldev);
+	if (ret < 0) {
+		dev_err(&cldev->dev, "Could not enable cl device\n");
+		goto err_out;
+	}
+
+	wd_info.firmware_version = mei_cldev_ver(cldev);
+
+	ret = mei_wdt_register(wdt);
+	if (ret)
+		goto err_disable;
+
+	return 0;
+
+err_disable:
+	mei_cldev_disable(cldev);
+
+err_out:
+	kfree(wdt);
+
+	return ret;
+}
+
+static int mei_wdt_remove(struct mei_cl_device *cldev)
+{
+	struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
+
+	mei_cldev_disable(cldev);
+
+	mei_wdt_unregister(wdt);
+
+	kfree(wdt);
+
+	return 0;
+}
+
+#define MEI_UUID_WD UUID_LE(0x05B79A6F, 0x4628, 0x4D7F, \
+			    0x89, 0x9D, 0xA9, 0x15, 0x14, 0xCB, 0x32, 0xAB)
+
+static struct mei_cl_device_id mei_wdt_tbl[] = {
+	{ .uuid = MEI_UUID_WD, .version = MEI_CL_VERSION_ANY},
+	/* required last entry */
+	{ }
+};
+MODULE_DEVICE_TABLE(mei, mei_wdt_tbl);
+
+static struct mei_cl_driver mei_wdt_driver = {
+	.id_table = mei_wdt_tbl,
+	.name = KBUILD_MODNAME,
+
+	.probe = mei_wdt_probe,
+	.remove = mei_wdt_remove,
+};
+
+static int __init mei_wdt_init(void)
+{
+	int ret;
+
+	ret = mei_cldev_driver_register(&mei_wdt_driver);
+	if (ret) {
+		pr_err(KBUILD_MODNAME ": module registration failed\n");
+		return ret;
+	}
+	return 0;
+}
+
+static void __exit mei_wdt_exit(void)
+{
+	mei_cldev_driver_unregister(&mei_wdt_driver);
+}
+
+module_init(mei_wdt_init);
+module_exit(mei_wdt_exit);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Device driver for Intel MEI iAMT watchdog");
-- 
2.4.3

[char-misc-next 4/6] 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>
---
 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 5/6] mei: wd: 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 watchdog interface
can be not registered and eliminate unnecessary pings and hence lower the
power consumption by not waking up the device.

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/watchdog/mei_wdt.c | 127 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 122 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
index 149b29f341cf..47f0dc2e822a 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/uuid.h>
 #include <linux/mei_cl_bus.h>
@@ -37,21 +38,27 @@
 
 /* Sub Commands */
 #define MEI_MC_START_WD_TIMER_REQ  0x13
+#define MEI_MC_START_WD_TIMER_RES  0x83
+#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,
 };
 
 struct mei_wdt;
@@ -75,12 +82,16 @@ struct mei_wdt_dev {
  * @cldev: mei watchdog client device
  * @mwd: watchdog device wrapper
  * @state: watchdog internal state
+ * @resp_required: ping required response
+ * @response: ping response
  * @timeout: watchdog current timeout
  */
 struct mei_wdt {
 	struct mei_cl_device *cldev;
 	struct mei_wdt_dev *mwd;
 	enum mei_wdt_state state;
+	bool resp_required;
+	struct completion response;
 	u16 timeout;
 };
 
@@ -97,10 +108,19 @@ struct mei_wdt_start_request {
 	u8 reserved[17];
 } __packed;
 
+struct mei_wdt_start_response {
+	struct mei_wdt_hdr hdr;
+	u8 status;
+	u8 wdstate;
+} __packed;
+
 struct mei_wdt_stop_request {
 	struct mei_wdt_hdr hdr;
 } __packed;
 
+static void mei_wdt_unregister(struct mei_wdt *wdt);
+static int mei_wdt_register(struct mei_wdt *wdt);
+
 /**
  * mei_wdt_ping - send wd start command
  *
@@ -192,12 +212,88 @@ static int mei_wdt_ops_stop(struct watchdog_device *wdd)
 	if (ret < 0)
 		return ret;
 
-	wdt->state = MEI_WDT_IDLE;
+	if (!wdt->resp_required)
+		wdt->state = MEI_WDT_IDLE;
 
 	return 0;
 }
 
 /**
+ * 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;
+	}
+
+	if (ret == 0) {
+		if (wdt->state == MEI_WDT_STOPPING)
+			wdt->state = MEI_WDT_IDLE;
+		return;
+	}
+
+	if (ret < sizeof(struct mei_wdt_hdr)) {
+		dev_err(&cldev->dev, "recv small data %d\n", ret);
+		return;
+	}
+
+	if (res.hdr.command != MEI_MANAGEMENT_CONTROL ||
+	    res.hdr.subcommand != MEI_MC_START_WD_TIMER_RES ||
+	    res.hdr.versionnumber != MEI_MC_VERSION_NUMBER)
+		return;
+
+	if (wdt->state == MEI_WDT_RUNNING) {
+		if (res.wdstate & MEI_WDT_WDSTATE_NOT_REQUIRED) {
+			wdt->state = MEI_WDT_NOT_REQUIRED;
+			mei_wdt_unregister(wdt);
+		}
+
+		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);
+			wdt->state = MEI_WDT_IDLE;
+			mei_wdt_register(wdt);
+		}
+		return;
+	}
+
+	dev_err(&cldev->dev, "not in running state %d\n", 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
@@ -219,11 +315,17 @@ static int mei_wdt_ops_ping(struct watchdog_device *wdd)
 	    wdt->state != MEI_WDT_RUNNING)
 		return 0;
 
+
+	if (wdt->resp_required)
+		reinit_completion(&wdt->response);
+
+	wdt->state = MEI_WDT_RUNNING;
 	ret = mei_wdt_ping(wdt);
 	if (ret < 0)
 		return ret;
 
-	wdt->state = MEI_WDT_RUNNING;
+	if (wdt->resp_required)
+		wait_for_completion_interruptible(&wdt->response);
 
 	return 0;
 }
@@ -349,8 +451,11 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
 		return -ENOMEM;
 
 	wdt->timeout = MEI_WDT_DEFAULT_TIMEOUT;
-	wdt->state = MEI_WDT_IDLE;
+	wdt->state = MEI_WDT_PROBE;
 	wdt->cldev = cldev;
+	wdt->resp_required = mei_cldev_ver(cldev) > 0x1;
+	init_completion(&wdt->response);
+
 	mei_cldev_set_drvdata(cldev, wdt);
 
 	ret = mei_cldev_enable(cldev);
@@ -361,8 +466,20 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
 
 	wd_info.firmware_version = mei_cldev_ver(cldev);
 
-	ret = mei_wdt_register(wdt);
-	if (ret)
+	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;
+	}
+
+	/* 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;
 
 	return 0;
-- 
2.4.3

[char-misc-next 6/6] mei: wd: 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 register the watchdog device with the kernel.

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/watchdog/mei_wdt.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
index 47f0dc2e822a..ee3b5d12b1b2 100644
--- a/drivers/watchdog/mei_wdt.c
+++ b/drivers/watchdog/mei_wdt.c
@@ -279,6 +279,21 @@ out:
 		complete(&wdt->response);
 }
 
+/*
+ * mei_wdt_event_notif - callback for notification
+ *
+ * @cldev: bus device
+ */
+static void mei_wdt_event_notif(struct mei_cl_device *cldev)
+{
+	struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
+
+	if (wdt->state == MEI_WDT_NOT_REQUIRED) {
+		mei_wdt_register(wdt);
+		wdt->state = MEI_WDT_IDLE;
+	}
+}
+
 /**
  * mei_wdt_event - callback for event receive
  *
@@ -291,6 +306,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_event_notif(cldev);
 }
 
 /**
@@ -466,9 +484,13 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
 
 	wd_info.firmware_version = mei_cldev_ver(cldev);
 
-	ret = mei_cldev_register_event_cb(wdt->cldev, BIT(MEI_CL_EVENT_RX),
+	ret = mei_cldev_register_event_cb(wdt->cldev,
+					  BIT(MEI_CL_EVENT_RX) |
+					  BIT(MEI_CL_EVENT_NOTIF),
 					  mei_wdt_event, NULL);
-	if (ret) {
+
+	/* on legacy devices notification is not supported */
+	if (ret && ret != -EOPNOTSUPP) {
 		dev_err(&cldev->dev, "Could not register event ret=%d\n", ret);
 		goto err_disable;
 	}
-- 
2.4.3

Re: [char-misc-next 3/6] mei: wd: implement MEI iAMT watchdog driver

[Guenter Roeck]

On 11/26/2015 04:31 AM, Tomas Winkler wrote:
> From: Alexander Usyskin <alexander.usyskin@intel.com>
>
> Create a driver with the generic watchdog interface
> for the MEI iAMT watchdog device.
>
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>   Documentation/misc-devices/mei/mei.txt |  12 +-
>   MAINTAINERS                            |   1 +
>   drivers/watchdog/Kconfig               |  15 ++
>   drivers/watchdog/Makefile              |   1 +
>   drivers/watchdog/mei_wdt.c             | 432 +++++++++++++++++++++++++++++++++
>   5 files changed, 455 insertions(+), 6 deletions(-)
>   create mode 100644 drivers/watchdog/mei_wdt.c
>
> diff --git a/Documentation/misc-devices/mei/mei.txt b/Documentation/misc-devices/mei/mei.txt
> index 91c1fa34f48b..2b80a0cd621f 100644
> --- a/Documentation/misc-devices/mei/mei.txt
> +++ b/Documentation/misc-devices/mei/mei.txt
> @@ -231,15 +231,15 @@ IT knows when a platform crashes even when there is a hard failure on the host.
>   The Intel AMT Watchdog is composed of two parts:
>   	1) Firmware feature - receives the heartbeats
>   	   and sends an event when the heartbeats stop.
> -	2) Intel MEI driver - connects to the watchdog feature, configures the
> -	   watchdog and sends the heartbeats.
> +	2) Intel MEI iAMT watchdog driver - connects to the watchdog feature,
> +	   configures the watchdog and sends the heartbeats.
>
> -The Intel MEI driver uses the kernel watchdog API to configure the Intel AMT
> -Watchdog and to send heartbeats to it. The default timeout of the
> +The Intel iAMT watchdog MEI driver uses the kernel watchdog API to configure
> +the Intel AMT Watchdog and to send heartbeats to it. The default timeout of the
>   watchdog is 120 seconds.
>
> -If the Intel AMT Watchdog feature does not exist (i.e. the connection failed),
> -the Intel MEI driver will disable the sending of heartbeats.
> +If the Intel AMT is not enabled in the firmware then the watchdog client won't enumerate
> +on the me client bus and watchdog devices won't be exposed.
>
>
>   Supported Chipsets
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 050d0e77a2cf..cf0a51518f4a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5664,6 +5664,7 @@ S:	Supported
>   F:	include/uapi/linux/mei.h
>   F:	include/linux/mei_cl_bus.h
>   F:	drivers/misc/mei/*
> +F:	drivers/watchdog/mei_wdt.c
>   F:	Documentation/misc-devices/mei/*
>
>   INTEL MIC DRIVERS (mic)
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7a8a6c6952e9..ec584714829d 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1154,6 +1154,21 @@ config SBC_EPX_C3_WATCHDOG
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called sbc_epx_c3.
>
> +config INTEL_MEI_WDT
> +	tristate "Intel MEI iAMT Watchdog"
> +	depends on INTEL_MEI && X86
> +	select WATCHDOG_CORE
> +	---help---
> +	  A device driver for the Intel MEI iAMT watchdog.
> +
> +	  The Intel AMT Watchdog is an OS Health (Hang/Crash) watchdog.
> +	  Whenever the OS hangs or crashes, iAMT will send an event
> +	  to any subscriber to this event. The watchdog doesn't reset the
> +	  the platform.
> +
> +	  To compile this driver as a module, choose M here:
> +	  the module will be called mei_wdt.
> +
>   # M32R Architecture
>
>   # M68K Architecture
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 53d4827ddfe1..9069c9dd8aa8 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -123,6 +123,7 @@ obj-$(CONFIG_MACHZ_WDT) += machzwd.o
>   obj-$(CONFIG_SBC_EPX_C3_WATCHDOG) += sbc_epx_c3.o
>   obj-$(CONFIG_INTEL_SCU_WATCHDOG) += intel_scu_watchdog.o
>   obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
> +obj-$(CONFIG_INTEL_MEI_WDT) += mei_wdt.o
>
>   # M32R Architecture
>
> diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
> new file mode 100644
> index 000000000000..149b29f341cf
> --- /dev/null
> +++ b/drivers/watchdog/mei_wdt.c
> @@ -0,0 +1,432 @@
> +/*
> + * Intel Management Engine Interface (Intel MEI) Linux driver
> + * Copyright (c) 2015, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/watchdog.h>
> +
> +#include <linux/uuid.h>
> +#include <linux/mei_cl_bus.h>
> +
> +/*
> + * iAMT Watchdog Device
> + */
> +#define INTEL_AMT_WATCHDOG_ID "iamt_wdt"
> +
> +#define MEI_WDT_DEFAULT_TIMEOUT   120  /* seconds */
> +#define MEI_WDT_MIN_TIMEOUT       120  /* seconds */

Is the large minimum timeout on purpose ?
Just asking, since it is quite unusual.

> +#define MEI_WDT_MAX_TIMEOUT     65535  /* seconds */
> +
> +/* Commands */
> +#define MEI_MANAGEMENT_CONTROL 0x02
> +
> +/* MEI Management Control version number */
> +#define MEI_MC_VERSION_NUMBER  0x10
> +
> +/* Sub Commands */
> +#define MEI_MC_START_WD_TIMER_REQ  0x13
> +#define MEI_MC_STOP_WD_TIMER_REQ   0x14
> +
> +/**
> + * enum mei_wdt_state - internal watchdog state
> + *
> + * @MEI_WDT_IDLE: wd is idle and not opened
> + * @MEI_WDT_START: wd was opened, start was called
> + * @MEI_WDT_RUNNING: wd is expecting keep alive pings
> + * @MEI_WDT_STOPPING: wd is stopping and will move to IDLE
> + */
> +enum mei_wdt_state {
> +	MEI_WDT_IDLE,
> +	MEI_WDT_START,
> +	MEI_WDT_RUNNING,
> +	MEI_WDT_STOPPING,
> +};
> +
> +struct mei_wdt;
> +
> +/**
> + * struct mei_wdt_dev - watchdog device wrapper
> + *
> + * @wdd: watchdog device
> + * @wdt: back pointer to mei_wdt driver
> + * @refcnt: reference counter
> + */
> +struct mei_wdt_dev {
> +	struct watchdog_device wdd;
> +	struct mei_wdt *wdt;
> +	struct kref refcnt;
> +};
> +
> +/**
> + * struct mei_wdt - mei watchdog driver
> + *
> + * @cldev: mei watchdog client device
> + * @mwd: watchdog device wrapper
> + * @state: watchdog internal state
> + * @timeout: watchdog current timeout
> + */
> +struct mei_wdt {
> +	struct mei_cl_device *cldev;
> +	struct mei_wdt_dev *mwd;
> +	enum mei_wdt_state state;
> +	u16 timeout;
> +};

Any special reason for having two data structures instead of one ?
You could just move the variables from struct mei_wdt_dev into
struct mei_wdt, no ?

> +
> +struct mei_wdt_hdr {
> +	u8 command;
> +	u8 bytecount;
> +	u8 subcommand;
> +	u8 versionnumber;
> +};
> +
> +struct mei_wdt_start_request {
> +	struct mei_wdt_hdr hdr;
> +	u16 timeout;
> +	u8 reserved[17];
> +} __packed;
> +
> +struct mei_wdt_stop_request {
> +	struct mei_wdt_hdr hdr;
> +} __packed;
> +
> +/**
> + * mei_wdt_ping - send wd start command
> + *
> + * @wdt: mei watchdog device
> + *
> + * Return: number of bytes sent on success,
> + *         negative errno code on failure
> + */
> +static int mei_wdt_ping(struct mei_wdt *wdt)
> +{
> +	struct mei_wdt_start_request req;
> +	const size_t req_len = sizeof(req);
> +
> +	memset(&req, 0, req_len);
> +	req.hdr.command = MEI_MANAGEMENT_CONTROL;
> +	req.hdr.bytecount = req_len - offsetof(struct mei_wdt_hdr, subcommand);
> +	req.hdr.subcommand = MEI_MC_START_WD_TIMER_REQ;
> +	req.hdr.versionnumber = MEI_MC_VERSION_NUMBER;
> +	req.timeout = wdt->timeout;
> +
> +	return mei_cldev_send(wdt->cldev, (u8 *)&req, req_len);
> +}
> +
> +/**
> + * mei_wdt_stop - send wd stop command
> + *
> + * @wdt: mei watchdog device
> + *
> + * Return: number of bytes sent on success,
> + *         negative errno code on failure
> + */
> +static int mei_wdt_stop(struct mei_wdt *wdt)
> +{
> +	struct mei_wdt_stop_request req;
> +	const size_t req_len = sizeof(req);
> +
> +	memset(&req, 0, req_len);
> +	req.hdr.command = MEI_MANAGEMENT_CONTROL;
> +	req.hdr.bytecount = req_len - offsetof(struct mei_wdt_hdr, subcommand);
> +	req.hdr.subcommand = MEI_MC_STOP_WD_TIMER_REQ;
> +	req.hdr.versionnumber = MEI_MC_VERSION_NUMBER;
> +
> +	return mei_cldev_send(wdt->cldev, (u8 *)&req, req_len);
> +}
> +
> +/**
> + * mei_wdt_ops_start - wd start command from the watchdog core.
> + *
> + * @wdd: watchdog device
> + *
> + * Return: 0 on success or -ENODEV;
> + */
> +static int mei_wdt_ops_start(struct watchdog_device *wdd)
> +{
> +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> +
> +	if (!mwd)
> +		return -ENODEV;

This can only happen because you call watchdog_set_drvdata() after
watchdog device registration. If that happens, the system is in
really bad shape.

I would suggest to move the call to watchdog_set_drvdata() ahead
of watchdog_register_device() and drop those checks.

> +
> +	mwd->wdt->state = MEI_WDT_START;
> +	wdd->timeout = mwd->wdt->timeout;
> +	return 0;
> +}
> +
> +/**
> + * mei_wdt_ops_stop - wd stop command from the watchdog core.
> + *
> + * @wdd: watchdog device
> + *
> + * Return: 0 if success, negative errno code for failure
> + */
> +static int mei_wdt_ops_stop(struct watchdog_device *wdd)
> +{
> +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> +	struct mei_wdt *wdt;
> +	int ret;
> +
> +	if (!mwd)
> +		return -ENODEV;
> +
> +	wdt = mwd->wdt;
> +
> +	if (wdt->state != MEI_WDT_RUNNING)
> +		return 0;
> +
> +	wdt->state = MEI_WDT_STOPPING;
> +
> +	ret = mei_wdt_stop(wdt);
> +	if (ret < 0)
> +		return ret;
> +
> +	wdt->state = MEI_WDT_IDLE;
> +
> +	return 0;
> +}
> +
> +/**
> + * mei_wdt_ops_ping - wd ping command from the watchdog core.
> + *
> + * @wdd: watchdog device
> + *
> + * Return: 0 if success, negative errno code on failure
> + */
> +static int mei_wdt_ops_ping(struct watchdog_device *wdd)
> +{
> +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> +	struct mei_wdt *wdt;
> +	int ret;
> +
> +	if (!mwd)
> +		return -ENODEV;
> +
> +	wdt = mwd->wdt;
> +
> +	if (wdt->state != MEI_WDT_START &&
> +	    wdt->state != MEI_WDT_RUNNING)

Unnecessary continuation line ?

> +		return 0;
> +
> +	ret = mei_wdt_ping(wdt);
> +	if (ret < 0)
> +		return ret;
> +
> +	wdt->state = MEI_WDT_RUNNING;
> +
> +	return 0;
> +}
> +
> +/**
> + * mei_wdt_ops_set_timeout - wd set timeout command from the watchdog core.
> + *
> + * @wdd: watchdog device
> + * @timeout: timeout value to set
> + *
> + * Return: 0 if success, negative errno code for failure
> + */
> +static int mei_wdt_ops_set_timeout(struct watchdog_device *wdd,
> +				   unsigned int timeout)
> +{
> +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> +	struct mei_wdt *wdt;
> +
> +	if (!mwd)
> +		return -ENODEV;
> +
> +	wdt = mwd->wdt;
> +
> +	/* valid value is already checked by the caller */
> +	wdt->timeout = timeout;
> +	wdd->timeout = timeout;

One of those seems unnecessary. Why keep the timeout twice ?

> +
> +	return 0;
> +}
> +
> +static void mei_wdt_release(struct kref *ref)
> +{
> +	struct mei_wdt_dev *mwd = container_of(ref, struct mei_wdt_dev, refcnt);
> +
> +	kfree(mwd);
> +}
> +
> +static void mei_wdt_ops_ref(struct watchdog_device *wdd)
> +{
> +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> +
> +	kref_get(&mwd->refcnt);
> +}
> +
> +static void mei_wdt_ops_unref(struct watchdog_device *wdd)
> +{
> +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> +
> +	kref_put(&mwd->refcnt, mei_wdt_release);
> +}
> +
> +static const struct watchdog_ops wd_ops = {
> +	.owner       = THIS_MODULE,
> +	.start       = mei_wdt_ops_start,
> +	.stop        = mei_wdt_ops_stop,
> +	.ping        = mei_wdt_ops_ping,
> +	.set_timeout = mei_wdt_ops_set_timeout,
> +	.ref         = mei_wdt_ops_ref,
> +	.unref       = mei_wdt_ops_unref,
> +};
> +
> +static struct watchdog_info wd_info = {
> +	.identity = INTEL_AMT_WATCHDOG_ID,
> +	.options  = WDIOF_KEEPALIVEPING |
> +		    WDIOF_SETTIMEOUT |
> +		    WDIOF_ALARMONLY,
> +};
> +
> +static int mei_wdt_register(struct mei_wdt *wdt)
> +{
> +	struct mei_wdt_dev *mwd;
> +	struct device *dev;
> +	int ret;
> +
> +	if (!wdt || !wdt->cldev)
> +		return -EINVAL;
> +
> +	dev = &wdt->cldev->dev;
> +
> +	mwd = kzalloc(sizeof(struct mei_wdt_dev), GFP_KERNEL);
> +	if (!mwd)
> +		return -ENOMEM;
> +
> +	mwd->wdt = wdt;
> +	mwd->wdd.info = &wd_info;
> +	mwd->wdd.ops = &wd_ops;
> +	mwd->wdd.parent = dev;
> +	mwd->wdd.timeout = MEI_WDT_DEFAULT_TIMEOUT;
> +	mwd->wdd.min_timeout = MEI_WDT_MIN_TIMEOUT;
> +	mwd->wdd.max_timeout = MEI_WDT_MAX_TIMEOUT;
> +	kref_init(&mwd->refcnt);
> +
> +	ret = watchdog_register_device(&mwd->wdd);
> +	if (ret) {
> +		dev_err(dev, "unable to register watchdog device = %d.\n", ret);
> +		kref_put(&mwd->refcnt, mei_wdt_release);
> +		return ret;
> +	}
> +
> +	wdt->mwd = mwd;
> +	watchdog_set_drvdata(&mwd->wdd, mwd);
> +	return 0;
> +}
> +
> +static void mei_wdt_unregister(struct mei_wdt *wdt)
> +{
> +	if (!wdt->mwd)
> +		return;
> +
> +	watchdog_unregister_device(&wdt->mwd->wdd);
> +	kref_put(&wdt->mwd->refcnt, mei_wdt_release);
> +	wdt->mwd = NULL;
> +}

Seems to me that using two separate data structures instead of one
adds a lot of complexity.

> +
> +static int mei_wdt_probe(struct mei_cl_device *cldev,
> +			 const struct mei_cl_device_id *id)
> +{
> +	struct mei_wdt *wdt;
> +	int ret;
> +
> +	wdt = kzalloc(sizeof(struct mei_wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
> +
> +	wdt->timeout = MEI_WDT_DEFAULT_TIMEOUT;
> +	wdt->state = MEI_WDT_IDLE;
> +	wdt->cldev = cldev;
> +	mei_cldev_set_drvdata(cldev, wdt);
> +
> +	ret = mei_cldev_enable(cldev);
> +	if (ret < 0) {
> +		dev_err(&cldev->dev, "Could not enable cl device\n");
> +		goto err_out;
> +	}
> +
> +	wd_info.firmware_version = mei_cldev_ver(cldev);
> +
> +	ret = mei_wdt_register(wdt);
> +	if (ret)
> +		goto err_disable;
> +
> +	return 0;
> +
> +err_disable:
> +	mei_cldev_disable(cldev);
> +
> +err_out:
> +	kfree(wdt);
> +
> +	return ret;
> +}
> +
> +static int mei_wdt_remove(struct mei_cl_device *cldev)
> +{
> +	struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
> +
> +	mei_cldev_disable(cldev);
> +
> +	mei_wdt_unregister(wdt);
> +
> +	kfree(wdt);
> +
> +	return 0;
> +}
> +
> +#define MEI_UUID_WD UUID_LE(0x05B79A6F, 0x4628, 0x4D7F, \
> +			    0x89, 0x9D, 0xA9, 0x15, 0x14, 0xCB, 0x32, 0xAB)
> +
> +static struct mei_cl_device_id mei_wdt_tbl[] = {
> +	{ .uuid = MEI_UUID_WD, .version = MEI_CL_VERSION_ANY},
> +	/* required last entry */
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(mei, mei_wdt_tbl);
> +
> +static struct mei_cl_driver mei_wdt_driver = {
> +	.id_table = mei_wdt_tbl,
> +	.name = KBUILD_MODNAME,
> +
> +	.probe = mei_wdt_probe,
> +	.remove = mei_wdt_remove,
> +};
> +
> +static int __init mei_wdt_init(void)
> +{
> +	int ret;
> +
> +	ret = mei_cldev_driver_register(&mei_wdt_driver);
> +	if (ret) {
> +		pr_err(KBUILD_MODNAME ": module registration failed\n");
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +static void __exit mei_wdt_exit(void)
> +{
> +	mei_cldev_driver_unregister(&mei_wdt_driver);
> +}
> +
> +module_init(mei_wdt_init);
> +module_exit(mei_wdt_exit);
> +
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Device driver for Intel MEI iAMT watchdog");
>

Re: [char-misc-next 6/6] mei: wd: re-register device on event

[Guenter Roeck]

On 11/26/2015 04:31 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 register the watchdog device with the kernel.
>

Can the watchdog device also be disabled on the fly ?

> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>   drivers/watchdog/mei_wdt.c | 26 ++++++++++++++++++++++++--
>   1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
> index 47f0dc2e822a..ee3b5d12b1b2 100644
> --- a/drivers/watchdog/mei_wdt.c
> +++ b/drivers/watchdog/mei_wdt.c
> @@ -279,6 +279,21 @@ out:
>   		complete(&wdt->response);
>   }
>
> +/*
> + * mei_wdt_event_notif - callback for notification
> + *
> + * @cldev: bus device
> + */
> +static void mei_wdt_event_notif(struct mei_cl_device *cldev)
> +{
> +	struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
> +
> +	if (wdt->state == MEI_WDT_NOT_REQUIRED) {
> +		mei_wdt_register(wdt);
> +		wdt->state = MEI_WDT_IDLE;
> +	}
> +}
> +
>   /**
>    * mei_wdt_event - callback for event receive
>    *
> @@ -291,6 +306,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_event_notif(cldev);
>   }
>
>   /**
> @@ -466,9 +484,13 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
>
>   	wd_info.firmware_version = mei_cldev_ver(cldev);
>
> -	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;
>   	}
>
Doesn't that mean, though, that the RX event is not registered either,
even if it is supported ? If so, should you retry and register only
MEI_CL_EVENT_RX ?

Thanks,
Guenter

Re: [char-misc-next 3/6] mei: wd: implement MEI iAMT watchdog driver

[Winkler, Tomas]

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

On Mon, 2015-11-30 at 08:55 -0800, Guenter Roeck wrote:
> On 11/26/2015 04:31 AM, Tomas Winkler wrote:
> > From: Alexander Usyskin <alexander.usyskin@intel.com>
> > 
> > Create a driver with the generic watchdog interface
> > for the MEI iAMT watchdog device.
> > 
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> >   Documentation/misc-devices/mei/mei.txt |  12 +-
> >   MAINTAINERS                            |   1 +
> >   drivers/watchdog/Kconfig               |  15 ++
> >   drivers/watchdog/Makefile              |   1 +
> >   drivers/watchdog/mei_wdt.c             | 432
> > +++++++++++++++++++++++++++++++++
> >   5 files changed, 455 insertions(+), 6 deletions(-)
> >   create mode 100644 drivers/watchdog/mei_wdt.c
> > 
> > diff --git a/Documentation/misc-devices/mei/mei.txt
> > b/Documentation/misc-devices/mei/mei.txt
> > index 91c1fa34f48b..2b80a0cd621f 100644
> > --- a/Documentation/misc-devices/mei/mei.txt
> > +++ b/Documentation/misc-devices/mei/mei.txt
> > @@ -231,15 +231,15 @@ IT knows when a platform crashes even when
> > there is a hard failure on the host.
> >   The Intel AMT Watchdog is composed of two parts:
> >   	1) Firmware feature - receives the heartbeats
> >   	   and sends an event when the heartbeats stop.
> > -	2) Intel MEI driver - connects to the watchdog feature,
> > configures the
> > -	   watchdog and sends the heartbeats.
> > +	2) Intel MEI iAMT watchdog driver - connects to the
> > watchdog feature,
> > +	   configures the watchdog and sends the heartbeats.
> > 
> > -The Intel MEI driver uses the kernel watchdog API to configure the
> > Intel AMT
> > -Watchdog and to send heartbeats to it. The default timeout of the
> > +The Intel iAMT watchdog MEI driver uses the kernel watchdog API to
> > configure
> > +the Intel AMT Watchdog and to send heartbeats to it. The default
> > timeout of the
> >   watchdog is 120 seconds.
> > 
> > -If the Intel AMT Watchdog feature does not exist (i.e. the
> > connection failed),
> > -the Intel MEI driver will disable the sending of heartbeats.
> > +If the Intel AMT is not enabled in the firmware then the watchdog
> > client won't enumerate
> > +on the me client bus and watchdog devices won't be exposed.
> > 
> > 
> >   Supported Chipsets
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 050d0e77a2cf..cf0a51518f4a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5664,6 +5664,7 @@ S:	Supported
> >   F:	include/uapi/linux/mei.h
> >   F:	include/linux/mei_cl_bus.h
> >   F:	drivers/misc/mei/*
> > +F:	drivers/watchdog/mei_wdt.c
> >   F:	Documentation/misc-devices/mei/*
> > 
> >   INTEL MIC DRIVERS (mic)
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 7a8a6c6952e9..ec584714829d 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -1154,6 +1154,21 @@ config SBC_EPX_C3_WATCHDOG
> >   	  To compile this driver as a module, choose M here: the
> >   	  module will be called sbc_epx_c3.
> > 
> > +config INTEL_MEI_WDT
> > +	tristate "Intel MEI iAMT Watchdog"
> > +	depends on INTEL_MEI && X86
> > +	select WATCHDOG_CORE
> > +	---help---
> > +	  A device driver for the Intel MEI iAMT watchdog.
> > +
> > +	  The Intel AMT Watchdog is an OS Health (Hang/Crash)
> > watchdog.
> > +	  Whenever the OS hangs or crashes, iAMT will send an
> > event
> > +	  to any subscriber to this event. The watchdog doesn't
> > reset the
> > +	  the platform.
> > +
> > +	  To compile this driver as a module, choose M here:
> > +	  the module will be called mei_wdt.
> > +
> >   # M32R Architecture
> > 
> >   # M68K Architecture
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index 53d4827ddfe1..9069c9dd8aa8 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -123,6 +123,7 @@ obj-$(CONFIG_MACHZ_WDT) += machzwd.o
> >   obj-$(CONFIG_SBC_EPX_C3_WATCHDOG) += sbc_epx_c3.o
> >   obj-$(CONFIG_INTEL_SCU_WATCHDOG) += intel_scu_watchdog.o
> >   obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
> > +obj-$(CONFIG_INTEL_MEI_WDT) += mei_wdt.o
> > 
> >   # M32R Architecture
> > 
> > diff --git a/drivers/watchdog/mei_wdt.c
> > b/drivers/watchdog/mei_wdt.c
> > new file mode 100644
> > index 000000000000..149b29f341cf
> > --- /dev/null
> > +++ b/drivers/watchdog/mei_wdt.c
> > @@ -0,0 +1,432 @@
> > +/*
> > + * Intel Management Engine Interface (Intel MEI) Linux driver
> > + * Copyright (c) 2015, Intel Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify it
> > + * under the terms and conditions of the GNU General Public
> > License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but
> > WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
> > License for
> > + * more details.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/watchdog.h>
> > +
> > +#include <linux/uuid.h>
> > +#include <linux/mei_cl_bus.h>
> > +
> > +/*
> > + * iAMT Watchdog Device
> > + */
> > +#define INTEL_AMT_WATCHDOG_ID "iamt_wdt"
> > +
> > +#define MEI_WDT_DEFAULT_TIMEOUT   120  /* seconds */
> > +#define MEI_WDT_MIN_TIMEOUT       120  /* seconds */
> 
> Is the large minimum timeout on purpose ?
> Just asking, since it is quite unusual.

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

> 
> > +#define MEI_WDT_MAX_TIMEOUT     65535  /* seconds */
> > +
> > +/* Commands */
> > +#define MEI_MANAGEMENT_CONTROL 0x02
> > +
> > +/* MEI Management Control version number */
> > +#define MEI_MC_VERSION_NUMBER  0x10
> > +
> > +/* Sub Commands */
> > +#define MEI_MC_START_WD_TIMER_REQ  0x13
> > +#define MEI_MC_STOP_WD_TIMER_REQ   0x14
> > +
> > +/**
> > + * enum mei_wdt_state - internal watchdog state
> > + *
> > + * @MEI_WDT_IDLE: wd is idle and not opened
> > + * @MEI_WDT_START: wd was opened, start was called
> > + * @MEI_WDT_RUNNING: wd is expecting keep alive pings
> > + * @MEI_WDT_STOPPING: wd is stopping and will move to IDLE
> > + */
> > +enum mei_wdt_state {
> > +	MEI_WDT_IDLE,
> > +	MEI_WDT_START,
> > +	MEI_WDT_RUNNING,
> > +	MEI_WDT_STOPPING,
> > +};
> > +
> > +struct mei_wdt;
> > +
> > +/**
> > + * struct mei_wdt_dev - watchdog device wrapper
> > + *
> > + * @wdd: watchdog device
> > + * @wdt: back pointer to mei_wdt driver
> > + * @refcnt: reference counter
> > + */
> > +struct mei_wdt_dev {
> > +	struct watchdog_device wdd;
> > +	struct mei_wdt *wdt;
> > +	struct kref refcnt;
> > +};
> > +
> > +/**
> > + * struct mei_wdt - mei watchdog driver
> > + *
> > + * @cldev: mei watchdog client device
> > + * @mwd: watchdog device wrapper
> > + * @state: watchdog internal state
> > + * @timeout: watchdog current timeout
> > + */
> > +struct mei_wdt {
> > +	struct mei_cl_device *cldev;
> > +	struct mei_wdt_dev *mwd;
> > +	enum mei_wdt_state state;
> > +	u16 timeout;
> > +};
> 
> Any special reason for having two data structures instead of one ?
> You could just move the variables from struct mei_wdt_dev into
> struct mei_wdt, no ?

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

> > +
> > +struct mei_wdt_hdr {
> > +	u8 command;
> > +	u8 bytecount;
> > +	u8 subcommand;
> > +	u8 versionnumber;
> > +};
> > +
> > +struct mei_wdt_start_request {
> > +	struct mei_wdt_hdr hdr;
> > +	u16 timeout;
> > +	u8 reserved[17];
> > +} __packed;
> > +
> > +struct mei_wdt_stop_request {
> > +	struct mei_wdt_hdr hdr;
> > +} __packed;
> > +
> > +/**
> > + * mei_wdt_ping - send wd start command
> > + *
> > + * @wdt: mei watchdog device
> > + *
> > + * Return: number of bytes sent on success,
> > + *         negative errno code on failure
> > + */
> > +static int mei_wdt_ping(struct mei_wdt *wdt)
> > +{
> > +	struct mei_wdt_start_request req;
> > +	const size_t req_len = sizeof(req);
> > +
> > +	memset(&req, 0, req_len);
> > +	req.hdr.command = MEI_MANAGEMENT_CONTROL;
> > +	req.hdr.bytecount = req_len - offsetof(struct mei_wdt_hdr,
> > subcommand);
> > +	req.hdr.subcommand = MEI_MC_START_WD_TIMER_REQ;
> > +	req.hdr.versionnumber = MEI_MC_VERSION_NUMBER;
> > +	req.timeout = wdt->timeout;
> > +
> > +	return mei_cldev_send(wdt->cldev, (u8 *)&req, req_len);
> > +}
> > +
> > +/**
> > + * mei_wdt_stop - send wd stop command
> > + *
> > + * @wdt: mei watchdog device
> > + *
> > + * Return: number of bytes sent on success,
> > + *         negative errno code on failure
> > + */
> > +static int mei_wdt_stop(struct mei_wdt *wdt)
> > +{
> > +	struct mei_wdt_stop_request req;
> > +	const size_t req_len = sizeof(req);
> > +
> > +	memset(&req, 0, req_len);
> > +	req.hdr.command = MEI_MANAGEMENT_CONTROL;
> > +	req.hdr.bytecount = req_len - offsetof(struct mei_wdt_hdr,
> > subcommand);
> > +	req.hdr.subcommand = MEI_MC_STOP_WD_TIMER_REQ;
> > +	req.hdr.versionnumber = MEI_MC_VERSION_NUMBER;
> > +
> > +	return mei_cldev_send(wdt->cldev, (u8 *)&req, req_len);
> > +}
> > +
> > +/**
> > + * mei_wdt_ops_start - wd start command from the watchdog core.
> > + *
> > + * @wdd: watchdog device
> > + *
> > + * Return: 0 on success or -ENODEV;
> > + */
> > +static int mei_wdt_ops_start(struct watchdog_device *wdd)
> > +{
> > +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> > +
> > +	if (!mwd)
> > +		return -ENODEV;
> 
> This can only happen because you call watchdog_set_drvdata() after
> watchdog device registration. If that happens, the system is in
> really bad shape.

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

> 
> I would suggest to move the call to watchdog_set_drvdata() ahead
> of watchdog_register_device() and drop those checks.
> 
> > +
> > +	mwd->wdt->state = MEI_WDT_START;
> > +	wdd->timeout = mwd->wdt->timeout;
> > +	return 0;
> > +}
> > +
> > +/**
> > + * mei_wdt_ops_stop - wd stop command from the watchdog core.
> > + *
> > + * @wdd: watchdog device
> > + *
> > + * Return: 0 if success, negative errno code for failure
> > + */
> > +static int mei_wdt_ops_stop(struct watchdog_device *wdd)
> > +{
> > +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> > +	struct mei_wdt *wdt;
> > +	int ret;
> > +
> > +	if (!mwd)
> > +		return -ENODEV;
> > +
> > +	wdt = mwd->wdt;
> > +
> > +	if (wdt->state != MEI_WDT_RUNNING)
> > +		return 0;
> > +
> > +	wdt->state = MEI_WDT_STOPPING;
> > +
> > +	ret = mei_wdt_stop(wdt);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	wdt->state = MEI_WDT_IDLE;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * mei_wdt_ops_ping - wd ping command from the watchdog core.
> > + *
> > + * @wdd: watchdog device
> > + *
> > + * Return: 0 if success, negative errno code on failure
> > + */
> > +static int mei_wdt_ops_ping(struct watchdog_device *wdd)
> > +{
> > +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> > +	struct mei_wdt *wdt;
> > +	int ret;
> > +
> > +	if (!mwd)
> > +		return -ENODEV;
> > +
> > +	wdt = mwd->wdt;
> > +
> > +	if (wdt->state != MEI_WDT_START &&
> > +	    wdt->state != MEI_WDT_RUNNING)
> 
> Unnecessary continuation line ?
Looks more readable to me. but we can also straight the condition. 
> 
> > +		return 0;
> > +
> > +	ret = mei_wdt_ping(wdt);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	wdt->state = MEI_WDT_RUNNING;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * mei_wdt_ops_set_timeout - wd set timeout command from the
> > watchdog core.
> > + *
> > + * @wdd: watchdog device
> > + * @timeout: timeout value to set
> > + *
> > + * Return: 0 if success, negative errno code for failure
> > + */
> > +static int mei_wdt_ops_set_timeout(struct watchdog_device *wdd,
> > +				   unsigned int timeout)
> > +{
> > +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> > +	struct mei_wdt *wdt;
> > +
> > +	if (!mwd)
> > +		return -ENODEV;
> > +
> > +	wdt = mwd->wdt;
> > +
> > +	/* valid value is already checked by the caller */
> > +	wdt->timeout = timeout;
> > +	wdd->timeout = timeout;
> 
> One of those seems unnecessary. Why keep the timeout twice ?

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

> > +
> > +	return 0;
> > +}
> > +
> > +static void mei_wdt_release(struct kref *ref)
> > +{
> > +	struct mei_wdt_dev *mwd = container_of(ref, struct
> > mei_wdt_dev, refcnt);
> > +
> > +	kfree(mwd);
> > +}
> > +
> > +static void mei_wdt_ops_ref(struct watchdog_device *wdd)
> > +{
> > +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> > +
> > +	kref_get(&mwd->refcnt);
> > +}
> > +
> > +static void mei_wdt_ops_unref(struct watchdog_device *wdd)
> > +{
> > +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> > +
> > +	kref_put(&mwd->refcnt, mei_wdt_release);
> > +}
> > +
> > +static const struct watchdog_ops wd_ops = {
> > +	.owner       = THIS_MODULE,
> > +	.start       = mei_wdt_ops_start,
> > +	.stop        = mei_wdt_ops_stop,
> > +	.ping        = mei_wdt_ops_ping,
> > +	.set_timeout = mei_wdt_ops_set_timeout,
> > +	.ref         = mei_wdt_ops_ref,
> > +	.unref       = mei_wdt_ops_unref,
> > +};
> > +
> > +static struct watchdog_info wd_info = {
> > +	.identity = INTEL_AMT_WATCHDOG_ID,
> > +	.options  = WDIOF_KEEPALIVEPING |
> > +		    WDIOF_SETTIMEOUT |
> > +		    WDIOF_ALARMONLY,
> > +};
> > +
> > +static int mei_wdt_register(struct mei_wdt *wdt)
> > +{
> > +	struct mei_wdt_dev *mwd;
> > +	struct device *dev;
> > +	int ret;
> > +
> > +	if (!wdt || !wdt->cldev)
> > +		return -EINVAL;
> > +
> > +	dev = &wdt->cldev->dev;
> > +
> > +	mwd = kzalloc(sizeof(struct mei_wdt_dev), GFP_KERNEL);
> > +	if (!mwd)
> > +		return -ENOMEM;
> > +
> > +	mwd->wdt = wdt;
> > +	mwd->wdd.info = &wd_info;
> > +	mwd->wdd.ops = &wd_ops;
> > +	mwd->wdd.parent = dev;
> > +	mwd->wdd.timeout = MEI_WDT_DEFAULT_TIMEOUT;
> > +	mwd->wdd.min_timeout = MEI_WDT_MIN_TIMEOUT;
> > +	mwd->wdd.max_timeout = MEI_WDT_MAX_TIMEOUT;
> > +	kref_init(&mwd->refcnt);
> > +
> > +	ret = watchdog_register_device(&mwd->wdd);
> > +	if (ret) {
> > +		dev_err(dev, "unable to register watchdog device =
> > %d.\n", ret);
> > +		kref_put(&mwd->refcnt, mei_wdt_release);
> > +		return ret;
> > +	}
> > +
> > +	wdt->mwd = mwd;
> > +	watchdog_set_drvdata(&mwd->wdd, mwd);
> > +	return 0;
> > +}
> > +
> > +static void mei_wdt_unregister(struct mei_wdt *wdt)
> > +{
> > +	if (!wdt->mwd)
> > +		return;
> > +
> > +	watchdog_unregister_device(&wdt->mwd->wdd);
> > +	kref_put(&wdt->mwd->refcnt, mei_wdt_release);
> > +	wdt->mwd = NULL;
> > +}
> 
> Seems to me that using two separate data structures instead of one
> adds a lot of complexity.

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

> > +
> > +static int mei_wdt_probe(struct mei_cl_device *cldev,
> > +			 const struct mei_cl_device_id *id)
> > +{
> > +	struct mei_wdt *wdt;
> > +	int ret;
> > +
> > +	wdt = kzalloc(sizeof(struct mei_wdt), GFP_KERNEL);
> > +	if (!wdt)
> > +		return -ENOMEM;
> > +
> > +	wdt->timeout = MEI_WDT_DEFAULT_TIMEOUT;
> > +	wdt->state = MEI_WDT_IDLE;
> > +	wdt->cldev = cldev;
> > +	mei_cldev_set_drvdata(cldev, wdt);
> > +
> > +	ret = mei_cldev_enable(cldev);
> > +	if (ret < 0) {
> > +		dev_err(&cldev->dev, "Could not enable cl
> > device\n");
> > +		goto err_out;
> > +	}
> > +
> > +	wd_info.firmware_version = mei_cldev_ver(cldev);
> > +
> > +	ret = mei_wdt_register(wdt);
> > +	if (ret)
> > +		goto err_disable;
> > +
> > +	return 0;
> > +
> > +err_disable:
> > +	mei_cldev_disable(cldev);
> > +
> > +err_out:
> > +	kfree(wdt);
> > +
> > +	return ret;
> > +}
> > +
> > +static int mei_wdt_remove(struct mei_cl_device *cldev)
> > +{
> > +	struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
> > +
> > +	mei_cldev_disable(cldev);
> > +
> > +	mei_wdt_unregister(wdt);
> > +
> > +	kfree(wdt);
> > +
> > +	return 0;
> > +}
> > +
> > +#define MEI_UUID_WD UUID_LE(0x05B79A6F, 0x4628, 0x4D7F, \
> > +			    0x89, 0x9D, 0xA9, 0x15, 0x14, 0xCB,
> > 0x32, 0xAB)
> > +
> > +static struct mei_cl_device_id mei_wdt_tbl[] = {
> > +	{ .uuid = MEI_UUID_WD, .version = MEI_CL_VERSION_ANY},
> > +	/* required last entry */
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(mei, mei_wdt_tbl);
> > +
> > +static struct mei_cl_driver mei_wdt_driver = {
> > +	.id_table = mei_wdt_tbl,
> > +	.name = KBUILD_MODNAME,
> > +
> > +	.probe = mei_wdt_probe,
> > +	.remove = mei_wdt_remove,
> > +};
> > +
> > +static int __init mei_wdt_init(void)
> > +{
> > +	int ret;
> > +
> > +	ret = mei_cldev_driver_register(&mei_wdt_driver);
> > +	if (ret) {
> > +		pr_err(KBUILD_MODNAME ": module registration
> > failed\n");
> > +		return ret;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static void __exit mei_wdt_exit(void)
> > +{
> > +	mei_cldev_driver_unregister(&mei_wdt_driver);
> > +}
> > +
> > +module_init(mei_wdt_init);
> > +module_exit(mei_wdt_exit);
> > +
> > +MODULE_AUTHOR("Intel Corporation");
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Device driver for Intel MEI iAMT watchdog");
> > 
> ÿôèº{.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 3/6] mei: wd: implement MEI iAMT watchdog driver

[Winkler, Tomas]

On Mon, 2015-11-30 at 08:55 -0800, Guenter Roeck wrote:
> On 11/26/2015 04:31 AM, Tomas Winkler wrote:
> > From: Alexander Usyskin <alexander.usyskin@intel.com>
> > 
> > Create a driver with the generic watchdog interface
> > for the MEI iAMT watchdog device.
> > 
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> >   Documentation/misc-devices/mei/mei.txt |  12 +-
> >   MAINTAINERS                            |   1 +
> >   drivers/watchdog/Kconfig               |  15 ++
> >   drivers/watchdog/Makefile              |   1 +
> >   drivers/watchdog/mei_wdt.c             | 432
> > +++++++++++++++++++++++++++++++++
> >   5 files changed, 455 insertions(+), 6 deletions(-)
> >   create mode 100644 drivers/watchdog/mei_wdt.c
> > 
> > diff --git a/Documentation/misc-devices/mei/mei.txt
> > b/Documentation/misc-devices/mei/mei.txt
> > index 91c1fa34f48b..2b80a0cd621f 100644
> > --- a/Documentation/misc-devices/mei/mei.txt
> > +++ b/Documentation/misc-devices/mei/mei.txt
> > @@ -231,15 +231,15 @@ IT knows when a platform crashes even when
> > there is a hard failure on the host.
> >   The Intel AMT Watchdog is composed of two parts:
> >   	1) Firmware feature - receives the heartbeats
> >   	   and sends an event when the heartbeats stop.
> > -	2) Intel MEI driver - connects to the watchdog feature,
> > configures the
> > -	   watchdog and sends the heartbeats.
> > +	2) Intel MEI iAMT watchdog driver - connects to the
> > watchdog feature,
> > +	   configures the watchdog and sends the heartbeats.
> > 
> > -The Intel MEI driver uses the kernel watchdog API to configure the
> > Intel AMT
> > -Watchdog and to send heartbeats to it. The default timeout of the
> > +The Intel iAMT watchdog MEI driver uses the kernel watchdog API to
> > configure
> > +the Intel AMT Watchdog and to send heartbeats to it. The default
> > timeout of the
> >   watchdog is 120 seconds.
> > 
> > -If the Intel AMT Watchdog feature does not exist (i.e. the
> > connection failed),
> > -the Intel MEI driver will disable the sending of heartbeats.
> > +If the Intel AMT is not enabled in the firmware then the watchdog
> > client won't enumerate
> > +on the me client bus and watchdog devices won't be exposed.
> > 
> > 
> >   Supported Chipsets
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 050d0e77a2cf..cf0a51518f4a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5664,6 +5664,7 @@ S:	Supported
> >   F:	include/uapi/linux/mei.h
> >   F:	include/linux/mei_cl_bus.h
> >   F:	drivers/misc/mei/*
> > +F:	drivers/watchdog/mei_wdt.c
> >   F:	Documentation/misc-devices/mei/*
> > 
> >   INTEL MIC DRIVERS (mic)
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 7a8a6c6952e9..ec584714829d 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -1154,6 +1154,21 @@ config SBC_EPX_C3_WATCHDOG
> >   	  To compile this driver as a module, choose M here: the
> >   	  module will be called sbc_epx_c3.
> > 
> > +config INTEL_MEI_WDT
> > +	tristate "Intel MEI iAMT Watchdog"
> > +	depends on INTEL_MEI && X86
> > +	select WATCHDOG_CORE
> > +	---help---
> > +	  A device driver for the Intel MEI iAMT watchdog.
> > +
> > +	  The Intel AMT Watchdog is an OS Health (Hang/Crash)
> > watchdog.
> > +	  Whenever the OS hangs or crashes, iAMT will send an
> > event
> > +	  to any subscriber to this event. The watchdog doesn't
> > reset the
> > +	  the platform.
> > +
> > +	  To compile this driver as a module, choose M here:
> > +	  the module will be called mei_wdt.
> > +
> >   # M32R Architecture
> > 
> >   # M68K Architecture
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index 53d4827ddfe1..9069c9dd8aa8 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -123,6 +123,7 @@ obj-$(CONFIG_MACHZ_WDT) += machzwd.o
> >   obj-$(CONFIG_SBC_EPX_C3_WATCHDOG) += sbc_epx_c3.o
> >   obj-$(CONFIG_INTEL_SCU_WATCHDOG) += intel_scu_watchdog.o
> >   obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
> > +obj-$(CONFIG_INTEL_MEI_WDT) += mei_wdt.o
> > 
> >   # M32R Architecture
> > 
> > diff --git a/drivers/watchdog/mei_wdt.c
> > b/drivers/watchdog/mei_wdt.c
> > new file mode 100644
> > index 000000000000..149b29f341cf
> > --- /dev/null
> > +++ b/drivers/watchdog/mei_wdt.c
> > @@ -0,0 +1,432 @@
> > +/*
> > + * Intel Management Engine Interface (Intel MEI) Linux driver
> > + * Copyright (c) 2015, Intel Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify it
> > + * under the terms and conditions of the GNU General Public
> > License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but
> > WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
> > License for
> > + * more details.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/watchdog.h>
> > +
> > +#include <linux/uuid.h>
> > +#include <linux/mei_cl_bus.h>
> > +
> > +/*
> > + * iAMT Watchdog Device
> > + */
> > +#define INTEL_AMT_WATCHDOG_ID "iamt_wdt"
> > +
> > +#define MEI_WDT_DEFAULT_TIMEOUT   120  /* seconds */
> > +#define MEI_WDT_MIN_TIMEOUT       120  /* seconds */
> 
> Is the large minimum timeout on purpose ?
> Just asking, since it is quite unusual.

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

> 
> > +#define MEI_WDT_MAX_TIMEOUT     65535  /* seconds */
> > +
> > +/* Commands */
> > +#define MEI_MANAGEMENT_CONTROL 0x02
> > +
> > +/* MEI Management Control version number */
> > +#define MEI_MC_VERSION_NUMBER  0x10
> > +
> > +/* Sub Commands */
> > +#define MEI_MC_START_WD_TIMER_REQ  0x13
> > +#define MEI_MC_STOP_WD_TIMER_REQ   0x14
> > +
> > +/**
> > + * enum mei_wdt_state - internal watchdog state
> > + *
> > + * @MEI_WDT_IDLE: wd is idle and not opened
> > + * @MEI_WDT_START: wd was opened, start was called
> > + * @MEI_WDT_RUNNING: wd is expecting keep alive pings
> > + * @MEI_WDT_STOPPING: wd is stopping and will move to IDLE
> > + */
> > +enum mei_wdt_state {
> > +	MEI_WDT_IDLE,
> > +	MEI_WDT_START,
> > +	MEI_WDT_RUNNING,
> > +	MEI_WDT_STOPPING,
> > +};
> > +
> > +struct mei_wdt;
> > +
> > +/**
> > + * struct mei_wdt_dev - watchdog device wrapper
> > + *
> > + * @wdd: watchdog device
> > + * @wdt: back pointer to mei_wdt driver
> > + * @refcnt: reference counter
> > + */
> > +struct mei_wdt_dev {
> > +	struct watchdog_device wdd;
> > +	struct mei_wdt *wdt;
> > +	struct kref refcnt;
> > +};
> > +
> > +/**
> > + * struct mei_wdt - mei watchdog driver
> > + *
> > + * @cldev: mei watchdog client device
> > + * @mwd: watchdog device wrapper
> > + * @state: watchdog internal state
> > + * @timeout: watchdog current timeout
> > + */
> > +struct mei_wdt {
> > +	struct mei_cl_device *cldev;
> > +	struct mei_wdt_dev *mwd;
> > +	enum mei_wdt_state state;
> > +	u16 timeout;
> > +};
> 
> Any special reason for having two data structures instead of one ?
> You could just move the variables from struct mei_wdt_dev into
> struct mei_wdt, no ?

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

> > +
> > +struct mei_wdt_hdr {
> > +	u8 command;
> > +	u8 bytecount;
> > +	u8 subcommand;
> > +	u8 versionnumber;
> > +};
> > +
> > +struct mei_wdt_start_request {
> > +	struct mei_wdt_hdr hdr;
> > +	u16 timeout;
> > +	u8 reserved[17];
> > +} __packed;
> > +
> > +struct mei_wdt_stop_request {
> > +	struct mei_wdt_hdr hdr;
> > +} __packed;
> > +
> > +/**
> > + * mei_wdt_ping - send wd start command
> > + *
> > + * @wdt: mei watchdog device
> > + *
> > + * Return: number of bytes sent on success,
> > + *         negative errno code on failure
> > + */
> > +static int mei_wdt_ping(struct mei_wdt *wdt)
> > +{
> > +	struct mei_wdt_start_request req;
> > +	const size_t req_len = sizeof(req);
> > +
> > +	memset(&req, 0, req_len);
> > +	req.hdr.command = MEI_MANAGEMENT_CONTROL;
> > +	req.hdr.bytecount = req_len - offsetof(struct mei_wdt_hdr,
> > subcommand);
> > +	req.hdr.subcommand = MEI_MC_START_WD_TIMER_REQ;
> > +	req.hdr.versionnumber = MEI_MC_VERSION_NUMBER;
> > +	req.timeout = wdt->timeout;
> > +
> > +	return mei_cldev_send(wdt->cldev, (u8 *)&req, req_len);
> > +}
> > +
> > +/**
> > + * mei_wdt_stop - send wd stop command
> > + *
> > + * @wdt: mei watchdog device
> > + *
> > + * Return: number of bytes sent on success,
> > + *         negative errno code on failure
> > + */
> > +static int mei_wdt_stop(struct mei_wdt *wdt)
> > +{
> > +	struct mei_wdt_stop_request req;
> > +	const size_t req_len = sizeof(req);
> > +
> > +	memset(&req, 0, req_len);
> > +	req.hdr.command = MEI_MANAGEMENT_CONTROL;
> > +	req.hdr.bytecount = req_len - offsetof(struct mei_wdt_hdr,
> > subcommand);
> > +	req.hdr.subcommand = MEI_MC_STOP_WD_TIMER_REQ;
> > +	req.hdr.versionnumber = MEI_MC_VERSION_NUMBER;
> > +
> > +	return mei_cldev_send(wdt->cldev, (u8 *)&req, req_len);
> > +}
> > +
> > +/**
> > + * mei_wdt_ops_start - wd start command from the watchdog core.
> > + *
> > + * @wdd: watchdog device
> > + *
> > + * Return: 0 on success or -ENODEV;
> > + */
> > +static int mei_wdt_ops_start(struct watchdog_device *wdd)
> > +{
> > +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> > +
> > +	if (!mwd)
> > +		return -ENODEV;
> 
> This can only happen because you call watchdog_set_drvdata() after
> watchdog device registration. If that happens, the system is in
> really bad shape.

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

> 
> I would suggest to move the call to watchdog_set_drvdata() ahead
> of watchdog_register_device() and drop those checks.
> 
> > +
> > +	mwd->wdt->state = MEI_WDT_START;
> > +	wdd->timeout = mwd->wdt->timeout;
> > +	return 0;
> > +}
> > +
> > +/**
> > + * mei_wdt_ops_stop - wd stop command from the watchdog core.
> > + *
> > + * @wdd: watchdog device
> > + *
> > + * Return: 0 if success, negative errno code for failure
> > + */
> > +static int mei_wdt_ops_stop(struct watchdog_device *wdd)
> > +{
> > +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> > +	struct mei_wdt *wdt;
> > +	int ret;
> > +
> > +	if (!mwd)
> > +		return -ENODEV;
> > +
> > +	wdt = mwd->wdt;
> > +
> > +	if (wdt->state != MEI_WDT_RUNNING)
> > +		return 0;
> > +
> > +	wdt->state = MEI_WDT_STOPPING;
> > +
> > +	ret = mei_wdt_stop(wdt);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	wdt->state = MEI_WDT_IDLE;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * mei_wdt_ops_ping - wd ping command from the watchdog core.
> > + *
> > + * @wdd: watchdog device
> > + *
> > + * Return: 0 if success, negative errno code on failure
> > + */
> > +static int mei_wdt_ops_ping(struct watchdog_device *wdd)
> > +{
> > +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> > +	struct mei_wdt *wdt;
> > +	int ret;
> > +
> > +	if (!mwd)
> > +		return -ENODEV;
> > +
> > +	wdt = mwd->wdt;
> > +
> > +	if (wdt->state != MEI_WDT_START &&
> > +	    wdt->state != MEI_WDT_RUNNING)
> 
> Unnecessary continuation line ?
Looks more readable to me. but we can also straight the condition. 
> 
> > +		return 0;
> > +
> > +	ret = mei_wdt_ping(wdt);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	wdt->state = MEI_WDT_RUNNING;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * mei_wdt_ops_set_timeout - wd set timeout command from the
> > watchdog core.
> > + *
> > + * @wdd: watchdog device
> > + * @timeout: timeout value to set
> > + *
> > + * Return: 0 if success, negative errno code for failure
> > + */
> > +static int mei_wdt_ops_set_timeout(struct watchdog_device *wdd,
> > +				   unsigned int timeout)
> > +{
> > +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> > +	struct mei_wdt *wdt;
> > +
> > +	if (!mwd)
> > +		return -ENODEV;
> > +
> > +	wdt = mwd->wdt;
> > +
> > +	/* valid value is already checked by the caller */
> > +	wdt->timeout = timeout;
> > +	wdd->timeout = timeout;
> 
> One of those seems unnecessary. Why keep the timeout twice ?

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

> > +
> > +	return 0;
> > +}
> > +
> > +static void mei_wdt_release(struct kref *ref)
> > +{
> > +	struct mei_wdt_dev *mwd = container_of(ref, struct
> > mei_wdt_dev, refcnt);
> > +
> > +	kfree(mwd);
> > +}
> > +
> > +static void mei_wdt_ops_ref(struct watchdog_device *wdd)
> > +{
> > +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> > +
> > +	kref_get(&mwd->refcnt);
> > +}
> > +
> > +static void mei_wdt_ops_unref(struct watchdog_device *wdd)
> > +{
> > +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
> > +
> > +	kref_put(&mwd->refcnt, mei_wdt_release);
> > +}
> > +
> > +static const struct watchdog_ops wd_ops = {
> > +	.owner       = THIS_MODULE,
> > +	.start       = mei_wdt_ops_start,
> > +	.stop        = mei_wdt_ops_stop,
> > +	.ping        = mei_wdt_ops_ping,
> > +	.set_timeout = mei_wdt_ops_set_timeout,
> > +	.ref         = mei_wdt_ops_ref,
> > +	.unref       = mei_wdt_ops_unref,
> > +};
> > +
> > +static struct watchdog_info wd_info = {
> > +	.identity = INTEL_AMT_WATCHDOG_ID,
> > +	.options  = WDIOF_KEEPALIVEPING |
> > +		    WDIOF_SETTIMEOUT |
> > +		    WDIOF_ALARMONLY,
> > +};
> > +
> > +static int mei_wdt_register(struct mei_wdt *wdt)
> > +{
> > +	struct mei_wdt_dev *mwd;
> > +	struct device *dev;
> > +	int ret;
> > +
> > +	if (!wdt || !wdt->cldev)
> > +		return -EINVAL;
> > +
> > +	dev = &wdt->cldev->dev;
> > +
> > +	mwd = kzalloc(sizeof(struct mei_wdt_dev), GFP_KERNEL);
> > +	if (!mwd)
> > +		return -ENOMEM;
> > +
> > +	mwd->wdt = wdt;
> > +	mwd->wdd.info = &wd_info;
> > +	mwd->wdd.ops = &wd_ops;
> > +	mwd->wdd.parent = dev;
> > +	mwd->wdd.timeout = MEI_WDT_DEFAULT_TIMEOUT;
> > +	mwd->wdd.min_timeout = MEI_WDT_MIN_TIMEOUT;
> > +	mwd->wdd.max_timeout = MEI_WDT_MAX_TIMEOUT;
> > +	kref_init(&mwd->refcnt);
> > +
> > +	ret = watchdog_register_device(&mwd->wdd);
> > +	if (ret) {
> > +		dev_err(dev, "unable to register watchdog device =
> > %d.\n", ret);
> > +		kref_put(&mwd->refcnt, mei_wdt_release);
> > +		return ret;
> > +	}
> > +
> > +	wdt->mwd = mwd;
> > +	watchdog_set_drvdata(&mwd->wdd, mwd);
> > +	return 0;
> > +}
> > +
> > +static void mei_wdt_unregister(struct mei_wdt *wdt)
> > +{
> > +	if (!wdt->mwd)
> > +		return;
> > +
> > +	watchdog_unregister_device(&wdt->mwd->wdd);
> > +	kref_put(&wdt->mwd->refcnt, mei_wdt_release);
> > +	wdt->mwd = NULL;
> > +}
> 
> Seems to me that using two separate data structures instead of one
> adds a lot of complexity.

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

> > +
> > +static int mei_wdt_probe(struct mei_cl_device *cldev,
> > +			 const struct mei_cl_device_id *id)
> > +{
> > +	struct mei_wdt *wdt;
> > +	int ret;
> > +
> > +	wdt = kzalloc(sizeof(struct mei_wdt), GFP_KERNEL);
> > +	if (!wdt)
> > +		return -ENOMEM;
> > +
> > +	wdt->timeout = MEI_WDT_DEFAULT_TIMEOUT;
> > +	wdt->state = MEI_WDT_IDLE;
> > +	wdt->cldev = cldev;
> > +	mei_cldev_set_drvdata(cldev, wdt);
> > +
> > +	ret = mei_cldev_enable(cldev);
> > +	if (ret < 0) {
> > +		dev_err(&cldev->dev, "Could not enable cl
> > device\n");
> > +		goto err_out;
> > +	}
> > +
> > +	wd_info.firmware_version = mei_cldev_ver(cldev);
> > +
> > +	ret = mei_wdt_register(wdt);
> > +	if (ret)
> > +		goto err_disable;
> > +
> > +	return 0;
> > +
> > +err_disable:
> > +	mei_cldev_disable(cldev);
> > +
> > +err_out:
> > +	kfree(wdt);
> > +
> > +	return ret;
> > +}
> > +
> > +static int mei_wdt_remove(struct mei_cl_device *cldev)
> > +{
> > +	struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
> > +
> > +	mei_cldev_disable(cldev);
> > +
> > +	mei_wdt_unregister(wdt);
> > +
> > +	kfree(wdt);
> > +
> > +	return 0;
> > +}
> > +
> > +#define MEI_UUID_WD UUID_LE(0x05B79A6F, 0x4628, 0x4D7F, \
> > +			    0x89, 0x9D, 0xA9, 0x15, 0x14, 0xCB,
> > 0x32, 0xAB)
> > +
> > +static struct mei_cl_device_id mei_wdt_tbl[] = {
> > +	{ .uuid = MEI_UUID_WD, .version = MEI_CL_VERSION_ANY},
> > +	/* required last entry */
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(mei, mei_wdt_tbl);
> > +
> > +static struct mei_cl_driver mei_wdt_driver = {
> > +	.id_table = mei_wdt_tbl,
> > +	.name = KBUILD_MODNAME,
> > +
> > +	.probe = mei_wdt_probe,
> > +	.remove = mei_wdt_remove,
> > +};
> > +
> > +static int __init mei_wdt_init(void)
> > +{
> > +	int ret;
> > +
> > +	ret = mei_cldev_driver_register(&mei_wdt_driver);
> > +	if (ret) {
> > +		pr_err(KBUILD_MODNAME ": module registration
> > failed\n");
> > +		return ret;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static void __exit mei_wdt_exit(void)
> > +{
> > +	mei_cldev_driver_unregister(&mei_wdt_driver);
> > +}
> > +
> > +module_init(mei_wdt_init);
> > +module_exit(mei_wdt_exit);
> > +
> > +MODULE_AUTHOR("Intel Corporation");
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Device driver for Intel MEI iAMT watchdog");
> > 
> 

Re: [char-misc-next 6/6] mei: wd: 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: 2907 bytes --]

On Mon, 2015-11-30 at 09:08 -0800, Guenter Roeck wrote:
> On 11/26/2015 04:31 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 register the watchdog device with the
> > kernel.
> > 
> 
> Can the watchdog device also be disabled on the fly ?
Yes, it can be disabled also on the fly, but this is not delivered via
an event, this is handled via ping return value. 

> 
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> >   drivers/watchdog/mei_wdt.c | 26 ++++++++++++++++++++++++--
> >   1 file changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/watchdog/mei_wdt.c
> > b/drivers/watchdog/mei_wdt.c
> > index 47f0dc2e822a..ee3b5d12b1b2 100644
> > --- a/drivers/watchdog/mei_wdt.c
> > +++ b/drivers/watchdog/mei_wdt.c
> > @@ -279,6 +279,21 @@ out:
> >   		complete(&wdt->response);
> >   }
> > 
> > +/*
> > + * mei_wdt_event_notif - callback for notification
> > + *
> > + * @cldev: bus device
> > + */
> > +static void mei_wdt_event_notif(struct mei_cl_device *cldev)
> > +{
> > +	struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
> > +
> > +	if (wdt->state == MEI_WDT_NOT_REQUIRED) {
> > +		mei_wdt_register(wdt);
> > +		wdt->state = MEI_WDT_IDLE;
> > +	}
> > +}
> > +
> >   /**
> >    * mei_wdt_event - callback for event receive
> >    *
> > @@ -291,6 +306,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_event_notif(cldev);
> >   }
> > 
> >   /**
> > @@ -466,9 +484,13 @@ static int mei_wdt_probe(struct mei_cl_device
> > *cldev,
> > 
> >   	wd_info.firmware_version = mei_cldev_ver(cldev);
> > 
> > -	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;
> >   	}
> > 
> Doesn't that mean, though, that the RX event is not registered
> either,
> even if it is supported ? If so, should you retry and register only
> MEI_CL_EVENT_RX ?

The code is correct but we definitely need to think of better API 
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 6/6] mei: wd: re-register device on event

[Winkler, Tomas]

On Mon, 2015-11-30 at 09:08 -0800, Guenter Roeck wrote:
> On 11/26/2015 04:31 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 register the watchdog device with the
> > kernel.
> > 
> 
> Can the watchdog device also be disabled on the fly ?
Yes, it can be disabled also on the fly, but this is not delivered via
an event, this is handled via ping return value. 

> 
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> >   drivers/watchdog/mei_wdt.c | 26 ++++++++++++++++++++++++--
> >   1 file changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/watchdog/mei_wdt.c
> > b/drivers/watchdog/mei_wdt.c
> > index 47f0dc2e822a..ee3b5d12b1b2 100644
> > --- a/drivers/watchdog/mei_wdt.c
> > +++ b/drivers/watchdog/mei_wdt.c
> > @@ -279,6 +279,21 @@ out:
> >   		complete(&wdt->response);
> >   }
> > 
> > +/*
> > + * mei_wdt_event_notif - callback for notification
> > + *
> > + * @cldev: bus device
> > + */
> > +static void mei_wdt_event_notif(struct mei_cl_device *cldev)
> > +{
> > +	struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
> > +
> > +	if (wdt->state == MEI_WDT_NOT_REQUIRED) {
> > +		mei_wdt_register(wdt);
> > +		wdt->state = MEI_WDT_IDLE;
> > +	}
> > +}
> > +
> >   /**
> >    * mei_wdt_event - callback for event receive
> >    *
> > @@ -291,6 +306,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_event_notif(cldev);
> >   }
> > 
> >   /**
> > @@ -466,9 +484,13 @@ static int mei_wdt_probe(struct mei_cl_device
> > *cldev,
> > 
> >   	wd_info.firmware_version = mei_cldev_ver(cldev);
> > 
> > -	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;
> >   	}
> > 
> Doesn't that mean, though, that the RX event is not registered
> either,
> even if it is supported ? If so, should you retry and register only
> MEI_CL_EVENT_RX ?

The code is correct but we definitely need to think of better API 
Tomas

Re: [char-misc-next 3/6] mei: wd: implement MEI iAMT watchdog driver

[Guenter Roeck]

On 12/01/2015 03:55 AM, Winkler, Tomas wrote:
[ ... ]

>>> +/**
>>> + * struct mei_wdt_dev - watchdog device wrapper
>>> + *
>>> + * @wdd: watchdog device
>>> + * @wdt: back pointer to mei_wdt driver
>>> + * @refcnt: reference counter
>>> + */
>>> +struct mei_wdt_dev {
>>> +	struct watchdog_device wdd;
>>> +	struct mei_wdt *wdt;
>>> +	struct kref refcnt;
>>> +};
>>> +
>>> +/**
>>> + * struct mei_wdt - mei watchdog driver
>>> + *
>>> + * @cldev: mei watchdog client device
>>> + * @mwd: watchdog device wrapper
>>> + * @state: watchdog internal state
>>> + * @timeout: watchdog current timeout
>>> + */
>>> +struct mei_wdt {
>>> +	struct mei_cl_device *cldev;
>>> +	struct mei_wdt_dev *mwd;
>>> +	enum mei_wdt_state state;
>>> +	u16 timeout;
>>> +};
>>
>> Any special reason for having two data structures instead of one ?
>> You could just move the variables from struct mei_wdt_dev into
>> struct mei_wdt, no ?
>
> Yes, on newer platform mei_wdt_dev might be not present in case the the
> device is not provisioned. This came to action in the following
> patches.
>

It is still an implementation choice to keep the data structures separate.
That mei_wdt_dev can be unregistered doesn't mean that the data structure
has to be destroyed or allocated on registration.

>>> +
>>> +struct mei_wdt_hdr {
>>> +	u8 command;
>>> +	u8 bytecount;
>>> +	u8 subcommand;
>>> +	u8 versionnumber;
>>> +};
>>> +
>>> +struct mei_wdt_start_request {
>>> +	struct mei_wdt_hdr hdr;
>>> +	u16 timeout;
>>> +	u8 reserved[17];
>>> +} __packed;
>>> +
>>> +struct mei_wdt_stop_request {
>>> +	struct mei_wdt_hdr hdr;
>>> +} __packed;
>>> +
>>> +/**
>>> + * mei_wdt_ping - send wd start command
>>> + *
>>> + * @wdt: mei watchdog device
>>> + *
>>> + * Return: number of bytes sent on success,
>>> + *         negative errno code on failure
>>> + */
>>> +static int mei_wdt_ping(struct mei_wdt *wdt)
>>> +{
>>> +	struct mei_wdt_start_request req;
>>> +	const size_t req_len = sizeof(req);
>>> +
>>> +	memset(&req, 0, req_len);
>>> +	req.hdr.command = MEI_MANAGEMENT_CONTROL;
>>> +	req.hdr.bytecount = req_len - offsetof(struct mei_wdt_hdr,
>>> subcommand);
>>> +	req.hdr.subcommand = MEI_MC_START_WD_TIMER_REQ;
>>> +	req.hdr.versionnumber = MEI_MC_VERSION_NUMBER;
>>> +	req.timeout = wdt->timeout;
>>> +
>>> +	return mei_cldev_send(wdt->cldev, (u8 *)&req, req_len);
>>> +}
>>> +
>>> +/**
>>> + * mei_wdt_stop - send wd stop command
>>> + *
>>> + * @wdt: mei watchdog device
>>> + *
>>> + * Return: number of bytes sent on success,
>>> + *         negative errno code on failure
>>> + */
>>> +static int mei_wdt_stop(struct mei_wdt *wdt)
>>> +{
>>> +	struct mei_wdt_stop_request req;
>>> +	const size_t req_len = sizeof(req);
>>> +
>>> +	memset(&req, 0, req_len);
>>> +	req.hdr.command = MEI_MANAGEMENT_CONTROL;
>>> +	req.hdr.bytecount = req_len - offsetof(struct mei_wdt_hdr,
>>> subcommand);
>>> +	req.hdr.subcommand = MEI_MC_STOP_WD_TIMER_REQ;
>>> +	req.hdr.versionnumber = MEI_MC_VERSION_NUMBER;
>>> +
>>> +	return mei_cldev_send(wdt->cldev, (u8 *)&req, req_len);
>>> +}
>>> +
>>> +/**
>>> + * mei_wdt_ops_start - wd start command from the watchdog core.
>>> + *
>>> + * @wdd: watchdog device
>>> + *
>>> + * Return: 0 on success or -ENODEV;
>>> + */
>>> +static int mei_wdt_ops_start(struct watchdog_device *wdd)
>>> +{
>>> +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
>>> +
>>> +	if (!mwd)
>>> +		return -ENODEV;
>>
>> This can only happen because you call watchdog_set_drvdata() after
>> watchdog device registration. If that happens, the system is in
>> really bad shape.
>
> mei_wdt_dev can destroyed during
> driver operation if the device is unprovisioned, but still you the
> condition should not happen unless we have a bug. We can put WARN_ON()
> there.
>

The calling code should take care of that and not call those functions
after unregistration. More on that below.

>>
>> I would suggest to move the call to watchdog_set_drvdata() ahead
>> of watchdog_register_device() and drop those checks.
>>
>>> +
>>> +	mwd->wdt->state = MEI_WDT_START;
>>> +	wdd->timeout = mwd->wdt->timeout;
>>> +	return 0;
>>> +}
>>> +
>>> +/**
>>> + * mei_wdt_ops_stop - wd stop command from the watchdog core.
>>> + *
>>> + * @wdd: watchdog device
>>> + *
>>> + * Return: 0 if success, negative errno code for failure
>>> + */
>>> +static int mei_wdt_ops_stop(struct watchdog_device *wdd)
>>> +{
>>> +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
>>> +	struct mei_wdt *wdt;
>>> +	int ret;
>>> +
>>> +	if (!mwd)
>>> +		return -ENODEV;
>>> +
>>> +	wdt = mwd->wdt;
>>> +
>>> +	if (wdt->state != MEI_WDT_RUNNING)
>>> +		return 0;
>>> +
>>> +	wdt->state = MEI_WDT_STOPPING;
>>> +
>>> +	ret = mei_wdt_stop(wdt);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	wdt->state = MEI_WDT_IDLE;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +/**
>>> + * mei_wdt_ops_ping - wd ping command from the watchdog core.
>>> + *
>>> + * @wdd: watchdog device
>>> + *
>>> + * Return: 0 if success, negative errno code on failure
>>> + */
>>> +static int mei_wdt_ops_ping(struct watchdog_device *wdd)
>>> +{
>>> +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
>>> +	struct mei_wdt *wdt;
>>> +	int ret;
>>> +
>>> +	if (!mwd)
>>> +		return -ENODEV;
>>> +
>>> +	wdt = mwd->wdt;
>>> +
>>> +	if (wdt->state != MEI_WDT_START &&
>>> +	    wdt->state != MEI_WDT_RUNNING)
>>
>> Unnecessary continuation line ?
> Looks more readable to me. but we can also straight the condition.
>>
>>> +		return 0;
>>> +
>>> +	ret = mei_wdt_ping(wdt);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	wdt->state = MEI_WDT_RUNNING;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +/**
>>> + * mei_wdt_ops_set_timeout - wd set timeout command from the
>>> watchdog core.
>>> + *
>>> + * @wdd: watchdog device
>>> + * @timeout: timeout value to set
>>> + *
>>> + * Return: 0 if success, negative errno code for failure
>>> + */
>>> +static int mei_wdt_ops_set_timeout(struct watchdog_device *wdd,
>>> +				   unsigned int timeout)
>>> +{
>>> +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
>>> +	struct mei_wdt *wdt;
>>> +
>>> +	if (!mwd)
>>> +		return -ENODEV;
>>> +
>>> +	wdt = mwd->wdt;
>>> +
>>> +	/* valid value is already checked by the caller */
>>> +	wdt->timeout = timeout;
>>> +	wdd->timeout = timeout;
>>
>> One of those seems unnecessary. Why keep the timeout twice ?
>
> We need two as wdd may not exists and we still need to send ping to
> detect if the device is provisioned.
>

Ok, makes sense.

>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void mei_wdt_release(struct kref *ref)
>>> +{
>>> +	struct mei_wdt_dev *mwd = container_of(ref, struct
>>> mei_wdt_dev, refcnt);
>>> +
>>> +	kfree(mwd);
>>> +}
>>> +
>>> +static void mei_wdt_ops_ref(struct watchdog_device *wdd)
>>> +{
>>> +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
>>> +
>>> +	kref_get(&mwd->refcnt);
>>> +}
>>> +
>>> +static void mei_wdt_ops_unref(struct watchdog_device *wdd)
>>> +{
>>> +	struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
>>> +
>>> +	kref_put(&mwd->refcnt, mei_wdt_release);
>>> +}
>>> +
>>> +static const struct watchdog_ops wd_ops = {
>>> +	.owner       = THIS_MODULE,
>>> +	.start       = mei_wdt_ops_start,
>>> +	.stop        = mei_wdt_ops_stop,
>>> +	.ping        = mei_wdt_ops_ping,
>>> +	.set_timeout = mei_wdt_ops_set_timeout,
>>> +	.ref         = mei_wdt_ops_ref,
>>> +	.unref       = mei_wdt_ops_unref,
>>> +};
>>> +
>>> +static struct watchdog_info wd_info = {
>>> +	.identity = INTEL_AMT_WATCHDOG_ID,
>>> +	.options  = WDIOF_KEEPALIVEPING |
>>> +		    WDIOF_SETTIMEOUT |
>>> +		    WDIOF_ALARMONLY,
>>> +};
>>> +
>>> +static int mei_wdt_register(struct mei_wdt *wdt)
>>> +{
>>> +	struct mei_wdt_dev *mwd;
>>> +	struct device *dev;
>>> +	int ret;
>>> +
>>> +	if (!wdt || !wdt->cldev)
>>> +		return -EINVAL;
>>> +
>>> +	dev = &wdt->cldev->dev;
>>> +
>>> +	mwd = kzalloc(sizeof(struct mei_wdt_dev), GFP_KERNEL);
>>> +	if (!mwd)
>>> +		return -ENOMEM;
>>> +
>>> +	mwd->wdt = wdt;
>>> +	mwd->wdd.info = &wd_info;
>>> +	mwd->wdd.ops = &wd_ops;
>>> +	mwd->wdd.parent = dev;
>>> +	mwd->wdd.timeout = MEI_WDT_DEFAULT_TIMEOUT;
>>> +	mwd->wdd.min_timeout = MEI_WDT_MIN_TIMEOUT;
>>> +	mwd->wdd.max_timeout = MEI_WDT_MAX_TIMEOUT;
>>> +	kref_init(&mwd->refcnt);
>>> +
>>> +	ret = watchdog_register_device(&mwd->wdd);
>>> +	if (ret) {
>>> +		dev_err(dev, "unable to register watchdog device =
>>> %d.\n", ret);
>>> +		kref_put(&mwd->refcnt, mei_wdt_release);
>>> +		return ret;
>>> +	}
>>> +
>>> +	wdt->mwd = mwd;
>>> +	watchdog_set_drvdata(&mwd->wdd, mwd);
>>> +	return 0;
>>> +}
>>> +
>>> +static void mei_wdt_unregister(struct mei_wdt *wdt)
>>> +{
>>> +	if (!wdt->mwd)
>>> +		return;
>>> +
>>> +	watchdog_unregister_device(&wdt->mwd->wdd);
>>> +	kref_put(&wdt->mwd->refcnt, mei_wdt_release);
>>> +	wdt->mwd = NULL;

If setting this to NULL was really needed, you would have a race condition here:
wdt->mwd is set to NULL only after the pointer it points to was freed, leaving
a small window where the code above could still access it.

>>> +}
>>
>> Seems to me that using two separate data structures instead of one
>> adds a lot of complexity.
>
> It might be reduced but I'm not sure it can be significantly simpler.
> It the reference counter will be part of watchdog_device it would be
> simpler.
>

It would be there if struct watchdog_device was allocated by the
watchdog core, which is not the case. I am still trying to create
a situation where the local data structure (struct mei_wdt in this case)
can be released while the watchdog device is still open
(ie how to unprovision the watchdog device while in use).

Once I figure that out, I hope I can find a way to protect it
differently and drop the ref/unref callbacks. I suspect it may
be necessary to separate struct watchdog_device into two parts:
one used by drivers and one used by the watchdog core.
The watchdog core would then manage its own data structure, and
no longer depend on struct watchdog_device (owned by the driver)
to actually be there. Different story, though.

Thanks,
Guenter

RE: [char-misc-next 3/6] mei: wd: implement MEI iAMT watchdog driver

[Winkler, Tomas]

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


> >>
> >> Any special reason for having two data structures instead of one ?
> >> You could just move the variables from struct mei_wdt_dev into
> >> struct mei_wdt, no ?
> >
> > Yes, on newer platform mei_wdt_dev might be not present in case the the
> > device is not provisioned. This came to action in the following
> > patches.
> >
> 
> It is still an implementation choice to keep the data structures separate.
> That mei_wdt_dev can be unregistered doesn't mean that the data structure
> has to be destroyed or allocated on registration.

That's correct, the issue is that if the MEI device resets or go through suspend/resume the mei_wdt will be removed destroyed
Why watchdog daemon will still holds open handler to watchdog_device and will releases it upon ping failure. This is where reference counter will come in.
Currently we do not support seamless reset. 

> >>> +
> >>> +	/* valid value is already checked by the caller */
> >>> +	wdt->timeout = timeout;
> >>> +	wdd->timeout = timeout;
> >>
> >> One of those seems unnecessary. Why keep the timeout twice ?
> >
> > We need two as wdd may not exists and we still need to send ping to
> > detect if the device is provisioned.
> >
> 
> Ok, makes sense.
> 
> >>> +
> >>> +static void mei_wdt_unregister(struct mei_wdt *wdt)
> >>> +{
> >>> +	if (!wdt->mwd)
> >>> +		return;
> >>> +
> >>> +	watchdog_unregister_device(&wdt->mwd->wdd);
> >>> +	kref_put(&wdt->mwd->refcnt, mei_wdt_release);
> >>> +	wdt->mwd = NULL;
> 
> If setting this to NULL was really needed, you would have a race condition here:
> wdt->mwd is set to NULL only after the pointer it points to was freed, leaving
> a small window where the code above could still access it.


Yes, good catch will fix that.

> 
> >>> +}
> >>
> >> Seems to me that using two separate data structures instead of one
> >> adds a lot of complexity.
> >
> > It might be reduced but I'm not sure it can be significantly simpler.
> > It the reference counter will be part of watchdog_device it would be
> > simpler.
> >
> 
> It would be there if struct watchdog_device was allocated by the
> watchdog core, which is not the case. I am still trying to create
> a situation where the local data structure (struct mei_wdt in this case)
> can be released while the watchdog device is still open
> (ie how to unprovision the watchdog device while in use).

This happen on device suspend/resume or device reset, this is not the case of provisioning.
Currently the our bus layer doesn't support seamless reconnection. 

> Once I figure that out, I hope I can find a way to protect it
> differently and drop the ref/unref callbacks. I suspect it may
> be necessary to separate struct watchdog_device into two parts:
> one used by drivers and one used by the watchdog core.
> The watchdog core would then manage its own data structure, and
> no longer depend on struct watchdog_device (owned by the driver)
> to actually be there. Different story, though.


I think that would somehow fit also our driver. 

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 3/6] mei: wd: implement MEI iAMT watchdog driver

[Winkler, Tomas]

> >>
> >> Any special reason for having two data structures instead of one ?
> >> You could just move the variables from struct mei_wdt_dev into
> >> struct mei_wdt, no ?
> >
> > Yes, on newer platform mei_wdt_dev might be not present in case the the
> > device is not provisioned. This came to action in the following
> > patches.
> >
> 
> It is still an implementation choice to keep the data structures separate.
> That mei_wdt_dev can be unregistered doesn't mean that the data structure
> has to be destroyed or allocated on registration.

That's correct, the issue is that if the MEI device resets or go through suspend/resume the mei_wdt will be removed destroyed
Why watchdog daemon will still holds open handler to watchdog_device and will releases it upon ping failure. This is where reference counter will come in.
Currently we do not support seamless reset. 

> >>> +
> >>> +	/* valid value is already checked by the caller */
> >>> +	wdt->timeout = timeout;
> >>> +	wdd->timeout = timeout;
> >>
> >> One of those seems unnecessary. Why keep the timeout twice ?
> >
> > We need two as wdd may not exists and we still need to send ping to
> > detect if the device is provisioned.
> >
> 
> Ok, makes sense.
> 
> >>> +
> >>> +static void mei_wdt_unregister(struct mei_wdt *wdt)
> >>> +{
> >>> +	if (!wdt->mwd)
> >>> +		return;
> >>> +
> >>> +	watchdog_unregister_device(&wdt->mwd->wdd);
> >>> +	kref_put(&wdt->mwd->refcnt, mei_wdt_release);
> >>> +	wdt->mwd = NULL;
> 
> If setting this to NULL was really needed, you would have a race condition here:
> wdt->mwd is set to NULL only after the pointer it points to was freed, leaving
> a small window where the code above could still access it.


Yes, good catch will fix that.

> 
> >>> +}
> >>
> >> Seems to me that using two separate data structures instead of one
> >> adds a lot of complexity.
> >
> > It might be reduced but I'm not sure it can be significantly simpler.
> > It the reference counter will be part of watchdog_device it would be
> > simpler.
> >
> 
> It would be there if struct watchdog_device was allocated by the
> watchdog core, which is not the case. I am still trying to create
> a situation where the local data structure (struct mei_wdt in this case)
> can be released while the watchdog device is still open
> (ie how to unprovision the watchdog device while in use).

This happen on device suspend/resume or device reset, this is not the case of provisioning.
Currently the our bus layer doesn't support seamless reconnection. 

> Once I figure that out, I hope I can find a way to protect it
> differently and drop the ref/unref callbacks. I suspect it may
> be necessary to separate struct watchdog_device into two parts:
> one used by drivers and one used by the watchdog core.
> The watchdog core would then manage its own data structure, and
> no longer depend on struct watchdog_device (owned by the driver)
> to actually be there. Different story, though.


I think that would somehow fit also our driver. 

Thanks
Tomas