On Mon, 11 Aug 2008 21:23:01 +0200 (CEST)
Marcin Obara <marcin_obara@users.sourceforge.net> wrote:

quoted text
> Fixes small issues (2 comment and 2 variable type changes)
> raised by Pavel Machek since previous LKML submition.
> 
> 
> The Intel Management Engine Interface (aka HECI: Host Embedded
> Controller Interface ) enables communication between the host OS and
> the Management Engine firmware. MEI is bi-directional, and either the
> host or Intel AMT firmware can initiate transactions.
> 
> The core hardware architecture of Intel Active Management Technology
> (Intel AMT) is resident in firmware. The micro-controller within the
> chipset's graphics and memory controller (GMCH) hub houses the
> Management Engine (ME) firmware, which implements various services
> on behalf of management applications.
> 
> Some of the ME subsystems that can be access via MEI driver:
> 
> - Intel(R) Quiet System Technology (QST) is implemented as a firmware
> subsystem  that  runs in the ME.  Programs that wish to expose the
> health monitoring and fan speed control capabilities of Intel(R) QST
> will need to use the MEI driver to communicate with the ME sub-system.
> - ASF is the "Alert Standard Format" which is an DMTF manageability
> standard. It is implemented in the PC's hardware and firmware, and is
> managed from a remote console.
> 
> Most recent Intel desktop chipsets have one or more of the above ME
> services. The MEI driver will make it possible to support the above
> features on Linux and provides applications access to the ME and it's
> features.
> 

This code adds new kernel->userspace interfaces?

Please fully document those interfaces so that we can review the
proposed design.

What follows is a low-level review.  I didn't really address the
higher-level design aspects because even having read it, I don't know
what all this code does, because I wasn't told.  I could work that out,
but I'd prefer not to have to, because that means that everyone else
who wishes to maintain this code would need to do the same thing.  That
isn't very efficient.

quoted text
> 
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b343814..f473f95 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2247,6 +2247,15 @@ L:	e1000-devel@lists.sourceforge.net
>  W:	http://e1000.sourceforge.net/
>  S:	Supported
>  
> +INTEL MANAGEABILITY ENGINE INTERFACE (HECI)
> +P:	Anas Nashif
> +M: 	anas.nashif@intel.com
> +P:	Marcin Obara
> +M:	marcin.obara@intel.com
> +W:	http://www.openamt.org/
> +L:	openamt-devel@lists.sourceforge.net
> +S:	Supported
> +
>  INTEL PRO/WIRELESS 2100 NETWORK CONNECTION SUPPORT
>  P:	Zhu Yi
>  M:	yi.zhu@intel.com
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index caff851..a49821d 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -1103,6 +1103,7 @@ config DEVPORT
>  	default y
>  
>  source "drivers/s390/char/Kconfig"
> +source "drivers/char/heci/Kconfig"
>  
>  endmenu
>  
> diff --git a/drivers/char/Makefile b/drivers/char/Makefile
> index 6850f6d..8ad2f2c 100644
> --- a/drivers/char/Makefile
> +++ b/drivers/char/Makefile
> @@ -105,6 +105,7 @@ obj-$(CONFIG_IPMI_HANDLER)	+= ipmi/
>  
>  obj-$(CONFIG_HANGCHECK_TIMER)	+= hangcheck-timer.o
>  obj-$(CONFIG_TCG_TPM)		+= tpm/
> +obj-$(CONFIG_INTEL_MEI)	+= heci/
>  
>  obj-$(CONFIG_PS3_FLASH)		+= ps3flash.o
>  
> diff --git a/drivers/char/heci/Kconfig b/drivers/char/heci/Kconfig
> new file mode 100644
> index 0000000..99b7d05
> --- /dev/null
> +++ b/drivers/char/heci/Kconfig
> @@ -0,0 +1,12 @@
> +#
> +# HECI device configuration
> +#
> +
> +config INTEL_MEI
> +       tristate "Intel Management Engine Interface (MEI) Support (EXPERIMENTAL)"
> +       depends on EXPERIMENTAL
> +       ---help---
> +         The Intel Management Engine Interface (Intel MEI) driver allows
> +         applications to access the Active Management Technology 
> +         firmware and other Management Engine sub-systems.
> +
> diff --git a/drivers/char/heci/Makefile b/drivers/char/heci/Makefile
> new file mode 100644
> index 0000000..a2d7182
> --- /dev/null
> +++ b/drivers/char/heci/Makefile
> @@ -0,0 +1,7 @@
> +#
> +## Makefile for the Intel MEI driver (heci)
> +#
> +
> +obj-$(CONFIG_INTEL_MEI) += heci.o
> +
> +heci-objs := heci_init.o heci_interface.o heci_main.o interrupt.o io_heci.o
> diff --git a/drivers/char/heci/heci.h b/drivers/char/heci/heci.h
> new file mode 100644
> index 0000000..9b279f3
> --- /dev/null
> +++ b/drivers/char/heci/heci.h
> @@ -0,0 +1,176 @@
> +/*
> + * Part of Intel(R) Manageability Engine Interface Linux driver
> + *
> + * Copyright (c) 2003 - 2008 Intel Corp.
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions, and the following disclaimer,
> + *    without modification.
> + * 2. Redistributions in binary form must reproduce at minimum a disclaimer
> + *    substantially similar to the "NO WARRANTY" disclaimer below
> + *    ("Disclaimer") and any redistribution must be conditioned upon
> + *    including a substantially similar Disclaimer requirement for further
> + *    binary redistribution.
> + * 3. Neither the names of the above-listed copyright holders nor the names
> + *    of any contributors may be used to endorse or promote products derived
> + *    from this software without specific prior written permission.
> + *
> + * Alternatively, this software may be distributed under the terms of the
> + * GNU General Public License ("GPL") version 2 as published by the Free
> + * Software Foundation.
> + *
> + * NO WARRANTY
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * HOLDERS OR CONTRIBUTORS BE LIABLE FOR SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
> + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING
> + * IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGES.
> + *
> + */
> +
> +#ifndef _HECI_H_
> +#define _HECI_H_
> +
> +#include <linux/version.h>
> +#include <linux/spinlock.h>
> +#include <linux/list.h>
> +#include <linux/pci.h>
> +#include <linux/timer.h>
> +#include <linux/interrupt.h>
> +#include <linux/workqueue.h>
> +#include <linux/module.h>
> +#include <linux/aio.h>
> +#include <linux/types.h>
> +#include "heci_data_structures.h"
> +
> +extern const struct guid heci_pthi_guid;
> +extern const struct guid heci_wd_guid;
> +extern const __u8 heci_start_wd_params[];
> +extern const __u8 heci_stop_wd_params[];
> +extern const __u8 heci_wd_state_independence_msg[3][4];
> +
> +/*
> + * heci device ID
> + */
> +#define    HECI_DEV_ID_82946GZ    0x2974  /* 82946GZ/GL */
> +#define    HECI_DEV_ID_82G35      0x2984  /* 82G35 Express */
> +#define    HECI_DEV_ID_82Q965     0x2994  /* 82Q963/Q965 */
> +#define    HECI_DEV_ID_82G965     0x29A4  /* 82P965/G965 */
> +
> +#define    HECI_DEV_ID_82GM965    0x2A04  /* Mobile PM965/GM965 */
> +#define    HECI_DEV_ID_82GME965   0x2A14  /* Mobile GME965/GLE960 */
> +
> +#define    HECI_DEV_ID_ICH9_82Q35 0x29B4  /* 82Q35 Express */
> +#define    HECI_DEV_ID_ICH9_82G33 0x29C4  /* 82G33/G31/P35/P31 Express */
> +#define    HECI_DEV_ID_ICH9_82Q33 0x29D4  /* 82Q33 Express */
> +#define    HECI_DEV_ID_ICH9_82X38 0x29E4  /* 82X38/X48 Express */
> +#define    HECI_DEV_ID_ICH9_3200  0x29F4  /* 3200/3210 Server */
> +
> +#define    HECI_DEV_ID_ICH9_6     0x28B4  /* Bearlake */
> +#define    HECI_DEV_ID_ICH9_7     0x28C4  /* Bearlake */
> +#define    HECI_DEV_ID_ICH9_8     0x28D4  /* Bearlake */
> +#define    HECI_DEV_ID_ICH9_9     0x28E4  /* Bearlake */
> +#define    HECI_DEV_ID_ICH9_10    0x28F4  /* Bearlake */
> +
> +#define    HECI_DEV_ID_ICH9M_1    0x2A44  /* Cantiga */
> +#define    HECI_DEV_ID_ICH9M_2    0x2A54  /* Cantiga */
> +#define    HECI_DEV_ID_ICH9M_3    0x2A64  /* Cantiga */
> +#define    HECI_DEV_ID_ICH9M_4    0x2A74  /* Cantiga */
> +
> +#define    HECI_DEV_ID_ICH10_1    0x2E04  /* Eaglelake */
> +#define    HECI_DEV_ID_ICH10_2    0x2E14  /* Eaglelake */
> +#define    HECI_DEV_ID_ICH10_3    0x2E24  /* Eaglelake */
> +#define    HECI_DEV_ID_ICH10_4    0x2E34  /* Eaglelake */
> +
> +/*
> + * heci init function prototypes
> + */
> +struct iamt_heci_device *init_heci_device(struct pci_dev *pdev);
> +void heci_reset(struct iamt_heci_device *dev, int interrupts);
> +int heci_hw_init(struct iamt_heci_device *dev);
> +int heci_task_initialize_clients(void *data);
> +int heci_initialize_clients(struct iamt_heci_device *dev);
> +struct heci_file_private *heci_alloc_file_private(struct file *file);
> +int heci_disconnect_host_client(struct iamt_heci_device *dev,
> +				struct heci_file_private *file_ext);
> +void heci_initialize_list(struct io_heci_list *list,
> +			  struct iamt_heci_device *dev);
> +void heci_flush_list(struct io_heci_list *list,
> +		     struct heci_file_private *file_ext);
> +void heci_flush_queues(struct iamt_heci_device *dev,
> +		       struct heci_file_private *file_ext);
> +
> +void heci_remove_client_from_file_list(struct iamt_heci_device *dev,
> +				       __u8 host_client_id);
> +
> +/*
> + *  interrupt function prototype
> + */
> +irqreturn_t heci_isr_interrupt(int irq, void *dev_id);
> +
> +void heci_wd_timer(unsigned long data);
> +
> +/*
> + *  input output function prototype
> + */
> +int heci_ioctl_get_version(struct iamt_heci_device *dev, int if_num,
> +			   struct heci_message_data *u_msg,
> +			   struct heci_message_data k_msg,
> +			   struct heci_file_private *file_ext);
> +
> +int heci_ioctl_connect_client(struct iamt_heci_device *dev, int if_num,
> +			      struct heci_message_data *u_msg,
> +			      struct heci_message_data k_msg,
> +			      struct file *file);
> +
> +int heci_ioctl_wd(struct iamt_heci_device *dev, int if_num,
> +		  struct heci_message_data k_msg,
> +		  struct heci_file_private *file_ext);
> +
> +int heci_ioctl_bypass_wd(struct iamt_heci_device *dev, int if_num,
> +		  struct heci_message_data k_msg,
> +		  struct heci_file_private *file_ext);
> +
> +int heci_start_read(struct iamt_heci_device *dev, int if_num,
> +		    struct heci_file_private *file_ext);
> +
> +int pthi_write(struct iamt_heci_device *dev,
> +	       struct heci_cb_private *priv_cb);
> +
> +int pthi_read(struct iamt_heci_device *dev, int if_num, struct file *file,
> +	      char *ubuf, size_t length, loff_t *offset);
> +
> +struct heci_cb_private *find_pthi_read_list_entry(
> +			struct iamt_heci_device *dev,
> +			struct file *file);
> +
> +void run_next_iamthif_cmd(struct iamt_heci_device *dev);
> +
> +void heci_free_cb_private(struct heci_cb_private *priv_cb);
> +
> +/**
> + * heci_fe_same_id - tell if file private data have same id
> + *
> + * @fe1: private data of 1. file object
> + * @fe2: private data of 2. file object
> + *
> + * returns  !=0 - if ids are the same, 0 - if differ.
> + */
> +static inline int heci_fe_same_id(const struct heci_file_private *fe1,
> +				  const struct heci_file_private *fe2)
> +{
> +	return ((fe1->host_client_id == fe2->host_client_id)
> +		&& (fe1->me_client_id == fe2->me_client_id));
> +}
> +
> +#endif /* _HECI_H_ */
> diff --git a/drivers/char/heci/heci_data_structures.h b/drivers/char/heci/heci_data_structures.h
> new file mode 100644
> index 0000000..beacf7c
> --- /dev/null
> +++ b/drivers/char/heci/heci_data_structures.h
> @@ -0,0 +1,536 @@
> +/*
> + * Part of Intel(R) Manageability Engine Interface Linux driver
> + *
> + * Copyright (c) 2003 - 2008 Intel Corp.
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions, and the following disclaimer,
> + *    without modification.
> + * 2. Redistributions in binary form must reproduce at minimum a disclaimer
> + *    substantially similar to the "NO WARRANTY" disclaimer below
> + *    ("Disclaimer") and any redistribution must be conditioned upon
> + *    including a substantially similar Disclaimer requirement for further
> + *    binary redistribution.
> + * 3. Neither the names of the above-listed copyright holders nor the names
> + *    of any contributors may be used to endorse or promote products derived
> + *    from this software without specific prior written permission.
> + *
> + * Alternatively, this software may be distributed under the terms of the
> + * GNU General Public License ("GPL") version 2 as published by the Free
> + * Software Foundation.
> + *
> + * NO WARRANTY
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * HOLDERS OR CONTRIBUTORS BE LIABLE FOR SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
> + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING
> + * IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGES.
> + *
> + */
> +
> +#ifndef _HECI_DATA_STRUCTURES_H_
> +#define _HECI_DATA_STRUCTURES_H_
> +
> +#include <linux/version.h>
> +#include <linux/spinlock.h>
> +#include <linux/list.h>
> +#include <linux/pci.h>
> +#include <linux/timer.h>
> +#include <linux/interrupt.h>
> +#include <linux/workqueue.h>
> +#include <linux/module.h>
> +#include <linux/aio.h>
> +#include <linux/types.h>
> +
> +/*
> + * error code definition
> + */
> +#define     ESLOTS_OVERFLOW              1
> +#define     ECORRUPTED_MESSAGE_HEADER    1000
> +#define     ECOMPLETE_MESSAGE            1001

What's this?  The driver defines its onw errno numbers?

Are these ever returned to userspace?

This is risky/confusing/misleading, isn't it?

quoted text
> +#define     HECI_FC_MESSAGE_RESERVED_LENGTH           5
> +
> +/*
> + * Number of queue lists used by this driver
> + */
> +#define HECI_IO_LISTS_NUMBER        7
> +
> +/*
> + * Maximum transmission unit (MTU) of heci messages
> + */
> +#define IAMTHIF_MTU 4160
> +
> +
> +/*
> + * HECI HW Section
> + */
> +
> +/* HECI registers */
> +/* H_CB_WW - Host Circular Buffer (CB) Write Window register */
> +#define H_CB_WW    0
> +/* H_CSR - Host Control Status register */
> +#define H_CSR      4
> +/* ME_CB_RW - ME Circular Buffer Read Window register (read only) */
> +#define ME_CB_RW   8
> +/* ME_CSR_HA - ME Control Status Host Access register (read only) */
> +#define ME_CSR_HA  0xC
> +
> +
> +/* register bits of H_CSR (Host Control Status register) */
> +/* Host Circular Buffer Depth - maximum number of 32-bit entries in CB */
> +#define H_CBD             0xFF000000
> +/* Host Circular Buffer Write Pointer */
> +#define H_CBWP            0x00FF0000
> +/* Host Circular Buffer Read Pointer */
> +#define H_CBRP            0x0000FF00
> +/* Host Reset */
> +#define H_RST             0x00000010
> +/* Host Ready */
> +#define H_RDY             0x00000008
> +/* Host Interrupt Generate */
> +#define H_IG              0x00000004
> +/* Host Interrupt Status */
> +#define H_IS              0x00000002
> +/* Host Interrupt Enable */
> +#define H_IE              0x00000001
> +
> +
> +/* register bits of ME_CSR_HA (ME Control Status Host Access register) */
> +/* ME CB (Circular Buffer) Depth HRA (Host Read Access)
> + *  - host read only access to ME_CBD */
> +#define ME_CBD_HRA        0xFF000000
> +/* ME CB Write Pointer HRA - host read only access to ME_CBWP */
> +#define ME_CBWP_HRA       0x00FF0000
> +/* ME CB Read Pointer HRA - host read only access to ME_CBRP */
> +#define ME_CBRP_HRA       0x0000FF00
> +/* ME Reset HRA - host read only access to ME_RST */
> +#define ME_RST_HRA        0x00000010
> +/* ME Ready HRA - host read only access to ME_RDY */
> +#define ME_RDY_HRA        0x00000008
> +/* ME Interrupt Generate HRA - host read only access to ME_IG */
> +#define ME_IG_HRA         0x00000004
> +/* ME Interrupt Status HRA - host read only access to ME_IS */
> +#define ME_IS_HRA         0x00000002
> +/* ME Interrupt Enable HRA - host read only access to ME_IE */
> +#define ME_IE_HRA         0x00000001
> +
> +#define HECI_MINORS_BASE	1
> +#define HECI_MINORS_COUNT	1
> +
> +#define  HECI_MINOR_NUMBER	1
> +#define  HECI_MAX_OPEN_HANDLE_COUNT	253
> +
> +/*
> + * debug kernel print macro define
> + */
> +extern int heci_debug;
> +extern struct device *heci_dev;
> +
> +#define DBG(format, arg...) do { \
> +	if (heci_debug) { \
> +		if (heci_dev) \
> +			dev_info(heci_dev, "%s: " format, __func__, ## arg); \
> +		else \
> +			printk(KERN_INFO "heci: %s: " format, \
> +				 __func__, ## arg); \
> +	} \
> +} while (0)

This macro is a bit dangerous.  If code does

	DBG("%d", foo++)

then the value of `foo' will depend upon whether or not heci_debug is set.

A programmer would need to be damn stupid to pass an
expression-with-side-effects into a debugging macro, so I don't think
anything needs to be done here.  Just sayin'.


Actually, the bigger risk is code will do

	DBG("%d", foo->bar);

and `foo' is NULL.  We don't get to find out about the bug until
someone enables runtime debugging.

quoted text
> +
> +/*
> + * time to wait HECI become ready after init
> + */
> +#define HECI_INTEROP_TIMEOUT    (HZ * 7)
> +
> +/*
> + * watch dog definition
> + */
> +#define HECI_WATCHDOG_DATA_SIZE         16
> +#define HECI_START_WD_DATA_SIZE         20
> +#define HECI_WD_PARAMS_SIZE             4
> +#define HECI_WD_STATE_INDEPENDENCE_MSG_SENT       (1 << 0)
> +
> +#define HECI_WD_HOST_CLIENT_ID          1
> +#define HECI_IAMTHIF_HOST_CLIENT_ID     2
> +
> +struct guid {
> +	__u32 data1;
> +	__u16 data2;
> +	__u16 data3;
> +	__u8 data4[8];
> +};
> +
> +/* File state */
> +enum file_state {
> +	HECI_FILE_INITIALIZING = 0,
> +	HECI_FILE_CONNECTING,
> +	HECI_FILE_CONNECTED,
> +	HECI_FILE_DISCONNECTING,
> +	HECI_FILE_DISCONNECTED
> +};
> +
> +/* HECI device states */
> +enum heci_states {
> +	HECI_INITIALIZING = 0,
> +	HECI_ENABLED,
> +	HECI_RESETING,
> +	HECI_DISABLED,
> +	HECI_RECOVERING_FROM_RESET,
> +	HECI_POWER_DOWN,
> +	HECI_POWER_UP
> +};
> +
> +enum iamthif_states {
> +	HECI_IAMTHIF_IDLE,
> +	HECI_IAMTHIF_WRITING,
> +	HECI_IAMTHIF_FLOW_CONTROL,
> +	HECI_IAMTHIF_READING,
> +	HECI_IAMTHIF_READ_COMPLETE
> +};
> +
> +enum heci_file_transaction_states {
> +	HECI_IDLE,
> +	HECI_WRITING,
> +	HECI_WRITE_COMPLETE,
> +	HECI_FLOW_CONTROL,
> +	HECI_READING,
> +	HECI_READ_COMPLETE
> +};
> +
> +/* HECI CB */
> +enum heci_cb_major_types {
> +	HECI_READ = 0,
> +	HECI_WRITE,
> +	HECI_IOCTL,
> +	HECI_OPEN,
> +	HECI_CLOSE
> +};
> +
> +/* HECI user data struct */
> +struct heci_message_data {
> +	__u32 size;
> +	char *data;
> +} __attribute__((packed));
> +
> +#define HECI_CONNECT_TIMEOUT             3	/* at least 2 seconds */
> +
> +#define IAMTHIF_STALL_TIMER              12	/* seconds */
> +#define IAMTHIF_READ_TIMER               15	/* seconds */
> +
> +struct heci_cb_private {
> +	struct list_head cb_list;
> +	enum heci_cb_major_types major_file_operations;
> +	void *file_private;
> +	struct heci_message_data request_buffer;
> +	struct heci_message_data response_buffer;
> +	unsigned long information;
> +	unsigned long read_time;
> +	struct file *file_object;
> +};

I find that the most valuable code comments are those which document
the core data structures.  For each field: what its role in life is,
what its relationship is with other fields and other structures, what
its locking rules are, etc.

Right now, this reader of your code is wondering about cb_list.

- What is it?  I _think_ it's the means by which multiple instances
  of heci_cb_private are attached to a heci_file_private?

- What lock protects that list?  heci_file_private.file_lock,
  perhaps?  That was unobvious from a quick read of the code.

This stuff matters.  It's basically the first thing which a reviewer of
the design and implementation wants to know about, and that reviewer
doesn't want to have to dive intothe implementation, then
reverse-engineer the design, then go back and review how accurately the
code actually implements that design.

IOW: better code comments, please.  This is more than a cosmetic
nicety.  It is important for maintainability and for reviewability and
is worth spending quality time over.

Also, it is unusual that the code does
INIT_LIST_HEAD(heci_cb_private.cb_list) in two separate places.  Please
check that this was intended+correct+desirable.


In numerous places, this code does

	foo = (struct heci_file_private *)priv_cb_pos->file_private;

please remove all these typecasts of void*.  They are unneeded and they
actually remove type-safety.  If someone were to change priv_cb_pos to
a u8, they'll really want those compilation warnings/errors.

(OK, that's unlikely to happen, but the casts are ugly and add no
value).

quoted text
> +/* Private file struct */
> +struct heci_file_private {
> +	struct list_head link;
> +	struct file *file;
> +	enum file_state state;
> +	wait_queue_head_t tx_wait;
> +	wait_queue_head_t rx_wait;
> +	wait_queue_head_t wait;
> +	spinlock_t file_lock; /* file lock */
> +	spinlock_t read_io_lock; /* read lock */
> +	spinlock_t write_io_lock; /* write lock */
> +	int read_pending;
> +	int status;
> +	/* ID of client connected */
> +	__u8 host_client_id;
> +	__u8 me_client_id;
> +	__u8 flow_ctrl_creds;
> +	__u8 timer_count;
> +	enum heci_file_transaction_states reading_state;
> +	enum heci_file_transaction_states writing_state;
> +	int sm_state;
> +	struct heci_cb_private *read_cb;
> +};
> +
> +struct io_heci_list {
> +	struct heci_cb_private heci_cb;
> +	int status;
> +	struct iamt_heci_device *device_extension;
> +};

Again, the roles of and the relationship between the above three
structs is key to understanding the overall driver design.

quoted text
> +struct heci_driver_version {
> +	__u8 major;
> +	__u8 minor;
> +	__u8 hotfix;
> +	__u16 build;
> +} __attribute__((packed));
> +
> +
> +struct heci_client {
> +	__u32 max_msg_length;
> +	__u8 protocol_version;
> +} __attribute__((packed));

What are these?  They map onto some hardware-defined thing?  In IO
space?  DMA'ed into main memory?  Dunno.  Some explanation here would
help.

quoted text
> +/*
> + *  HECI BUS Interface Section
> + */
> +struct heci_msg_hdr {
> +	__u32 me_addr:8;
> +	__u32 host_addr:8;
> +	__u32 length:9;
> +	__u32 reserved:6;
> +	__u32 msg_complete:1;
> +} __attribute__((packed));
> +
> +
> +struct hbm_cmd {
> +	__u8 cmd:7;
> +	__u8 is_response:1;
> +} __attribute__((packed));
> +
> +
> +struct heci_bus_message {
> +	struct hbm_cmd cmd;
> +	__u8 command_specific_data[];
> +} __attribute__((packed));
> +
> +struct hbm_version {
> +	__u8 minor_version;
> +	__u8 major_version;
> +} __attribute__((packed));
> +
> +struct hbm_host_version_request {
> +	struct hbm_cmd cmd;
> +	__u8 reserved;
> +	struct hbm_version host_version;
> +} __attribute__((packed));
> +
> +struct hbm_host_version_response {
> +	struct hbm_cmd cmd;
> +	int host_version_supported;
> +	struct hbm_version me_max_version;
> +} __attribute__((packed));
> +
> +struct hbm_host_stop_request {
> +	struct hbm_cmd cmd;
> +	__u8 reason;
> +	__u8 reserved[2];
> +} __attribute__((packed));
> +
> +struct hbm_host_stop_response {
> +	struct hbm_cmd cmd;
> +	__u8 reserved[3];
> +} __attribute__((packed));
> +
> +struct hbm_me_stop_request {
> +	struct hbm_cmd cmd;
> +	__u8 reason;
> +	__u8 reserved[2];
> +} __attribute__((packed));
> +
> +struct hbm_host_enum_request {
> +	struct hbm_cmd cmd;
> +	__u8 reserved[3];
> +} __attribute__((packed));
> +
> +struct hbm_host_enum_response {
> +	struct hbm_cmd cmd;
> +	__u8 reserved[3];
> +	__u8 valid_addresses[32];
> +} __attribute__((packed));
> +
> +struct heci_client_properties {
> +	struct guid protocol_name;
> +	__u8 protocol_version;
> +	__u8 max_number_of_connections;
> +	__u8 fixed_address;
> +	__u8 single_recv_buf;
> +	__u32 max_msg_length;
> +} __attribute__((packed));
> +
> +struct hbm_props_request {
> +	struct hbm_cmd cmd;
> +	__u8 address;
> +	__u8 reserved[2];
> +} __attribute__((packed));
> +
> +
> +struct hbm_props_response {
> +	struct hbm_cmd cmd;
> +	__u8 address;
> +	__u8 status;
> +	__u8 reserved[1];
> +	struct heci_client_properties client_properties;
> +} __attribute__((packed));
> +
> +struct hbm_client_connect_request {
> +	struct hbm_cmd cmd;
> +	__u8 me_addr;
> +	__u8 host_addr;
> +	__u8 reserved;
> +} __attribute__((packed));
> +
> +struct hbm_client_connect_response {
> +	struct hbm_cmd cmd;
> +	__u8 me_addr;
> +	__u8 host_addr;
> +	__u8 status;
> +} __attribute__((packed));
> +
> +struct hbm_client_disconnect_request {
> +	struct hbm_cmd cmd;
> +	__u8 me_addr;
> +	__u8 host_addr;
> +	__u8 reserved[1];
> +} __attribute__((packed));
> +
> +struct hbm_flow_control {
> +	struct hbm_cmd cmd;
> +	__u8 me_addr;
> +	__u8 host_addr;
> +	__u8 reserved[HECI_FC_MESSAGE_RESERVED_LENGTH];
> +} __attribute__((packed));
> +
> +struct heci_me_client {
> +	struct heci_client_properties props;
> +	__u8 client_id;
> +	__u8 flow_ctrl_creds;
> +} __attribute__((packed));

Dittoes.

quoted text
> +/* private device struct */
> +struct iamt_heci_device {
> +	struct pci_dev *pdev;	/* pointer to pci device struct */
> +	/*
> +	 * lists of queues
> +	 */
> +	 /* array of pointers to  aio lists */

What is the relationship between this code and aio?  I note that
several files include linux/aio.h, which was a bit of a surprise.  No
other drivers do that.

This part of your design was perhaps more suited to
documentation-via-changelog than via code comments.

quoted text
> +	struct io_heci_list *io_list_array[HECI_IO_LISTS_NUMBER];
> +	struct io_heci_list read_list;	/* driver read queue */
> +	struct io_heci_list write_list;	/* driver write queue */
> +	struct io_heci_list write_waiting_list;	/* write waiting queue */
> +	struct io_heci_list ctrl_wr_list;	/* managed write IOCTL list */
> +	struct io_heci_list ctrl_rd_list;	/* managed read IOCTL list */
> +	struct io_heci_list pthi_cmd_list;	/* PTHI list for cmd waiting */

All protected by spin_lock_bh(iamt_heci_device.device_lock), it appears.

quoted text
> +	/* driver managed PTHI list for reading completed pthi cmd data */
> +	struct io_heci_list pthi_read_complete_list;
> +	/*
> +	 * list of files
> +	 */
> +	struct list_head file_list;

list-heads are nasty things.  Quite anonymous, quite type-unsafe.  It's
a bit hard to tell which list_ehad gets strung onto which anchoring
list_head, and the compiler cannot tell when a programmer screws this
up.  Suitable comments will help.


quoted text
> +	/*
> +	 * memory of device
> +	 */
> +	unsigned int mem_base;
> +	unsigned int mem_length;

What are these?

I'm surprised that they dont' have type resource_size_t or dma_addr_t
or void __iomem * or somesuch thing.

<reads the code a bit>

yup, resource_size_t.

quoted text
> +	char *mem_addr;

void __iomem *?

quoted text
> +	/*
> +	 * lock for the device

comment needs full detailing, please.

quoted text
> +	 */
> +	spinlock_t device_lock; /* device lock*/
> +	struct work_struct work;

What's this?

I see two separate sites which are doing a
PREPARE_WORK(iamt_heci_device.work).  This is quite unusual and
possibly incorrect.  Please check it all.  The one in
heci_isr_interrupt() is hopefully unneeded.

quoted text
> +	int recvd_msg;
> +
> +	struct task_struct *reinit_tsk;

What's this?

Seems to be protected by device_lock?

quoted text
> +	struct timer_list wd_timer;

See comments below.

quoted text
> +	/*
> +	 * hw states of host and fw(ME)
> +	 */
> +	__u32 host_hw_state;
> +	__u32 me_hw_state;

nit: __u32 is normally only needed in data structures which are to be
included by userspace.  This structure shouldn't be visible to
userspace compilation hence it could use plain old u32.

quoted text
> +	/*
> +	 * waiting queue for receive message from FW
> +	 */
> +	wait_queue_head_t wait_recvd_msg;
> +	wait_queue_head_t wait_stop_wd;
> +	/*
> +	 * heci device  states
> +	 */
> +	enum heci_states heci_state;
> +	int stop;
> +
> +	__u32 extra_write_index;
> +	__u32 rd_msg_buf[128];	/* used for control messages */
> +	__u32 wr_msg_buf[128];	/* used for control messages */
> +	__u32 ext_msg_buf[8];	/* for control responses    */
> +	__u32 rd_msg_hdr;

Seems strange that the above buffers are a per-device global.  It
implies that the driver can only do one-message-at-a-time.

quoted text
> +
> +	struct hbm_version version;
> +
> +	int host_buffer_is_empty;
> +	struct heci_file_private wd_file_ext;
> +	struct heci_me_client *me_clients; /* Note: memory has to be allocated*/
> +	__u8 heci_me_clients[32];	/* list of existing clients */
> +	__u8 num_heci_me_clients;
> +	__u8 heci_host_clients[32];	/* list of existing clients */

This is an "array", nbot a list.

quoted text
> +	__u8 current_host_client_id;
> +
> +	int wd_pending;
> +	int wd_stoped;

"stopped"

quoted text
> +	__u16 wd_timeout;	/* seconds ((wd_data[1] << 8) + wd_data[0]) */
> +	unsigned char wd_data[HECI_START_WD_DATA_SIZE];
> +
> +
> +	__u16 wd_due_counter;
> +	int asf_mode;
> +	int wd_bypass;	/* if 1, don't refresh watchdog ME client */
> +
> +	struct file *iamthif_file_object;
> +	struct heci_file_private iamthif_file_ext;
> +	int iamthif_ioctl;
> +	int iamthif_canceled;
> +	__u32 iamthif_timer;
> +	__u32 iamthif_stall_timer;
> +	unsigned char iamthif_msg_buf[IAMTHIF_MTU];
> +	__u32 iamthif_msg_buf_size;
> +	__u32 iamthif_msg_buf_index;
> +	int iamthif_flow_control_pending;
> +	enum iamthif_states iamthif_state;
> +
> +	struct heci_cb_private *iamthif_current_cb;
> +	__u8 write_hang;
> +	int need_reset;
> +	long open_handle_count;
> +
> +};
> +
> +/**
> + * read_heci_register - Read a byte from the heci device
> + *
> + * @dev: the device structure
> + * @offset: offset from which to read the data
> + *
> + * returns  the byte read.
> + */
> +static inline __u32 read_heci_register(struct iamt_heci_device *dev,
> +					unsigned long offset)
> +{
> +	return readl(dev->mem_addr + offset);
> +}
> +
> +/**
> + * write_heci_register - Write  4 bytes to the heci device
> + *
> + * @dev: the device structure
> + * @offset: offset from which to write the data
> + * @value: the byte to write
> + */
> +static inline void write_heci_register(struct iamt_heci_device *dev,
> +					unsigned long offset,  __u32 value)
> +{
> +	writel(value, dev->mem_addr + offset);
> +}
> +
> +#endif /* _HECI_DATA_STRUCTURES_H_ */
> diff --git a/drivers/char/heci/heci_init.c b/drivers/char/heci/heci_init.c
> new file mode 100644
> index 0000000..bdd9180
> --- /dev/null
> +++ b/drivers/char/heci/heci_init.c
> @@ -0,0 +1,1077 @@
> +/*
> + * Part of Intel(R) Manageability Engine Interface Linux driver
> + *
> + * Copyright (c) 2003 - 2008 Intel Corp.
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions, and the following disclaimer,
> + *    without modification.
> + * 2. Redistributions in binary form must reproduce at minimum a disclaimer
> + *    substantially similar to the "NO WARRANTY" disclaimer below
> + *    ("Disclaimer") and any redistribution must be conditioned upon
> + *    including a substantially similar Disclaimer requirement for further
> + *    binary redistribution.
> + * 3. Neither the names of the above-listed copyright holders nor the names
> + *    of any contributors may be used to endorse or promote products derived
> + *    from this software without specific prior written permission.
> + *
> + * Alternatively, this software may be distributed under the terms of the
> + * GNU General Public License ("GPL") version 2 as published by the Free
> + * Software Foundation.
> + *
> + * NO WARRANTY
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * HOLDERS OR CONTRIBUTORS BE LIABLE FOR SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
> + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING
> + * IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGES.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/reboot.h>
> +#include <linux/poll.h>
> +#include <linux/init.h>
> +#include <linux/kdev_t.h>
> +#include <linux/moduleparam.h>
> +#include <linux/wait.h>
> +#include <linux/delay.h>
> +#include <linux/kthread.h>
> +
> +#include "heci_data_structures.h"
> +#include "heci_interface.h"
> +#include "heci.h"
> +
> +
> +const __u8 heci_start_wd_params[] = { 0x02, 0x12, 0x13, 0x10 };
> +const __u8 heci_stop_wd_params[] = { 0x02, 0x02, 0x14, 0x10 };
> +
> +const __u8 heci_wd_state_independence_msg[3][4] = {
> +	{0x05, 0x02, 0x51, 0x10},
> +	{0x05, 0x02, 0x52, 0x10},
> +	{0x07, 0x02, 0x01, 0x10}
> +};
> +
> +const struct guid heci_asf_guid = {
> +	0x75B30CD6, 0xA29E, 0x4AF7,
> +	{0xA7, 0x12, 0xE6, 0x17, 0x43, 0x93, 0xC8, 0xA6}
> +};

This can have static scope.

(Please check the whole driver for needlessly-global symbols)

quoted text
> +const struct guid heci_wd_guid = {
> +	0x05B79A6F, 0x4628, 0x4D7F,
> +	{0x89, 0x9D, 0xA9, 0x15, 0x14, 0xCB, 0x32, 0xAB}
> +};
> +const struct guid heci_pthi_guid = {
> +	0x12f80028, 0xb4b7, 0x4b2d,
> +	{0xac, 0xa8, 0x46, 0xe0, 0xff, 0x65, 0x81, 0x4c}
> +};
> +
> +
> +/*
> + *  heci init function prototypes
> + */
> +static void heci_check_asf_mode(struct iamt_heci_device *dev);
> +static int host_start_message(struct iamt_heci_device *dev);
> +static int host_enum_clients_message(struct iamt_heci_device *dev);
> +static int allocate_me_clients_storage(struct iamt_heci_device *dev);
> +static void host_init_wd(struct iamt_heci_device *dev);
> +static void host_init_iamthif(struct iamt_heci_device *dev);
> +static int heci_wait_event_int_timeout(struct iamt_heci_device *dev,
> +				       long timeout);
> +
> +
> +/**
> + * heci_initialize_list - Sets up a queue list.
> + *
> + * @list: An instance of our list structure
> + * @dev: Device object for our driver
> + */
> +void heci_initialize_list(struct io_heci_list *list,
> +			  struct iamt_heci_device *dev)
> +{
> +	/* initialize our queue list */
> +	INIT_LIST_HEAD(&list->heci_cb.cb_list);
> +	list->status = 0;
> +	list->device_extension = dev;
> +}
> +
> +/**
> + * heci_flush_queues - flush our queues list belong to file_ext.
> + *
> + * @dev: Device object for our driver
> + * @file_ext: private data of the file object
> + */
> +void heci_flush_queues(struct iamt_heci_device *dev,
> +		       struct heci_file_private *file_ext)
> +{
> +	int i;
> +
> +	if (!dev || !file_ext)
> +		return;
> +
> +	/* flush our queue list belong to file_ext */
> +	for (i = 0; i < HECI_IO_LISTS_NUMBER; i++) {
> +		DBG("remove list entry belong to file_ext\n");
> +		heci_flush_list(dev->io_list_array[i], file_ext);
> +	}
> +}
> +
> +
> +/**
> + * heci_flush_list - remove list entry belong to file_ext.
> + *
> + * @list:  An instance of our list structure
> + * @file_ext: private data of the file object

Part of this function's interface is most definitely "must be called
under device_lock".  That's just as worthy of documentation as the
formal argument.  Unfortunately the kernel-doc system fails to
formalise this sort of thing.

One way of firmly documenting this is assert_spin_locked().  Your call.

quoted text
> + */
> +void heci_flush_list(struct io_heci_list *list,
> +		struct heci_file_private *file_ext)
> +{
> +	struct heci_file_private *file_ext_tmp;
> +	struct heci_cb_private *priv_cb_pos = NULL;
> +	struct heci_cb_private *priv_cb_next = NULL;
> +
> +	if (!list || !file_ext)
> +		return;

Can this happen?

quoted text
> +	if (list->status != 0)
> +		return;
> +
> +	if (list_empty(&list->heci_cb.cb_list))
> +		return;
> +
> +	list_for_each_entry_safe(priv_cb_pos, priv_cb_next,
> +				 &list->heci_cb.cb_list, cb_list) {
> +		if (priv_cb_pos) {
> +			file_ext_tmp = (struct heci_file_private *)
> +				priv_cb_pos->file_private;
> +			if (file_ext_tmp) {
> +				if (heci_fe_same_id(file_ext, file_ext_tmp))
> +					list_del(&priv_cb_pos->cb_list);
> +			}
> +		}
> +	}
> +}
> +
> +/**
> + * heci_reset_iamthif_params - initializes heci device iamthif
> + *
> + * @dev: The heci device structure
> + */
> +static void heci_reset_iamthif_params(struct iamt_heci_device *dev)
> +{
> +	/* reset iamthif parameters. */
> +	dev->iamthif_current_cb = NULL;
> +	dev->iamthif_msg_buf_size = 0;
> +	dev->iamthif_msg_buf_index = 0;
> +	dev->iamthif_canceled = 0;
> +	dev->iamthif_file_ext.file = NULL;
> +	dev->iamthif_ioctl = 0;
> +	dev->iamthif_state = HECI_IAMTHIF_IDLE;
> +	dev->iamthif_timer = 0;
> +}

It is unusual for a driver to unconditionally zap lots of per-device
fields like this during normal operation.

quoted text
> +/**
> + * init_heci_device - allocates and initializes the heci device structure
> + *
> + * @pdev: The pci device structure
> + *
> + * returns The heci_device_device pointer on success, NULL on failure.
> + */
> +struct iamt_heci_device *init_heci_device(struct pci_dev *pdev)
> +{
> +	int i;
> +	struct iamt_heci_device *dev;
> +
> +	dev = kzalloc(sizeof(struct iamt_heci_device), GFP_KERNEL);
> +	if (!dev)
> +		return NULL;
> +
> +	/* setup our list array */
> +	dev->io_list_array[0] = &dev->read_list;
> +	dev->io_list_array[1] = &dev->write_list;
> +	dev->io_list_array[2] = &dev->write_waiting_list;
> +	dev->io_list_array[3] = &dev->ctrl_wr_list;
> +	dev->io_list_array[4] = &dev->ctrl_rd_list;
> +	dev->io_list_array[5] = &dev->pthi_cmd_list;
> +	dev->io_list_array[6] = &dev->pthi_read_complete_list;
> +	INIT_LIST_HEAD(&dev->file_list);
> +	INIT_LIST_HEAD(&dev->wd_file_ext.link);
> +	INIT_LIST_HEAD(&dev->iamthif_file_ext.link);
> +	spin_lock_init(&dev->device_lock);
> +	init_waitqueue_head(&dev->wait_recvd_msg);
> +	init_waitqueue_head(&dev->wait_stop_wd);
> +	dev->heci_state = HECI_INITIALIZING;
> +	dev->iamthif_state = HECI_IAMTHIF_IDLE;
> +
> +	/* init work for schedule work */
> +	INIT_WORK(&dev->work, NULL);
> +	for (i = 0; i < HECI_IO_LISTS_NUMBER; i++)
> +		heci_initialize_list(dev->io_list_array[i], dev);
> +	dev->pdev = pdev;
> +	return dev;
> +}
> +
> +
> +
> +

Various stray newlines..

quoted text
> +static int heci_wait_event_int_timeout(struct iamt_heci_device *dev,
> +		long timeout)
> +{
> +	return wait_event_interruptible_timeout(dev->wait_recvd_msg,
> +			(dev->recvd_msg), timeout);
> +}

See above comments regarding *_interruptible* dangers.

quoted text
> +/**
> + * heci_hw_init  - init host and fw to start work.
> + *
> + * @dev: Device object for our driver
> + *
> + * returns 0 on success, <0 on failure.
> + */
> +int heci_hw_init(struct iamt_heci_device *dev)
> +{
> +	int err = 0;
> +
> +	dev->host_hw_state = read_heci_register(dev, H_CSR);
> +	dev->me_hw_state = read_heci_register(dev, ME_CSR_HA);
> +	DBG("host_hw_state = 0x%08x, mestate = 0x%08x.\n",
> +	    dev->host_hw_state, dev->me_hw_state);
> +
> +	if ((dev->host_hw_state & H_IS) == H_IS) {
> +		/* acknowledge interrupt and stop interupts */
> +		heci_set_csr_register(dev);
> +	}
> +	dev->recvd_msg = 0;
> +	DBG("reset in start the heci device.\n");
> +
> +	heci_reset(dev, 1);
> +
> +	DBG("host_hw_state = 0x%08x, me_hw_state = 0x%08x.\n",
> +	    dev->host_hw_state, dev->me_hw_state);
> +
> +	/* wait for ME to turn on ME_RDY */
> +	if (!dev->recvd_msg)
> +		err = heci_wait_event_int_timeout(dev, HECI_INTEROP_TIMEOUT);
> +
> +	if (!err && !dev->recvd_msg) {
> +		dev->heci_state = HECI_DISABLED;
> +		DBG("wait_event_interruptible_timeout failed"
> +		    "on wait for ME to turn on ME_RDY.\n");
> +		return -ENODEV;
> +	} else {
> +		if (!(((dev->host_hw_state & H_RDY) == H_RDY)
> +		      && ((dev->me_hw_state & ME_RDY_HRA) == ME_RDY_HRA))) {
> +			dev->heci_state = HECI_DISABLED;
> +			DBG("host_hw_state = 0x%08x, me_hw_state = 0x%08x.\n",
> +			    dev->host_hw_state,
> +			    dev->me_hw_state);
> +
> +			if (!(dev->host_hw_state & H_RDY) != H_RDY)
> +				DBG("host turn off H_RDY.\n");
> +
> +			if (!(dev->me_hw_state & ME_RDY_HRA) != ME_RDY_HRA)
> +				DBG("ME turn off ME_RDY.\n");
> +
> +			printk(KERN_ERR
> +			       "heci: link layer initialization failed.\n");
> +			return -ENODEV;
> +		}
> +	}

If heci_wait_event_int_timeout() got terminated due to a pending
signal, this code drops that information on the floor. 
heci_wait_event_int_timeout() will have returned before
that-which-it-was-waiting-for has happened, and I suspect that bad
things will happen.

Please check the signal_pending() behaviour in here, and try to runtime
test it.

The same goes for all foo_interruptible code sites in this driver.  It
is a dangerous function call to be used in a driver.  Few people will
remember to test the it-returned-early-due-to-signal_pending case.

quoted text
> +	dev->recvd_msg = 0;
> +	DBG("host_hw_state = 0x%08x, me_hw_state = 0x%08x.\n",
> +	    dev->host_hw_state, dev->me_hw_state);
> +	DBG("ME turn on ME_RDY and host turn on H_RDY.\n");
> +	printk(KERN_INFO "heci: link layer has been established.\n");
> +	return 0;
> +}
> +
> +/**
> + * heci_hw_reset - reset fw via heci csr register.
> + *
> + * @dev: Device object for our driver
> + * @interrupts: if interrupt should be enable after reset.
> + */
> +static void heci_hw_reset(struct iamt_heci_device *dev, int interrupts)
> +{
> +	dev->host_hw_state |= (H_RST | H_IG);
> +
> +	if (interrupts)
> +		heci_csr_enable_interrupts(dev);
> +	else
> +		heci_csr_disable_interrupts(dev);
> +
> +	BUG_ON((dev->host_hw_state & H_RST) != H_RST);
> +	BUG_ON((dev->host_hw_state & H_RDY) != 0);
> +}
> +
> +/**
> + * heci_reset - reset host and fw.
> + *
> + * @dev: Device object for our driver
> + * @interrupts: if interrupt should be enable after reset.

"Called under spin_lock_bh(device_lock)"

quoted text
> + */
> +void heci_reset(struct iamt_heci_device *dev, int interrupts)
> +{
> +	struct heci_file_private *file_pos = NULL;
> +	struct heci_file_private *file_next = NULL;
> +	struct heci_cb_private *priv_cb_pos = NULL;
> +	struct heci_cb_private *priv_cb_next = NULL;
> +	int unexpected = 0;
> +
> +	if (dev->heci_state == HECI_RECOVERING_FROM_RESET) {
> +		dev->need_reset = 1;
> +		return;
> +	}
> +
> +	if (dev->heci_state != HECI_INITIALIZING &&
> +	    dev->heci_state != HECI_DISABLED &&
> +	    dev->heci_state != HECI_POWER_DOWN &&
> +	    dev->heci_state != HECI_POWER_UP)
> +		unexpected = 1;
> +
> +	if (dev->reinit_tsk != NULL) {
> +		kthread_stop(dev->reinit_tsk);

kthread_stop() can sleep, hence cannnot be called from under spinlock.

quoted text
> +		dev->reinit_tsk = NULL;
> +	}
> +
> +	dev->host_hw_state = read_heci_register(dev, H_CSR);
> +
> +	DBG("before reset host_hw_state = 0x%08x.\n",
> +	    dev->host_hw_state);
> +
> +	heci_hw_reset(dev, interrupts);
> +
> +	dev->host_hw_state &= ~H_RST;
> +	dev->host_hw_state |= H_IG;
> +
> +	write_heci_register(dev, H_CSR, dev->host_hw_state);
> +
> +	DBG("currently saved host_hw_state = 0x%08x.\n",
> +	    dev->host_hw_state);
> +
> +	dev->need_reset = 0;
> +
> +	if (dev->heci_state != HECI_INITIALIZING) {
> +		if ((dev->heci_state != HECI_DISABLED) &&
> +		    (dev->heci_state != HECI_POWER_DOWN))
> +			dev->heci_state = HECI_RESETING;
> +
> +		list_for_each_entry_safe(file_pos,
> +				file_next, &dev->file_list, link) {
> +			file_pos->state = HECI_FILE_DISCONNECTED;
> +			file_pos->flow_ctrl_creds = 0;
> +			file_pos->read_cb = NULL;
> +			file_pos->timer_count = 0;
> +		}
> +		/* remove entry if already in list */
> +		DBG("list del iamthif and wd file list.\n");
> +		heci_remove_client_from_file_list(dev,
> +				dev->wd_file_ext.host_client_id);
> +
> +		heci_remove_client_from_file_list(dev,
> +				dev->iamthif_file_ext.host_client_id);
> +
> +		heci_reset_iamthif_params(dev);
> +		dev->wd_due_counter = 0;
> +		dev->extra_write_index = 0;
> +	}
> +
> +	dev->num_heci_me_clients = 0;
> +	dev->rd_msg_hdr = 0;
> +	dev->stop = 0;
> +	dev->wd_pending = 0;
> +
> +	/* update the state of the registers after reset */
> +	dev->host_hw_state =  read_heci_register(dev, H_CSR);
> +	dev->me_hw_state =  read_heci_register(dev, ME_CSR_HA);
> +
> +	DBG("after reset host_hw_state = 0x%08x, me_hw_state = 0x%08x.\n",
> +	    dev->host_hw_state, dev->me_hw_state);
> +
> +	if (unexpected)
> +		printk(KERN_WARNING "heci: unexpected reset.\n");
> +
> +	/* Wake up all readings so they can be interrupted */
> +	list_for_each_entry_safe(file_pos, file_next, &dev->file_list, link) {
> +		if (&file_pos->rx_wait &&
> +		    waitqueue_active(&file_pos->rx_wait)) {

The use of waitqueue_active() has well-known races, only I forget the
details.  Barrier-related, iirc.  Nick Piggin might recall the details.
We should have added a comment to waitqueue_active().

quoted text
> +			printk(KERN_INFO "heci: Waking up client!\n");
> +			wake_up_interruptible(&file_pos->rx_wait);
> +		}
> +	}
> +	/* remove all waiting requests */
> +	if (dev->write_list.status == 0 &&
> +		!list_empty(&dev->write_list.heci_cb.cb_list)) {
> +		list_for_each_entry_safe(priv_cb_pos, priv_cb_next,
> +				&dev->write_list.heci_cb.cb_list, cb_list) {
> +			if (priv_cb_pos) {
> +				list_del(&priv_cb_pos->cb_list);
> +				heci_free_cb_private(priv_cb_pos);
> +			}
> +		}
> +	}
> +}
> +
> +/**
> + * heci_initialize_clients - heci communication initialization.
> + *
> + * @dev: Device object for our driver
> + */
> +int heci_initialize_clients(struct iamt_heci_device *dev)
> +{
> +	int status;
> +
> +	msleep(100); /* FW needs time to be ready to talk with us */
> +	DBG("link is established start sending messages.\n");
> +	/* link is established start sending messages. */
> +	status = host_start_message(dev);
> +	if (status != 0) {
> +		spin_lock_bh(&dev->device_lock);
> +		dev->heci_state = HECI_DISABLED;
> +		spin_unlock_bh(&dev->device_lock);
> +		DBG("start sending messages failed.\n");
> +		return status;
> +	}
> +
> +	/* enumerate clients */
> +	status = host_enum_clients_message(dev);
> +	if (status != 0) {
> +		spin_lock_bh(&dev->device_lock);
> +		dev->heci_state = HECI_DISABLED;
> +		spin_unlock_bh(&dev->device_lock);
> +		DBG("enum clients failed.\n");
> +		return status;
> +	}
> +	/* allocate storage for ME clients representation */
> +	status = allocate_me_clients_storage(dev);
> +	if (status != 0) {
> +		spin_lock_bh(&dev->device_lock);
> +		dev->num_heci_me_clients = 0;
> +		dev->heci_state = HECI_DISABLED;
> +		spin_unlock_bh(&dev->device_lock);
> +		DBG("allocate clients failed.\n");
> +		return status;
> +	}
> +
> +	heci_check_asf_mode(dev);
> +	/* heci watchdog initialization */
> +	host_init_wd(dev);
> +	/* heci iamthif client initialization */
> +	host_init_iamthif(dev);
> +
> +	spin_lock_bh(&dev->device_lock);
> +	if (dev->need_reset) {
> +		dev->need_reset = 0;
> +		dev->heci_state = HECI_DISABLED;
> +		spin_unlock_bh(&dev->device_lock);
> +		return -ENODEV;

I'm surprised that we can just retrun here without having to release
any resources.

quoted text
> +	}
> +
> +	memset(dev->heci_host_clients, 0, sizeof(dev->heci_host_clients));

I suspect this memset wasn't needed.

quoted text
> +	dev->open_handle_count = 0;
> +	dev->heci_host_clients[0] |= 7;

Mysterious.

quoted text
> +	dev->current_host_client_id = 3;
> +	dev->heci_state = HECI_ENABLED;
> +	spin_unlock_bh(&dev->device_lock);
> +	DBG("initialization heci clients successful.\n");
> +	return 0;
> +}
> +
> +/**
> + * heci_task_initialize_clients - heci reinitialization task
> + *
> + * @data: Device object for our driver
> + */
> +int heci_task_initialize_clients(void *data)
> +{
> +	int ret;
> +	struct iamt_heci_device *dev = (struct iamt_heci_device *) data;

Unneeded and undesirable cast of void*.

quoted text
> +	spin_lock_bh(&dev->device_lock);
> +	if (dev->reinit_tsk != NULL) {
> +		spin_unlock_bh(&dev->device_lock);
> +		DBG("reinit task already started.\n");
> +		return 0;
> +	}
> +	dev->reinit_tsk = current;
> +	current->flags |= PF_NOFREEZE;

A manipulation such as this must have a comment.  Because there's
really no way in whcih the reader of the code can work out why it is
there.

And a driver shouldn't be accessing PF_NOFREEZE directly.  There are
various interfaces in freezer.h whcih should be used instead (I forget
which one - it has changed a few times).


quoted text
> +	spin_unlock_bh(&dev->device_lock);
> +
> +	ret = heci_initialize_clients(dev);
> +
> +	spin_lock_bh(&dev->device_lock);
> +	dev->reinit_tsk = NULL;
> +	spin_unlock_bh(&dev->device_lock);

This looks racy.  It could at least do

	if (dev->reinit_tsk == current)
		dev->reinit_tsk = NULL;

but it all still seems a bit dodgy.  Perhaps reinit_tsk should be
protected by a sleeping lock (ie: a mutex).


quoted text
> +	return ret;
> +}
> +
> +/**
> + * host_start_message - heci host send start message.
> + *
> + * @dev: Device object for our driver
> + *
> + * returns 0 on success, <0 on failure.
> + */
> +static int host_start_message(struct iamt_heci_device *dev)
> +{
> +	long timeout = 60;	/* 60 second */
> +
> +	struct heci_msg_hdr *heci_hdr;
> +	struct hbm_host_version_request *host_start_req;
> +	struct hbm_host_stop_request *host_stop_req;
> +	int err = 0;
> +
> +	/* host start message */
> +	heci_hdr = (struct heci_msg_hdr *) &dev->wr_msg_buf[0];

COuld we make this code cleaner and more typesafe by defining
wr_msg_buf as a union of heci_msg_hdr, hbm_cmd, etc?

quoted text
> +	heci_hdr->host_addr = 0;
> +	heci_hdr->me_addr = 0;
> +	heci_hdr->length = sizeof(struct hbm_host_version_request);
> +	heci_hdr->msg_complete = 1;
> +	heci_hdr->reserved = 0;
> +
> +	host_start_req =
> +	    (struct hbm_host_version_request *) &dev->wr_msg_buf[1];
> +	memset(host_start_req, 0, sizeof(struct hbm_host_version_request));
> +	host_start_req->cmd.cmd = HOST_START_REQ_CMD;
> +	host_start_req->host_version.major_version = HBM_MAJOR_VERSION;
> +	host_start_req->host_version.minor_version = HBM_MINOR_VERSION;
> +	dev->recvd_msg = 0;
> +	if (!heci_write_message(dev, heci_hdr,
> +				       (unsigned char *) (host_start_req),

Maybe heci_write_message() should take a void*.

quoted text
> +				       heci_hdr->length)) {
> +		DBG("send version to fw fail.\n");
> +		return -ENODEV;
> +	}
> +	DBG("call wait_event_interruptible_timeout for response message.\n");
> +	/* wait for response */
> +	err = heci_wait_event_int_timeout(dev, timeout * HZ);
> +	if (!err && !dev->recvd_msg) {
> +		DBG("wait_timeout failed on host start response message.\n");
> +		return -ENODEV;
> +	}
> +	dev->recvd_msg = 0;
> +	DBG("wait_timeout successful on host start response message.\n");
> +	if ((dev->version.major_version != HBM_MAJOR_VERSION) ||
> +	    (dev->version.minor_version != HBM_MINOR_VERSION)) {
> +		/* send stop message */
> +		heci_hdr->host_addr = 0;
> +		heci_hdr->me_addr = 0;
> +		heci_hdr->length = sizeof(struct hbm_host_stop_request);
> +		heci_hdr->msg_complete = 1;
> +		heci_hdr->reserved = 0;
> +
> +		host_stop_req =
> +		    (struct hbm_host_stop_request *) &dev->wr_msg_buf[1];
> +
> +		memset(host_stop_req, 0, sizeof(struct hbm_host_stop_request));
> +		host_stop_req->cmd.cmd = HOST_STOP_REQ_CMD;
> +		host_stop_req->reason = DRIVER_STOP_REQUEST;
> +		heci_write_message(dev, heci_hdr,
> +				   (unsigned char *) (host_stop_req),
> +				   heci_hdr->length);
> +		DBG("version mismatch.\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * host_enum_clients_message - host send enumeration client request message.
> + *
> + * @dev: Device object for our driver
> + *
> + * returns 0 on success, <0 on failure.
> + */
> +static int host_enum_clients_message(struct iamt_heci_device *dev)
> +{
> +	long timeout = 5;	/*5 second */
> +	struct heci_msg_hdr *heci_hdr;
> +	struct hbm_host_enum_request *host_enum_req;
> +	int err = 0;
> +	int i, j;
> +
> +	heci_hdr = (struct heci_msg_hdr *) &dev->wr_msg_buf[0];
> +	/* enumerate clients */
> +	heci_hdr->host_addr = 0;
> +	heci_hdr->me_addr = 0;
> +	heci_hdr->length = sizeof(struct hbm_host_enum_request);
> +	heci_hdr->msg_complete = 1;
> +	heci_hdr->reserved = 0;
> +
> +	host_enum_req = (struct hbm_host_enum_request *) &dev->wr_msg_buf[1];
> +	memset(host_enum_req, 0, sizeof(struct hbm_host_enum_request));
> +	host_enum_req->cmd.cmd = HOST_ENUM_REQ_CMD;
> +	if (!heci_write_message(dev, heci_hdr,
> +			       (unsigned char *) (host_enum_req),
> +			       heci_hdr->length)) {
> +		DBG("send enumeration request failed.\n");
> +		return -ENODEV;
> +	}
> +	/* wait for response */
> +	dev->recvd_msg = 0;
> +	err = heci_wait_event_int_timeout(dev, timeout * HZ);
> +	if (!err && !dev->recvd_msg) {
> +		DBG("wait_event_interruptible_timeout failed "
> +				"on enumeration clients response message.\n");
> +		return -ENODEV;
> +	}
> +	dev->recvd_msg = 0;
> +
> +	spin_lock_bh(&dev->device_lock);
> +	/* count how many ME clients we have */
> +	for (i = 0; i < sizeof(dev->heci_me_clients); i++) {
> +		for (j = 0; j < 8; j++) {
> +			if ((dev->heci_me_clients[i] & (1 << j)) != 0)
> +				dev->num_heci_me_clients++;
> +
> +		}
> +	}
> +	spin_unlock_bh(&dev->device_lock);
> +
> +	return 0;
> +}

There seems to be a bit of code duplication here.

quoted text
> +/**
> + * host_client_properties - reads properties for client
> + *
> + * @dev: Device object for our driver
> + * @idx: client index in me client array
> + * @client_id: id of the client
> + *
> + * returns 0 on success, <0 on failure.
> + */
> +static int host_client_properties(struct iamt_heci_device *dev,
> +				  struct heci_me_client *client)
> +{
> +	struct heci_msg_hdr *heci_hdr;
> +	struct hbm_props_request *host_cli_req;
> +	int err;
> +
> +	heci_hdr = (struct heci_msg_hdr *) &dev->wr_msg_buf[0];
> +	heci_hdr->host_addr = 0;
> +	heci_hdr->me_addr = 0;
> +	heci_hdr->length = sizeof(struct hbm_props_request);
> +	heci_hdr->msg_complete = 1;
> +	heci_hdr->reserved = 0;
> +
> +	host_cli_req = (struct hbm_props_request *) &dev->wr_msg_buf[1];
> +	memset(host_cli_req, 0, sizeof(struct hbm_props_request));
> +	host_cli_req->cmd.cmd = HOST_CLIENT_PROPERTEIS_REQ_CMD;
> +	host_cli_req->address = client->client_id;
> +	if (!heci_write_message(dev, heci_hdr,
> +				(unsigned char *) (host_cli_req),
> +				heci_hdr->length)) {
> +		DBG("send props request failed.\n");
> +		return -ENODEV;
> +	}
> +	/* wait for response */
> +	dev->recvd_msg = 0;
> +	err = heci_wait_event_int_timeout(dev, 10 * HZ);
> +	if (!err && !dev->recvd_msg) {
> +		DBG("wait failed on props resp msg.\n");
> +		return -ENODEV;
> +	}
> +	dev->recvd_msg = 0;
> +	return 0;
> +}
> +
> +/**
> + * allocate_me_clients_storage - allocate storage for me clients
> + *
> + * @dev: Device object for our driver
> + *
> + * returns 0 on success, <0 on failure.
> + */
> +static int allocate_me_clients_storage(struct iamt_heci_device *dev)
> +{
> +	struct heci_me_client *clients;
> +	struct heci_me_client *client;
> +	__u8 num, i, j;
> +	int err;
> +
> +	if (dev->num_heci_me_clients <= 0)
> +		return 0;

Can this happen?

flow_ctrl_creds() test for num_heci_me_clients == 0, whereas this code
also tests for num_heci_me_clients<0.  Which one is correct?

quoted text
> +	spin_lock_bh(&dev->device_lock);
> +	kfree(dev->me_clients);
> +	dev->me_clients = NULL;
> +	spin_unlock_bh(&dev->device_lock);
> +
> +	/* allocate storage for ME clients representation */
> +	clients = kcalloc(dev->num_heci_me_clients,
> +			sizeof(struct heci_me_client), GFP_KERNEL);
> +	if (!clients) {
> +		DBG("memory allocation for ME clients failed.\n");
> +		return -ENOMEM;
> +	}
> +
> +	spin_lock_bh(&dev->device_lock);
> +	dev->me_clients = clients;
> +	spin_unlock_bh(&dev->device_lock);

Strange locking.  Shouldn't we at least check to see whether some other
thread has just initialised dev->me_clients?

If that is not possible, then why is the locking needed at all?

quoted text
> +	num = 0;
> +	for (i = 0; i < sizeof(dev->heci_me_clients); i++) {
> +		for (j = 0; j < 8; j++) {
> +			if ((dev->heci_me_clients[i] & (1 << j)) != 0) {
> +				client = &dev->me_clients[num];
> +				client->client_id = (i * 8) + j;
> +				client->flow_ctrl_creds = 0;
> +				err = host_client_properties(dev, client);
> +				if (err != 0) {
> +					spin_lock_bh(&dev->device_lock);
> +					kfree(dev->me_clients);
> +					dev->me_clients = NULL;
> +					spin_unlock_bh(&dev->device_lock);

dittoes all over the place.

quoted text
> +					return err;
> +				}
> +				num++;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * heci_init_file_private - initializes private file structure.
> + *
> + * @priv: private file structure to be initialized
> + * @file: the file structure
> + */
> +static void heci_init_file_private(struct heci_file_private *priv,
> +				   struct file *file)
> +{
> +	memset(priv, 0, sizeof(struct heci_file_private));
> +	spin_lock_init(&priv->file_lock);
> +	spin_lock_init(&priv->read_io_lock);
> +	spin_lock_init(&priv->write_io_lock);
> +	init_waitqueue_head(&priv->wait);
> +	init_waitqueue_head(&priv->rx_wait);
> +	DBG("priv->rx_wait =%p\n", &priv->rx_wait);
> +	init_waitqueue_head(&priv->tx_wait);
> +	INIT_LIST_HEAD(&priv->link);
> +	priv->reading_state = HECI_IDLE;
> +	priv->writing_state = HECI_IDLE;
> +}
> +
> +/**
> + * heci_find_me_client - search for ME client guid
> + *                       sets client_id in heci_file_private if found
> + * @dev: Device object for our driver
> + * @priv: private file structure to set client_id in
> + * @cguid: searched guid of ME client
> + * @client_id: id of host client to be set in file private structure
> + *
> + * returns ME client index
> + */
> +static __u8 heci_find_me_client(struct iamt_heci_device *dev,
> +				struct heci_file_private *priv,
> +				const struct guid *cguid, __u8 client_id)
> +{
> +	__u8 i;
> +
> +	if ((dev == NULL) || (priv == NULL) || (cguid == NULL))
> +		return 0;

Can these all happen?

quoted text
> +	for (i = 0; i < dev->num_heci_me_clients; i++) {
> +		if (memcmp(cguid,
> +			   &dev->me_clients[i].props.protocol_name,
> +			   sizeof(struct guid)) == 0) {
> +			priv->me_client_id = dev->me_clients[i].client_id;
> +			priv->state = HECI_FILE_CONNECTING;
> +			priv->host_client_id = client_id;
> +
> +			list_add_tail(&priv->link, &dev->file_list);
> +			return i;
> +		}
> +	}
> +	return 0;
> +}
> +
> +/**
> + * heci_check_asf_mode - check for ASF client
> + *
> + * @dev: Device object for our driver
> + */
> +static void heci_check_asf_mode(struct iamt_heci_device *dev)
> +{
> +	__u8 i;
> +
> +	spin_lock_bh(&dev->device_lock);
> +	dev->asf_mode = 0;
> +	/* find ME ASF client - otherwise assume AMT mode */
> +	DBG("find ME ASF client - otherwise assume AMT mode.\n");
> +	for (i = 0; i < dev->num_heci_me_clients; i++) {
> +		if (memcmp(&heci_asf_guid,
> +				&dev->me_clients[i].props.protocol_name,
> +				sizeof(struct guid)) == 0) {
> +			dev->asf_mode = 1;
> +			spin_unlock_bh(&dev->device_lock);
> +			DBG("found ME ASF client.\n");
> +			return;
> +		}
> +	}
> +	spin_unlock_bh(&dev->device_lock);
> +	DBG("assume AMT mode.\n");
> +}
> +
> +/**
> + * heci_connect_me_client - connect ME client
> + * @dev: Device object for our driver
> + * @priv: private file structure
> + * @timeout: connect timeout in seconds
> + *
> + * returns 1 - if connected, 0 - if not
> + */
> +static __u8 heci_connect_me_client(struct iamt_heci_device *dev,
> +				   struct heci_file_private *priv,
> +				   long timeout)
> +{
> +	int err = 0;
> +
> +	if ((dev == NULL) || (priv == NULL))
> +		return 0;

That's a bit excessively parenthesised.

Can these things happen?

quoted text
> +	if (!heci_connect(dev, priv)) {
> +		DBG("failed to call heci_connect for client_id=%d.\n",
> +		    priv->host_client_id);
> +		spin_lock_bh(&dev->device_lock);
> +		heci_remove_client_from_file_list(dev, priv->host_client_id);
> +		priv->state = HECI_FILE_DISCONNECTED;
> +		spin_unlock_bh(&dev->device_lock);
> +		return 0;
> +	}
> +
> +	err = wait_event_timeout(dev->wait_recvd_msg,
> +	    (HECI_FILE_CONNECTED == priv->state ||
> +	     HECI_FILE_DISCONNECTED == priv->state),
> +	    timeout * HZ);
> +	if (HECI_FILE_CONNECTED != priv->state) {
> +		spin_lock_bh(&dev->device_lock);
> +		heci_remove_client_from_file_list(dev, priv->host_client_id);
> +		DBG("failed to connect client_id=%d state=%d.\n",
> +		    priv->host_client_id, priv->state);
> +		if (err)
> +			DBG("failed connect err=%08x\n", err);
> +		priv->state = HECI_FILE_DISCONNECTED;
> +		spin_unlock_bh(&dev->device_lock);
> +		return 0;
> +	}
> +	DBG("successfully connected client_id=%d.\n",
> +	    priv->host_client_id);
> +	return 1;
> +}
> +
> +/**
> + * host_init_wd - heci initialization wd.
> + *
> + * @dev: Device object for our driver
> + */
> +static void host_init_wd(struct iamt_heci_device *dev)
> +{
> +	spin_lock_bh(&dev->device_lock);
> +
> +	heci_init_file_private(&dev->wd_file_ext, NULL);
> +
> +	/* look for WD client and connect to it */
> +	dev->wd_file_ext.state = HECI_FILE_DISCONNECTED;
> +	dev->wd_timeout = 0;
> +
> +	if (dev->asf_mode) {
> +		memcpy(dev->wd_data, heci_stop_wd_params, HECI_WD_PARAMS_SIZE);
> +	} else {
> +		/* AMT mode */
> +		dev->wd_timeout = AMT_WD_VALUE;
> +		DBG("dev->wd_timeout=%d.\n", dev->wd_timeout);
> +		memcpy(dev->wd_data, heci_start_wd_params, HECI_WD_PARAMS_SIZE);
> +		memcpy(dev->wd_data + HECI_WD_PARAMS_SIZE,
> +		       &dev->wd_timeout, sizeof(__u16));
> +	}
> +
> +	/* find ME WD client */
> +	heci_find_me_client(dev, &dev->wd_file_ext,
> +			    &heci_wd_guid, HECI_WD_HOST_CLIENT_ID);
> +	spin_unlock_bh(&dev->device_lock);
> +
> +	DBG("check wd_file_ext\n");
> +	if (HECI_FILE_CONNECTING == dev->wd_file_ext.state) {
> +		if (heci_connect_me_client(dev, &dev->wd_file_ext, 15) == 1) {
> +			DBG("dev->wd_timeout=%d.\n", dev->wd_timeout);
> +			if (dev->wd_timeout != 0)
> +				dev->wd_due_counter = 1;
> +			else
> +				dev->wd_due_counter = 0;
> +			DBG("successfully connected to WD client.\n");
> +		}
> +	} else
> +		DBG("failed to find WD client.\n");
> +
> +
> +	spin_lock_bh(&dev->device_lock);
> +	dev->wd_timer.function = &heci_wd_timer;
> +	dev->wd_timer.data = (unsigned long) dev;
> +	spin_unlock_bh(&dev->device_lock);

Use setup_timer().

Note that setup_timer() does init_timer(), but we already have an
init_timer(dev->wd_timer) elsewhere.  Please sort that out.

The taking of device_lock here surely is unneeded.

quoted text
> +}
> +
> +
> +/**
> + * host_init_iamthif - heci initialization iamthif client.
> + *
> + * @dev: Device object for our driver
> + *
> + */
> +static void host_init_iamthif(struct iamt_heci_device *dev)
> +{
> +	__u8 i;
> +
> +	spin_lock_bh(&dev->device_lock);
> +
> +	heci_init_file_private(&dev->iamth