Re: [PATCH] Intel Management Engine Interface

From: Andrew Morton
Date: Tue Aug 12 2008 - 00:54:26 EST

Next message: Andrew Morton: "Re: [-mm][PATCH 1/2] mm owner fix race between swap and exit"
Previous message: Krzysztof Helt: "Re: [PATCH RESEND] agp: fix SIS 5591/5592 wrong PCI id"
In reply to: Marcin Obara: "Re: [PATCH] Intel Management Engine Interface"
Next in thread: Jiri Slaby: "Re: [PATCH] Intel Management Engine Interface"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

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

>
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b343814..f473f95 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2247,6 +2247,15 @@ L: e1000-devel@xxxxxxxxxxxxxxxxxxxxx
> W: http://e1000.sourceforge.net/
> S: Supported
>
> +INTEL MANAGEABILITY ENGINE INTERFACE (HECI)
> +P: Anas Nashif
> +M: anas.nashif@xxxxxxxxx
> +P: Marcin Obara
> +M: marcin.obara@xxxxxxxxx
> +W: http://www.openamt.org/
> +L: openamt-devel@xxxxxxxxxxxxxxxxxxxxx
> +S: Supported
> +
> INTEL PRO/WIRELESS 2100 NETWORK CONNECTION SUPPORT
> P: Zhu Yi
> M: yi.zhu@xxxxxxxxx
> 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?

> +#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.

> +
> +/*
> + * 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).

> +/* 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.

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

> +/*
> + * 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.

> +/* 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.

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

> + /* 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.

> + /*
> + * 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.

> + char *mem_addr;

void __iomem *?

> + /*
> + * lock for the device

comment needs full detailing, please.

> + */
> + 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.

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

What's this?

Seems to be protected by device_lock?

> + struct timer_list wd_timer;

See comments below.

> + /*
> + * 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.

> + /*
> + * 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.

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

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

"stopped"

> + __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)

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

> + */
> +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?

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

> +/**
> + * 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..

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

> +/**
> + * 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.

> + 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)"

> + */
> +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.

> + 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().

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

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

I suspect this memset wasn't needed.

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

Mysterious.

> + 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*.

> + 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).

> + 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).

> + 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?

> + 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*.

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

> +/**
> + * 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?

> + 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?

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

> + 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?

> + 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?

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

> +}
> +
> +
> +/**
> + * 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->iamthif_file_ext, NULL);
> + dev->iamthif_file_ext.state = HECI_FILE_DISCONNECTED;
> +
> + /* find ME PTHI client */
> + i = heci_find_me_client(dev, &dev->iamthif_file_ext,
> + &heci_pthi_guid, HECI_IAMTHIF_HOST_CLIENT_ID);
> + if (dev->iamthif_file_ext.state != HECI_FILE_CONNECTING) {
> + DBG("failed to find iamthif client.\n");
> + spin_unlock_bh(&dev->device_lock);
> + return;
> + }
> +
> + BUG_ON(dev->me_clients[i].props.max_msg_length != IAMTHIF_MTU);
> +
> + spin_unlock_bh(&dev->device_lock);
> + if (heci_connect_me_client(dev, &dev->iamthif_file_ext, 15) == 1) {
> + DBG("connected to iamthif client.\n");
> + dev->iamthif_state = HECI_IAMTHIF_IDLE;
> + }
> +}
> +
> +/**
> + * heci_alloc_file_private - allocates a private file structure and set it up.
> + * @file: the file structure
> + *
> + * returns The allocated file or NULL on failure
> + */
> +struct heci_file_private *heci_alloc_file_private(struct file *file)
> +{
> + struct heci_file_private *priv;
> +
> + priv = kmalloc(sizeof(struct heci_file_private), GFP_KERNEL);
> + if (!priv)
> + return NULL;
> +
> + heci_init_file_private(priv, file);
> +
> + return priv;
> +}
> +
> +
> +

stray newlines

> +/**
> + * heci_disconnect_host_client - send disconnect message to fw from host client.

stray double-space.

> + *
> + * @dev: Device object for our driver
> + * @file_ext: private data of the file object
> + *
> + * returns 0 on success, <0 on failure.
> + */
> +int heci_disconnect_host_client(struct iamt_heci_device *dev,
> + struct heci_file_private *file_ext)
> +{
> + int rets, err;
> + long timeout = 15; /* 15 seconds */
> + struct heci_cb_private *priv_cb;
> +
> + if ((!dev) || (!file_ext))
> + return -ENODEV;

Can these happen?

Excessively parenthesised.

(Please review this in the many other similar places)

> + if (file_ext->state != HECI_FILE_DISCONNECTING)
> + return 0;
> +
> + priv_cb = kzalloc(sizeof(struct heci_cb_private), GFP_KERNEL);
> + if (!priv_cb)
> + return -ENOMEM;
> +
> + INIT_LIST_HEAD(&priv_cb->cb_list);
> + priv_cb->file_private = file_ext;
> + priv_cb->major_file_operations = HECI_CLOSE;
> + spin_lock_bh(&dev->device_lock);
> + if (dev->host_buffer_is_empty) {
> + dev->host_buffer_is_empty = 0;
> + if (heci_disconnect(dev, file_ext)) {
> + list_add_tail(&priv_cb->cb_list,
> + &dev->ctrl_rd_list.heci_cb.cb_list);
> + } else {
> + spin_unlock_bh(&dev->device_lock);
> + rets = -ENODEV;
> + DBG("failed to call heci_disconnect.\n");
> + goto free;

This will return with dev->host_buffer_is_empty = 0. Is that correct?

> + }
> + } else {
> + DBG("add disconnect cb to control write list\n");
> + list_add_tail(&priv_cb->cb_list,
> + &dev->ctrl_wr_list.heci_cb.cb_list);
> + }
> + spin_unlock_bh(&dev->device_lock);
> +
> + err = wait_event_timeout(dev->wait_recvd_msg,
> + (HECI_FILE_DISCONNECTED == file_ext->state),
> + timeout * HZ);
> + if (HECI_FILE_DISCONNECTED == file_ext->state) {
> + rets = 0;
> + DBG("successfully disconnected from fw client.\n");
> + } else {
> + rets = -ENODEV;
> + if (HECI_FILE_DISCONNECTED != file_ext->state)
> + DBG("wrong status client disconnect.\n");
> +
> + if (err)
> + DBG("wait failed disconnect err=%08x\n", err);
> +
> + DBG("failed to disconnect from fw client.\n");
> + }
> +
> + spin_lock_bh(&dev->device_lock);
> + heci_flush_list(&dev->ctrl_rd_list, file_ext);
> + heci_flush_list(&dev->ctrl_wr_list, file_ext);
> + spin_unlock_bh(&dev->device_lock);
> +free:
> + heci_free_cb_private(priv_cb);
> + return rets;
> +}
> +
> +/**
> + * heci_remove_client_from_file_list -
> + * remove file private data from device file list
> + *
> + * @dev: Device object for our driver
> + * @host_client_id: host client id to be removed
> + */
> +void heci_remove_client_from_file_list(struct iamt_heci_device *dev,
> + __u8 host_client_id)
> +{
> + struct heci_file_private *file_pos = NULL;
> + struct heci_file_private *file_next = NULL;
> + list_for_each_entry_safe(file_pos, file_next, &dev->file_list, link) {
> + if (host_client_id == file_pos->host_client_id) {
> + DBG("remove host client = %d, ME client = %d\n",
> + file_pos->host_client_id,
> + file_pos->me_client_id);
> + list_del_init(&file_pos->link);
> + break;
> + }
> + }
> +}
> diff --git a/drivers/char/heci/heci_interface.c b/drivers/char/heci/heci_interface.c
> new file mode 100644
> index 0000000..c5f51a7
> --- /dev/null
> +++ b/drivers/char/heci/heci_interface.c
> @@ -0,0 +1,485 @@
> +/*
> + * 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 "heci.h"
> +#include "heci_interface.h"
> +
> +
> +/**
> + * heci_set_csr_register - write H_CSR register to the heci device
> + *
> + * @dev: device object for our driver
> + */
> +void heci_set_csr_register(struct iamt_heci_device *dev)
> +{
> + write_heci_register(dev, H_CSR, dev->host_hw_state);
> + dev->host_hw_state = read_heci_register(dev, H_CSR);
> +}
> +
> +/**
> + * heci_csr_enable_interrupts - enable heci device interrupts
> + *
> + * @dev: device object for our driver
> + */
> +void heci_csr_enable_interrupts(struct iamt_heci_device *dev)
> +{
> + dev->host_hw_state |= H_IE;
> + heci_set_csr_register(dev);
> +}
> +
> +/**
> + * heci_csr_disable_interrupts - disable heci device interrupts
> + *
> + * @dev: device object for our driver
> + */
> +void heci_csr_disable_interrupts(struct iamt_heci_device *dev)
> +{
> + dev->host_hw_state &= ~H_IE;
> + heci_set_csr_register(dev);
> +}
> +
> +
> +/**
> + * _host_get_filled_slots - get number of device filled buffer slots
> + *
> + * @device: the device structure
> + *
> + * returns numer of filled slots
> + */
> +static unsigned char _host_get_filled_slots(const struct iamt_heci_device *dev)
> +{
> + char read_ptr, write_ptr;
> +
> + read_ptr = (char) ((dev->host_hw_state & H_CBRP) >> 8);
> + write_ptr = (char) ((dev->host_hw_state & H_CBWP) >> 16);
> +
> + return (unsigned char) (write_ptr - read_ptr);
> +}

hm, so this is implementing a ring buffer and the code itself
implicitly "knows" that the H_CBRP and H_CBWP masks span 0-255, so it
can use C char arithmetic.

That's a bit funky.

> +/**
> + * host_buffer_is_empty - check if host buffer is empty.
> + *
> + * @dev: device object for our driver
> + *
> + * returns 1 if empty, 0 - otherwise.
> + */
> +int host_buffer_is_empty(struct iamt_heci_device *dev)
> +{
> + unsigned char filled_slots;
> +
> + dev->host_hw_state = read_heci_register(dev, H_CSR);
> + filled_slots = _host_get_filled_slots(dev);
> +
> + if (filled_slots > 0)
> + return 0;
> +
> + return 1;
> +}

Seems strange that we have a device field called
"host_buffer_is_empty", and a helper function of the same name which
appears to have no relationship with that field.

host_buffer_is_empty() is a quite poorly-chosen name for a kenrel-wide
symbol.

> +/**
> + * count_empty_write_slots - count write empty slots.
> + *
> + * @dev: device object for our driver
> + *
> + * returns -1(ESLOTS_OVERFLOW) if overflow, otherwise empty slots count
> + */
> +__s32 count_empty_write_slots(const struct iamt_heci_device *dev)
> +{
> + unsigned char buffer_depth, filled_slots, empty_slots;
> +
> + buffer_depth = (unsigned char) ((dev->host_hw_state & H_CBD) >> 24);

More implict "we know that is an eight-bit-field" code.

> + filled_slots = _host_get_filled_slots(dev);
> + empty_slots = buffer_depth - filled_slots;
> +
> + if (filled_slots > buffer_depth) {
> + /* overflow */
> + return -ESLOTS_OVERFLOW;
> + }
> +
> + return (__s32) empty_slots;
> +}

Also an inappropriate name for a kernel-wide symbol.

Please review all the global symbols, ensure that they have a suitable
prefix (heci_ would be OK). Or make them static, if poss.

> +/**
> + * heci_write_message - write a message to heci device.
> + *
> + * @dev: device object for our driver
> + * @heci_hdr: header of message
> + * @write_buffer: message buffer will be write
> + * @write_length: message size will be write
> + *
> + * returns 1 if success, 0 - otherwise.
> + */
> +int heci_write_message(struct iamt_heci_device *dev,
> + struct heci_msg_hdr *header,
> + unsigned char *write_buffer,
> + unsigned long write_length)
> +{
> + __u32 temp_msg = 0;
> + unsigned long bytes_written = 0;
> + unsigned char buffer_depth, filled_slots, empty_slots;
> + unsigned long dw_to_write;
> +
> + dev->host_hw_state = read_heci_register(dev, H_CSR);
> + DBG("host_hw_state = 0x%08x.\n", dev->host_hw_state);
> + DBG("heci_write_message header=%08x.\n", *((__u32 *) header));
> + buffer_depth = (unsigned char) ((dev->host_hw_state & H_CBD) >> 24);
> + filled_slots = _host_get_filled_slots(dev);
> + empty_slots = buffer_depth - filled_slots;
> + DBG("filled = %hu, empty = %hu.\n", filled_slots, empty_slots);
> +
> + dw_to_write = ((write_length + 3) / 4);

Please use the helpers, if they exist. I expect that DIV_ROUND_UP() or
roundup() could be used here. (Multiple instances)

> + if (dw_to_write > empty_slots)
> + return 0;
> +
> + write_heci_register(dev, H_CB_WW, *((__u32 *) header));
> +
> + while (write_length >= 4) {
> + write_heci_register(dev, H_CB_WW,
> + *(__u32 *) (write_buffer + bytes_written));
> + bytes_written += 4;
> + write_length -= 4;
> + }
> +
> + if (write_length > 0) {
> + memcpy(&temp_msg, &write_buffer[bytes_written], write_length);
> + write_heci_register(dev, H_CB_WW, temp_msg);
> + }
> +
> + dev->host_hw_state |= H_IG;
> + write_heci_register(dev, H_CSR, dev->host_hw_state);
> + dev->me_hw_state = read_heci_register(dev, ME_CSR_HA);
> + if ((dev->me_hw_state & ME_RDY_HRA) != ME_RDY_HRA)
> + return 0;
> +
> + dev->write_hang = 0;
> + return 1;
> +}
> +
> +/**
> + * count_full_read_slots - count read full slots.
> + *
> + * @dev: device object for our driver
> + *
> + * returns -1(ESLOTS_OVERFLOW) if overflow, otherwise filled slots count
> + */
> +__s32 count_full_read_slots(struct iamt_heci_device *dev)

namespace...

> +{
> + char read_ptr, write_ptr;
> + unsigned char buffer_depth, filled_slots;
> +
> + dev->me_hw_state = read_heci_register(dev, ME_CSR_HA);
> + buffer_depth = (unsigned char)((dev->me_hw_state & ME_CBD_HRA) >> 24);
> + read_ptr = (char) ((dev->me_hw_state & ME_CBRP_HRA) >> 8);
> + write_ptr = (char) ((dev->me_hw_state & ME_CBWP_HRA) >> 16);
> + filled_slots = (unsigned char) (write_ptr - read_ptr);
> +
> + if (filled_slots > buffer_depth) {
> + /* overflow */
> + return -ESLOTS_OVERFLOW;
> + }
> +
> + DBG("filled_slots =%08x \n", filled_slots);
> + return (__s32) filled_slots;
> +}
> +
> +/**
> + * heci_read_slots - read a message from heci device.
> + *
> + * @dev: device object for our driver
> + * @buffer: message buffer will be write
> + * @buffer_length: message size will be read
> + */
> +void heci_read_slots(struct iamt_heci_device *dev,
> + unsigned char *buffer, unsigned long buffer_length)
> +{
> + __u32 i = 0;
> + unsigned char temp_buf[sizeof(__u32)];
> +
> + while (buffer_length >= sizeof(__u32)) {
> + ((__u32 *) buffer)[i] = read_heci_register(dev, ME_CB_RW);
> + DBG("buffer[%d]= %d\n", i, ((__u32 *) buffer)[i]);
> + i++;
> + buffer_length -= sizeof(__u32);
> + }
> +
> + if (buffer_length > 0) {
> + *((__u32 *) &temp_buf) = read_heci_register(dev, ME_CB_RW);
> + memcpy(&buffer[i * 4], temp_buf, buffer_length);
> + }
> +
> + dev->host_hw_state |= H_IG;
> + heci_set_csr_register(dev);
> +}
> +
> +/**
> + * flow_ctrl_creds - check flow_control credentials.
> + *
> + * @dev: device object for our driver
> + * @file_ext: private data of the file object
> + *
> + * returns 1 if flow_ctrl_creds >0, 0 - otherwise.
> + */
> +int flow_ctrl_creds(struct iamt_heci_device *dev,

namespace

> + struct heci_file_private *file_ext)
> +{
> + __u8 i;
> +
> + if (!dev->num_heci_me_clients)
> + return 0;
> +
> + if (file_ext == NULL)
> + return 0;
> +
> + if (file_ext->flow_ctrl_creds > 0)
> + return 1;
> +
> + for (i = 0; i < dev->num_heci_me_clients; i++) {
> + if (dev->me_clients[i].client_id == file_ext->me_client_id) {
> + if (dev->me_clients[i].flow_ctrl_creds > 0) {
> + BUG_ON(dev->me_clients[i].props.single_recv_buf
> + == 0);
> + return 1;
> + }
> + return 0;
> + }
> + }
> + BUG();
> + return 0;
> +}
> +
> +/**
> + * flow_ctrl_reduce - reduce flow_control.
> + *
> + * @dev: device object for our driver
> + * @file_ext: private data of the file object
> + */
> +void flow_ctrl_reduce(struct iamt_heci_device *dev,

namespace

> + struct heci_file_private *file_ext)
> +{
> + __u8 i;
> +
> + if (!dev->num_heci_me_clients)
> + return;
> +
> + for (i = 0; i < dev->num_heci_me_clients; i++) {
> + if (dev->me_clients[i].client_id == file_ext->me_client_id) {
> + if (dev->me_clients[i].props.single_recv_buf != 0) {
> + BUG_ON(dev->me_clients[i].flow_ctrl_creds <= 0);
> + dev->me_clients[i].flow_ctrl_creds--;
> + } else {
> + BUG_ON(file_ext->flow_ctrl_creds <= 0);
> + file_ext->flow_ctrl_creds--;
> + }
> + return;
> + }
> + }
> + BUG();
> +}
> +
> +/**
> + * heci_send_flow_control - send flow control to fw.
> + *
> + * @dev: device object for our driver
> + * @file_ext: private data of the file object
> + *
> + * returns 1 if success, 0 - otherwise.
> + */
> +int heci_send_flow_control(struct iamt_heci_device *dev,
> + struct heci_file_private *file_ext)
> +{
> + struct heci_msg_hdr *heci_hdr;
> + struct hbm_flow_control *heci_flow_control;
> +
> + 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_flow_control);
> + heci_hdr->msg_complete = 1;
> + heci_hdr->reserved = 0;
> +
> + heci_flow_control = (struct hbm_flow_control *) &dev->wr_msg_buf[1];
> + memset(heci_flow_control, 0, sizeof(heci_flow_control));
> + heci_flow_control->host_addr = file_ext->host_client_id;
> + heci_flow_control->me_addr = file_ext->me_client_id;
> + heci_flow_control->cmd.cmd = HECI_FLOW_CONTROL_CMD;
> + memset(heci_flow_control->reserved, 0,
> + sizeof(heci_flow_control->reserved));

This code seems pretty repetitive.

I expect it would be hard to factor out the common bits cleanly, dunno.

> + DBG("sending flow control host client = %d, me client = %d\n",
> + file_ext->host_client_id, file_ext->me_client_id);
> + if (!heci_write_message(dev, heci_hdr,
> + (unsigned char *) heci_flow_control,
> + sizeof(struct hbm_flow_control)))
> + return 0;
> +
> + return 1;
> +
> +}
> +
> +/**
> + * other_client_is_connecting - check if other
> + * client with the same client id is connected.
> + *
> + * @dev: device object for our driver
> + * @file_ext: private data of the file object
> + *
> + * returns 1 if other client is connected, 0 - otherwise.
> + */
> +int other_client_is_connecting(struct iamt_heci_device *dev,

namespace

> + struct heci_file_private *file_ext)
> +{
> + struct heci_file_private *file_pos = NULL;
> + struct heci_file_private *file_next = NULL;
> +
> + list_for_each_entry_safe(file_pos, file_next, &dev->file_list, link) {
> + if ((file_pos->state == HECI_FILE_CONNECTING)
> + && (file_pos != file_ext)
> + && file_ext->me_client_id == file_pos->me_client_id)
> + return 1;
> +
> + }
> + return 0;
> +}
> +
> +/**
> + * heci_send_wd - send watch dog message to fw.
> + *
> + * @dev: device object for our driver
> + *
> + * returns 1 if success, 0 - otherwise.

A common kernel convention is "returns 0 on success, else a -ve errno".

Please use that convention unless there are good reasons why it is not
practical to do so.

> + */
> +int heci_send_wd(struct iamt_heci_device *dev)
> +{
> + struct heci_msg_hdr *heci_hdr;
> +
> + heci_hdr = (struct heci_msg_hdr *) &dev->wr_msg_buf[0];
> + heci_hdr->host_addr = dev->wd_file_ext.host_client_id;
> + heci_hdr->me_addr = dev->wd_file_ext.me_client_id;
> + heci_hdr->msg_complete = 1;
> + heci_hdr->reserved = 0;
> +
> + if (!memcmp(dev->wd_data, heci_start_wd_params,
> + HECI_WD_PARAMS_SIZE)) {
> + heci_hdr->length = HECI_START_WD_DATA_SIZE;
> + } else {
> + BUG_ON(memcmp(dev->wd_data, heci_stop_wd_params,
> + HECI_WD_PARAMS_SIZE));
> + heci_hdr->length = HECI_WD_PARAMS_SIZE;
> + }
> +
> + if (!heci_write_message(dev, heci_hdr, dev->wd_data, heci_hdr->length))
> + return 0;
> +
> + return 1;
> +}
> +
> +/**
> + * heci_disconnect - send disconnect message to fw.
> + *
> + * @dev: device object for our driver
> + * @file_ext: private data of the file object
> + *
> + * returns 1 if success, 0 - otherwise.
> + */
> +int heci_disconnect(struct iamt_heci_device *dev,
> + struct heci_file_private *file_ext)
> +{
> + struct heci_msg_hdr *heci_hdr;
> + struct hbm_client_disconnect_request *heci_cli_disconnect;
> +
> + 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_client_disconnect_request);
> + heci_hdr->msg_complete = 1;
> + heci_hdr->reserved = 0;
> +
> + heci_cli_disconnect =
> + (struct hbm_client_disconnect_request *) &dev->wr_msg_buf[1];
> + memset(heci_cli_disconnect, 0, sizeof(heci_cli_disconnect));
> + heci_cli_disconnect->host_addr = file_ext->host_client_id;
> + heci_cli_disconnect->me_addr = file_ext->me_client_id;
> + heci_cli_disconnect->cmd.cmd = CLIENT_DISCONNECT_REQ_CMD;
> + heci_cli_disconnect->reserved[0] = 0;
> +
> + if (!heci_write_message(dev, heci_hdr,
> + (unsigned char *) heci_cli_disconnect,
> + sizeof(struct hbm_client_disconnect_request)))
> + return 0;
> +
> + return 1;
> +}
> +
> +/**
> + * heci_connect - send connect message to fw.
> + *
> + * @dev: device object for our driver
> + * @file_ext: private data of the file object
> + *
> + * returns 1 if success, 0 - otherwise.
> + */
> +int heci_connect(struct iamt_heci_device *dev,
> + struct heci_file_private *file_ext)
> +{
> + struct heci_msg_hdr *heci_hdr;
> + struct hbm_client_connect_request *heci_cli_connect;
> +
> + 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_client_connect_request);
> + heci_hdr->msg_complete = 1;
> + heci_hdr->reserved = 0;
> +
> + heci_cli_connect =
> + (struct hbm_client_connect_request *) &dev->wr_msg_buf[1];
> + heci_cli_connect->host_addr = file_ext->host_client_id;
> + heci_cli_connect->me_addr = file_ext->me_client_id;
> + heci_cli_connect->cmd.cmd = CLIENT_CONNECT_REQ_CMD;
> + heci_cli_connect->reserved = 0;
> +
> + if (!heci_write_message(dev, heci_hdr,
> + (unsigned char *) heci_cli_connect,
> + sizeof(struct hbm_client_connect_request)))
> + return 0;
> +
> + return 1;
> +}
> diff --git a/drivers/char/heci/heci_interface.h b/drivers/char/heci/heci_interface.h
> new file mode 100644
> index 0000000..37336eb
> --- /dev/null
> +++ b/drivers/char/heci/heci_interface.h
> @@ -0,0 +1,170 @@
> +/*
> + * 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_INTERFACE_H_
> +#define _HECI_INTERFACE_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"
> +
> +
> +#define HBM_MINOR_VERSION 0
> +#define HBM_MAJOR_VERSION 1
> +#define HBM_TIMEOUT 1 /* 1 second */
> +
> +
> +#define HOST_START_REQ_CMD 0x01
> +#define HOST_START_RES_CMD 0x81
> +
> +#define HOST_STOP_REQ_CMD 0x02
> +#define HOST_STOP_RES_CMD 0x82
> +
> +#define ME_STOP_REQ_CMD 0x03
> +
> +#define HOST_ENUM_REQ_CMD 0x04
> +#define HOST_ENUM_RES_CMD 0x84
> +
> +#define HOST_CLIENT_PROPERTEIS_REQ_CMD 0x05
> +#define HOST_CLIENT_PROPERTEIS_RES_CMD 0x85
> +
> +#define CLIENT_CONNECT_REQ_CMD 0x06
> +#define CLIENT_CONNECT_RES_CMD 0x86
> +
> +#define CLIENT_DISCONNECT_REQ_CMD 0x07
> +#define CLIENT_DISCONNECT_RES_CMD 0x87
> +
> +#define HECI_FLOW_CONTROL_CMD 0x08

Some comments describing all the above would be useful. The emphasis
should, as always, be upon by "why they exist" rather than upon "what
they do".

> +
> +#define AMT_WD_VALUE 120 /* seconds */
> +
> +#define HECI_WATCHDOG_DATA_SIZE 16
> +#define HECI_START_WD_DATA_SIZE 20
> +#define HECI_WD_PARAMS_SIZE 4
> +
> +/* IOCTL commands */
> +#define IOCTL_HECI_GET_VERSION \
> + _IOWR('H' , 0x0, struct heci_message_data)
> +#define IOCTL_HECI_CONNECT_CLIENT \
> + _IOWR('H' , 0x01, struct heci_message_data)
> +#define IOCTL_HECI_WD \
> + _IOWR('H' , 0x02, struct heci_message_data)
> +#define IOCTL_HECI_BYPASS_WD \
> + _IOWR('H' , 0x10, struct heci_message_data)

So this driver implements ioctls!

That is a core, top-level, userspace-visible design decision! It
should have been given prominent billing in the changelog description.

These four values are supposed to be exported to userspace headers,
yes? But they're buried in a kernel-internal header and don't have the
appropriate __KERNEL__ wrappers.

> +enum heci_stop_reason_types{
> + DRIVER_STOP_REQUEST = 0x00,
> + DEVICE_D1_ENTRY = 0x01,
> + DEVICE_D2_ENTRY = 0x02,
> + DEVICE_D3_ENTRY = 0x03,
> + SYSTEM_S1_ENTRY = 0x04,
> + SYSTEM_S2_ENTRY = 0x05,
> + SYSTEM_S3_ENTRY = 0x06,
> + SYSTEM_S4_ENTRY = 0x07,
> + SYSTEM_S5_ENTRY = 0x08
> +};
> +
> +enum me_stop_reason_types{
> + FW_UPDATE = 0x00
> +};
> +
> +enum client_connect_status_types{
> + CCS_SUCCESS = 0x00,
> + CCS_NOT_FOUND = 0x01,
> + CCS_ALREADY_STARTED = 0x02,
> + CCS_OUT_OF_RESOURCES = 0x03,
> + CCS_MESSAGE_SMALL = 0x04
> +};
> +
> +enum client_disconnect_status_types{
> + CDS_SUCCESS = 0x00
> +};
> +
> +
> +/*
> + * heci interface function prototypes
> + */
> +void heci_set_csr_register(struct iamt_heci_device *dev);
> +void heci_csr_enable_interrupts(struct iamt_heci_device *dev);
> +void heci_csr_disable_interrupts(struct iamt_heci_device *dev);
> +
> +void heci_read_slots(struct iamt_heci_device *dev,
> + unsigned char *buffer, unsigned long buffer_length);
> +
> +int heci_write_message(struct iamt_heci_device *dev,
> + struct heci_msg_hdr *header,
> + unsigned char *write_buffer,
> + unsigned long write_length);
> +
> +int host_buffer_is_empty(struct iamt_heci_device *dev);
> +
> +__s32 count_full_read_slots(struct iamt_heci_device *dev);
> +
> +__s32 count_empty_write_slots(const struct iamt_heci_device *dev);
> +
> +int flow_ctrl_creds(struct iamt_heci_device *dev,
> + struct heci_file_private *file_ext);
> +
> +int heci_send_wd(struct iamt_heci_device *dev);
> +
> +void flow_ctrl_reduce(struct iamt_heci_device *dev,
> + struct heci_file_private *file_ext);
> +
> +int heci_send_flow_control(struct iamt_heci_device *dev,
> + struct heci_file_private *file_ext);
> +
> +int heci_disconnect(struct iamt_heci_device *dev,
> + struct heci_file_private *file_ext);
> +int other_client_is_connecting(struct iamt_heci_device *dev,
> + struct heci_file_private *file_ext);
> +int heci_connect(struct iamt_heci_device *dev,
> + struct heci_file_private *file_ext);
> +
> +#endif /* _HECI_INTERFACE_H_ */
> diff --git a/drivers/char/heci/heci_main.c b/drivers/char/heci/heci_main.c
> new file mode 100644
> index 0000000..c587c94
> --- /dev/null
> +++ b/drivers/char/heci/heci_main.c
> @@ -0,0 +1,1563 @@
> +/*
> + * 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/moduleparam.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/fs.h>
> +#include <linux/errno.h>
> +#include <linux/types.h>
> +#include <linux/fcntl.h>
> +#include <linux/aio.h>
> +#include <linux/pci.h>
> +#include <linux/reboot.h>
> +#include <linux/poll.h>
> +#include <linux/init.h>
> +#include <linux/kdev_t.h>
> +#include <linux/ioctl.h>
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/unistd.h>
> +#include <linux/kthread.h>
> +
> +#include "heci.h"
> +#include "heci_interface.h"
> +#include "heci_version.h"
> +
> +
> +#define HECI_READ_TIMEOUT 45

I was going to ask "what units", but this is unused.

> +#define HECI_DRIVER_NAME "heci"
> +
> +/*
> + * heci driver strings
> + */
> +char heci_driver_name[] = HECI_DRIVER_NAME;
> +char heci_driver_string[] = "Intel(R) Management Engine Interface";
> +char heci_driver_version[] = HECI_DRIVER_VERSION;
> +char heci_copyright[] = "Copyright (c) 2003 - 2008 Intel Corporation.";
> +
> +
> +#ifdef HECI_DEBUG
> +int heci_debug = 1;
> +#else
> +int heci_debug;
> +#endif
> +MODULE_PARM_DESC(heci_debug, "Debug enabled or not");
> +module_param(heci_debug, int, 0644);
> +
> +
> +#define HECI_DEV_NAME "heci"
> +
> +/* heci char device for registration */
> +static struct cdev heci_cdev;
> +
> +/* major number for device */
> +static int heci_major;
> +/* The device pointer */
> +static struct pci_dev *heci_device;
> +
> +struct class *heci_class;
> +struct device *heci_dev;
> +
> +
> +/* heci_pci_tbl - PCI Device ID Table */
> +static struct pci_device_id heci_pci_tbl[] = {
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_82946GZ)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_82G35)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_82Q965)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_82G965)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_82GM965)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_82GME965)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH9_82Q35)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH9_82G33)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH9_82Q33)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH9_82X38)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH9_3200)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH9_6)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH9_7)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH9_8)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH9_9)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH9_10)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH9M_1)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH9M_2)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH9M_3)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH9M_4)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH10_1)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH10_2)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH10_3)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, HECI_DEV_ID_ICH10_4)},
> + /* required last entry */
> + {0, }
> +};
> +
> +MODULE_DEVICE_TABLE(pci, heci_pci_tbl);
> +
> +/*
> + * Local Function Prototypes
> + */
> +static int __init heci_init_module(void);
> +static void __exit heci_exit_module(void);
> +static int __devinit heci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *ent);
> +static void __devexit heci_remove(struct pci_dev *pdev);
> +static int heci_open(struct inode *inode, struct file *file);
> +static int heci_release(struct inode *inode, struct file *file);
> +static ssize_t heci_read(struct file *file, char __user *ubuf,
> + size_t length, loff_t *offset);
> +static int heci_ioctl(struct inode *inode, struct file *file,
> + unsigned int cmd, unsigned long data);
> +static ssize_t heci_write(struct file *file, const char __user *ubuf,
> + size_t length, loff_t *offset);
> +static unsigned int heci_poll(struct file *file, poll_table *wait);
> +static struct heci_cb_private *find_read_list_entry(
> + struct iamt_heci_device *dev,
> + struct heci_file_private *file_ext);
> +#ifdef CONFIG_PM
> +static int heci_suspend(struct pci_dev *pdev, pm_message_t state);
> +static int heci_resume(struct pci_dev *pdev);
> +static __u16 g_sus_wd_timeout;
> +#else
> +#define heci_suspend NULL
> +#define heci_resume NULL
> +#endif
> +/*
> + * PCI driver structure
> + */
> +static struct pci_driver heci_driver = {
> + .name = heci_driver_name,
> + .id_table = heci_pci_tbl,
> + .probe = heci_probe,
> + .remove = __devexit_p(heci_remove),
> + .shutdown = __devexit_p(heci_remove),
> + .suspend = heci_suspend,
> + .resume = heci_resume
> +};
> +
> +/*
> + * file operations structure will be use heci char device.
> + */
> +static struct file_operations heci_fops = {
> + .owner = THIS_MODULE,
> + .read = heci_read,
> + .ioctl = heci_ioctl,
> + .open = heci_open,
> + .release = heci_release,
> + .write = heci_write,
> + .poll = heci_poll,
> +};
> +
> +/**
> + * heci_registration_cdev - set up the cdev structure for heci device.
> + *
> + * @dev: char device struct
> + * @hminor: minor number for registration char device
> + * @fops: file operations structure
> + *
> + * returns 0 on success, <0 on failure.
> + */
> +static int heci_registration_cdev(struct cdev *dev, int hminor,
> + struct file_operations *fops)
> +{
> + int ret, devno = MKDEV(heci_major, hminor);
> +
> + cdev_init(dev, fops);
> + dev->owner = THIS_MODULE;
> + ret = cdev_add(dev, devno, 1);
> + /* Fail gracefully if need be */
> + if (ret) {
> + printk(KERN_ERR "heci: Error %d registering heci device %d\n",
> + ret, hminor);
> + }
> + return ret;
> +}
> +
> +/* Display the version of heci driver. */
> +static ssize_t version_show(struct class *dev, char *buf)
> +{
> + return sprintf(buf, "%s %s.\n",
> + heci_driver_string, heci_driver_version);
> +}
> +
> +static CLASS_ATTR(version, S_IRUGO, version_show, NULL);

More undocumented, unchangelogged kernel->userspace interface.

Please at least describe these proposed major kernel interface additions
in the changelog.

There's also Documentation/ABI...

> +/**
> + * heci_register_cdev - registers heci char device
> + *
> + * returns 0 on success, <0 on failure.
> + */
> +static int heci_register_cdev(void)
> +{
> + int ret;
> + dev_t dev;
> +
> + /* registration of char devices */
> + ret = alloc_chrdev_region(&dev, HECI_MINORS_BASE, HECI_MINORS_COUNT,
> + HECI_DRIVER_NAME);
> + if (ret) {
> + printk(KERN_ERR "heci: Error allocating char device region.\n");
> + return ret;
> + }
> +
> + heci_major = MAJOR(dev);
> +
> + ret = heci_registration_cdev(&heci_cdev, HECI_MINOR_NUMBER,
> + &heci_fops);
> + if (ret)
> + unregister_chrdev_region(MKDEV(heci_major, HECI_MINORS_BASE),
> + HECI_MINORS_COUNT);
> +
> + return ret;
> +}
> +
> +/**
> + * heci_unregister_cdev - unregisters heci char device
> + */
> +static void heci_unregister_cdev(void)
> +{
> + cdev_del(&heci_cdev);
> + unregister_chrdev_region(MKDEV(heci_major, HECI_MINORS_BASE),
> + HECI_MINORS_COUNT);
> +}
> +
> +/**
> + * heci_sysfs_device_create - adds device entry to sysfs
> + *
> + * returns 0 on success, <0 on failure.
> + */
> +static int heci_sysfs_device_create(void)
> +{
> + struct class *class;
> + struct device *tmphdev;
> + int err = 0;
> +
> + class = class_create(THIS_MODULE, HECI_DRIVER_NAME);
> + if (IS_ERR(class)) {
> + err = PTR_ERR(class);
> + printk(KERN_ERR "heci: Error creating heci class.\n");
> + goto err_out;
> + }
> +
> + err = class_create_file(class, &class_attr_version);
> + if (err) {
> + class_destroy(class);
> + printk(KERN_ERR "heci: Error creating heci class file.\n");
> + goto err_out;
> + }
> +
> + tmphdev = device_create(class, NULL, heci_cdev.dev, NULL,
> + HECI_DEV_NAME);
> + if (IS_ERR(tmphdev)) {
> + err = PTR_ERR(tmphdev);
> + class_remove_file(class, &class_attr_version);
> + class_destroy(class);
> + goto err_out;
> + }
> +
> + heci_dev = tmphdev;
> + heci_class = class;
> +err_out:
> + return err;
> +}

So it has a sysfs interface as well as a /dev node interface.

I shouldn't have had to read this far to find that out.

> +/**
> + * heci_sysfs_device_remove - unregisters the device entry on sysfs
> + */
> +static void heci_sysfs_device_remove(void)
> +{
> + if ((heci_class == NULL) || (IS_ERR(heci_class)))
> + return;

Can these happen?

> + heci_dev = NULL;
> + device_destroy(heci_class, heci_cdev.dev);
> + class_remove_file(heci_class, &class_attr_version);
> + class_destroy(heci_class);

I see no kthread_stop() here. Is it needed?

> +}
> +
> +/**
> + * heci_init_module - Driver Registration Routine
> + *
> + * heci_init_module is the first routine called when the driver is
> + * loaded. All it does is register with the PCI subsystem.
> + *
> + * returns 0 on success, <0 on failure.
> + */
> +static int __init heci_init_module(void)
> +{
> + int ret = 0;
> +
> + printk(KERN_INFO "heci: %s - version %s\n", heci_driver_string,
> + heci_driver_version);
> + printk(KERN_INFO "heci: %s\n", heci_copyright);
> +
> + /* init pci module */
> + ret = pci_register_driver(&heci_driver);

Should this driver have depended upon CONFIG_PCI?

> + if (ret < 0) {
> + printk(KERN_ERR "heci: Error registering driver.\n");
> + goto end;
> + }
> +
> + ret = heci_register_cdev();
> + if (ret)
> + goto unregister_pci;
> +
> + ret = heci_sysfs_device_create();
> + if (ret)
> + goto unregister_cdev;
> +
> + return ret;
> +
> +unregister_cdev:
> + heci_unregister_cdev();
> +unregister_pci:
> + pci_unregister_driver(&heci_driver);
> +end:
> + return ret;
> +}
> +
> +module_init(heci_init_module);
> +
> +
> +/**
> + * heci_exit_module - Driver Exit Cleanup Routine
> + *
> + * heci_exit_module is called just before the driver is removed
> + * from memory.
> + */
> +static void __exit heci_exit_module(void)
> +{
> + pci_unregister_driver(&heci_driver);
> + heci_sysfs_device_remove();
> + heci_unregister_cdev();
> +}
> +
> +module_exit(heci_exit_module);
> +
> +
> +/**
> + * heci_probe - Device Initialization Routine
> + *
> + * @pdev: PCI device information struct
> + * @ent: entry in kcs_pci_tbl
> + *
> + * returns 0 on success, <0 on failure.
> + */
> +static int __devinit heci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *ent)
> +{
> + struct iamt_heci_device *dev = NULL;
> + int i, err = 0;
> +
> + if (heci_device) {
> + err = -EEXIST;
> + goto end;

Now surely that can't happen.

> + }
> + /* enable pci dev */
> + err = pci_enable_device(pdev);
> + if (err) {
> + printk(KERN_ERR "heci: Failed to enable pci device.\n");
> + goto end;
> + }
> + /* set PCI host mastering */
> + pci_set_master(pdev);
> + /* pci request regions for heci driver */
> + err = pci_request_regions(pdev, heci_driver_name);
> + if (err) {
> + printk(KERN_ERR "heci: Failed to get pci regions.\n");
> + goto disable_device;
> + }
> + /* allocates and initializes the heci dev structure */
> + dev = init_heci_device(pdev);
> + if (!dev) {
> + err = -ENOMEM;
> + goto release_regions;
> + }
> + /* mapping IO device memory */
> + for (i = 0; i <= 5; i++) {
> + if (pci_resource_len(pdev, i) == 0)
> + continue;
> + if (pci_resource_flags(pdev, i) & IORESOURCE_IO) {
> + printk(KERN_ERR "heci: heci has IO ports.\n");
> + goto free_device;
> + } else if (pci_resource_flags(pdev, i) & IORESOURCE_MEM) {
> + if (dev->mem_base) {
> + printk(KERN_ERR
> + "heci: Too many mem addresses.\n");
> + goto free_device;
> + }
> + dev->mem_base = pci_resource_start(pdev, i);
> + dev->mem_length = pci_resource_len(pdev, i);
> + }
> + }
> + if (!dev->mem_base) {
> + printk(KERN_ERR "heci: No address to use.\n");
> + err = -ENODEV;
> + goto free_device;
> + }
> + dev->mem_addr = ioremap_nocache(dev->mem_base,
> + dev->mem_length);

argh, mem_addr definitely has a quite wrong type.

Please run sparse across the driver.

> + if (!dev->mem_addr) {
> + printk(KERN_ERR "heci: Remap IO device memory failure.\n");
> + err = -ENOMEM;
> + goto free_device;
> + }
> + /* request and enable interrupt */
> + err = request_irq(pdev->irq, heci_isr_interrupt, IRQF_SHARED,
> + heci_driver_name, dev);

Doing request_irq() after pci_enable_device() is dengerous. If the
device happened to be requesting an interrupt when we did
pci_enable_device(), things will generally go pear-shaped.

> + if (err) {
> + printk(KERN_ERR "heci: Request_irq failure. irq = %d \n",
> + pdev->irq);
> + goto unmap_memory;
> + }
> +
> + if (heci_hw_init(dev)) {
> + printk(KERN_ERR "heci: Init hw failure.\n");
> + err = -ENODEV;
> + goto release_irq;
> + }
> + init_timer(&dev->wd_timer);
> +
> + heci_initialize_clients(dev);
> + if (dev->heci_state != HECI_ENABLED) {
> + err = -ENODEV;
> + goto release_hw;
> + }
> +
> + spin_lock_bh(&dev->device_lock);
> + heci_device = pdev;
> + pci_set_drvdata(pdev, dev);

This seems to have come a bit late. Isn't this information needed in
the interrupt handler?

Bear in mind that your interrupt handler can be called even if the
device isn't generating an interrupt, due to IRQ sharing.

Please test with CONFIG_DEBUG_SHIRQ enabled.

> + spin_unlock_bh(&dev->device_lock);
> +
> + if (dev->wd_timeout)
> + mod_timer(&dev->wd_timer, jiffies);
> +
> +#ifdef CONFIG_PM
> + g_sus_wd_timeout = 0;
> +#endif
> + printk(KERN_INFO "heci driver initialization successful.\n");
> + return 0;
> +
> +release_hw:
> + /* disable interrupts */
> + dev->host_hw_state = read_heci_register(dev, H_CSR);
> + heci_csr_disable_interrupts(dev);
> +
> + del_timer_sync(&dev->wd_timer);
> +
> + flush_scheduled_work();

Might want cancel_work_sync() here.

> +release_irq:
> + free_irq(pdev->irq, dev);
> +unmap_memory:
> + if (dev->mem_addr)
> + iounmap(dev->mem_addr);
> +free_device:
> + kfree(dev);
> +release_regions:
> + pci_release_regions(pdev);
> +disable_device:
> + pci_disable_device(pdev);
> +end:
> + printk(KERN_ERR "heci driver initialization failed.\n");
> + return err;
> +}
> +
> +/**
> + * heci_remove - Device Removal Routine
> + *
> + * @pdev: PCI device information struct
> + *
> + * heci_remove is called by the PCI subsystem to alert the driver
> + * that it should release a PCI device.
> + */
> +static void __devexit heci_remove(struct pci_dev *pdev)
> +{
> + struct iamt_heci_device *dev = pci_get_drvdata(pdev);
> +
> + if (heci_device != pdev)
> + return;
> +
> + if (dev == NULL)
> + return;
> +
> + spin_lock_bh(&dev->device_lock);
> + if (heci_device != pdev) {
> + spin_unlock_bh(&dev->device_lock);
> + return;
> + }
> +
> + if (dev->reinit_tsk != NULL) {
> + kthread_stop(dev->reinit_tsk);
> + dev->reinit_tsk = NULL;
> + }

It is a bug to call kthread_stop() under spinlock.

Please always test new code with all kernel debugging options enabled.

Please review Documentation/SubmitChecklist.

> + del_timer_sync(&dev->wd_timer);
> + if (dev->wd_file_ext.state == HECI_FILE_CONNECTED
> + && dev->wd_timeout) {
> + dev->wd_timeout = 0;
> + dev->wd_due_counter = 0;
> + memcpy(dev->wd_data, heci_stop_wd_params, HECI_WD_PARAMS_SIZE);
> + dev->stop = 1;
> + if (dev->host_buffer_is_empty &&
> + flow_ctrl_creds(dev, &dev->wd_file_ext)) {
> + dev->host_buffer_is_empty = 0;
> +
> + if (!heci_send_wd(dev))
> + DBG("send stop WD failed\n");
> + else
> + flow_ctrl_reduce(dev, &dev->wd_file_ext);
> +
> + dev->wd_pending = 0;
> + } else {
> + dev->wd_pending = 1;
> + }
> + dev->wd_stoped = 0;
> + spin_unlock_bh(&dev->device_lock);
> +
> + wait_event_interruptible_timeout(dev->wait_stop_wd,
> + (dev->wd_stoped), 10 * HZ);
> + spin_lock_bh(&dev->device_lock);
> + if (!dev->wd_stoped)
> + DBG("stop wd failed to complete.\n");
> + else
> + DBG("stop wd complete.\n");
> +
> + }
> +
> + heci_device = NULL;
> + spin_unlock_bh(&dev->device_lock);
> +
> + if (dev->iamthif_file_ext.state == HECI_FILE_CONNECTED) {
> + dev->iamthif_file_ext.state = HECI_FILE_DISCONNECTING;
> + heci_disconnect_host_client(dev,
> + &dev->iamthif_file_ext);
> + }
> + if (dev->wd_file_ext.state == HECI_FILE_CONNECTED) {
> + dev->wd_file_ext.state = HECI_FILE_DISCONNECTING;
> + heci_disconnect_host_client(dev,
> + &dev->wd_file_ext);
> + }
> +
> + spin_lock_bh(&dev->device_lock);
> +
> + /* 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);
> +
> + dev->iamthif_current_cb = NULL;
> + dev->iamthif_file_ext.file = NULL;
> + dev->num_heci_me_clients = 0;
> +
> + spin_unlock_bh(&dev->device_lock);
> +
> + flush_scheduled_work();
> +
> + /* disable interrupts */
> + heci_csr_disable_interrupts(dev);
> +
> + free_irq(pdev->irq, dev);
> + pci_set_drvdata(pdev, NULL);
> +
> + if (dev->mem_addr)
> + iounmap(dev->mem_addr);
> +
> + kfree(dev);
> +
> + pci_release_regions(pdev);
> + pci_disable_device(pdev);
> +}
> +
> +/**
> + * heci_clear_list - remove all callbacks associated with file
> + * from heci_cb_list
> + *
> + * @file: file information struct
> + * @heci_cb_list: callbacks list
> + *
> + * heci_clear_list is called to clear resources associated with file
> + * when application calls close function or Ctrl-C was pressed
> + *
> + * returns 1 if callback removed from the list, 0 otherwise
> + */
> +static int heci_clear_list(struct iamt_heci_device *dev,
> + struct file *file, struct list_head *heci_cb_list)
> +{
> + struct heci_cb_private *priv_cb_pos = NULL;
> + struct heci_cb_private*priv_cb_next = NULL;
> + struct file *file_temp;
> + int rets = 0;
> +
> + /* list all list member */
> + list_for_each_entry_safe(priv_cb_pos, priv_cb_next,
> + heci_cb_list, cb_list) {
> + file_temp = (struct file *)priv_cb_pos->file_object;
> + /* check if list member associated with a file */
> + if (file_temp == file) {
> + /* remove member from the list */
> + list_del(&priv_cb_pos->cb_list);
> + /* check if cb equal to current iamthif cb */
> + if (dev->iamthif_current_cb == priv_cb_pos) {
> + dev->iamthif_current_cb = NULL;
> + /* send flow control to iamthif client */
> + heci_send_flow_control(dev,
> + &dev->iamthif_file_ext);
> + }
> + /* free all allocated buffers */
> + heci_free_cb_private(priv_cb_pos);
> + rets = 1;
> + }
> + }
> + return rets;
> +}
> +
> +/**
> + * heci_clear_lists - remove all callbacks associated with file
> + *
> + * @dev: device information struct
> + * @file: file information struct
> + *
> + * heci_clear_lists is called to clear resources associated with file
> + * when application calls close function or Ctrl-C was pressed
> + *
> + * returns 1 if callback removed from the list, 0 otherwise
> + */
> +static int heci_clear_lists(struct iamt_heci_device *dev, struct file *file)
> +{
> + int rets = 0;
> +
> + /* remove callbacks associated with a file */
> + heci_clear_list(dev, file, &dev->pthi_cmd_list.heci_cb.cb_list);
> + if (heci_clear_list(dev, file,
> + &dev->pthi_read_complete_list.heci_cb.cb_list))
> + rets = 1;
> +
> + heci_clear_list(dev, file, &dev->ctrl_rd_list.heci_cb.cb_list);
> +
> + if (heci_clear_list(dev, file, &dev->ctrl_wr_list.heci_cb.cb_list))
> + rets = 1;
> +
> + if (heci_clear_list(dev, file,
> + &dev->write_waiting_list.heci_cb.cb_list))
> + rets = 1;
> +
> + if (heci_clear_list(dev, file, &dev->write_list.heci_cb.cb_list))
> + rets = 1;
> +
> + /* check if iamthif_current_cb not NULL */
> + if (dev->iamthif_current_cb && (!rets)) {
> + /* check file and iamthif current cb association */
> + if (dev->iamthif_current_cb->file_object == file) {
> + /* remove cb */
> + heci_free_cb_private(dev->iamthif_current_cb);
> + dev->iamthif_current_cb = NULL;
> + rets = 1;
> + }
> + }
> + return rets;
> +}
> +
> +/**
> + * heci_open - the open function
> + *
> + * @inode: pointer to inode structure
> + * @file: pointer to file structure
> + *
> + * returns 0 on success, <0 on error
> + */
> +static int heci_open(struct inode *inode, struct file *file)
> +{
> + struct heci_file_private *file_ext;
> + int if_num = iminor(inode);
> + struct iamt_heci_device *dev;
> +
> + if (!heci_device)
> + return -ENODEV;
> +
> + dev = pci_get_drvdata(heci_device);
> + if ((if_num != HECI_MINOR_NUMBER) || (!dev))

Surely these can't happen?

> + return -ENODEV;
> +
> + file_ext = heci_alloc_file_private(file);

Has this driver been tested with multiple processes all manipulating
the same device?

> + if (file_ext == NULL)
> + return -ENOMEM;
> +
> + spin_lock_bh(&dev->device_lock);
> + if (dev->heci_state != HECI_ENABLED) {
> + spin_unlock_bh(&dev->device_lock);
> + kfree(file_ext);
> + return -ENODEV;
> + }
> + if (dev->open_handle_count >= HECI_MAX_OPEN_HANDLE_COUNT) {
> + spin_unlock_bh(&dev->device_lock);
> + kfree(file_ext);
> + return -ENFILE;
> + };
> + dev->open_handle_count++;
> + list_add_tail(&file_ext->link, &dev->file_list);
> + while ((dev->heci_host_clients[dev->current_host_client_id / 8]
> + & (1 << (dev->current_host_client_id % 8))) != 0) {

find_next_bit()?

> + dev->current_host_client_id++; /* allow overflow */
> + DBG("current_host_client_id = %d\n",
> + dev->current_host_client_id);
> + DBG("dev->open_handle_count = %lu\n",
> + dev->open_handle_count);
> + }
> + DBG("current_host_client_id = %d\n", dev->current_host_client_id);
> + file_ext->host_client_id = dev->current_host_client_id;
> + dev->heci_host_clients[file_ext->host_client_id / 8] |=
> + (1 << (file_ext->host_client_id % 8));
> + spin_unlock_bh(&dev->device_lock);
> + spin_lock(&file_ext->file_lock);
> + file_ext->state = HECI_FILE_INITIALIZING;
> + file_ext->sm_state = 0;
> +
> + file->private_data = file_ext;
> + spin_unlock(&file_ext->file_lock);
> +
> + return 0;
> +}
> +
> +/**
> + * heci_release - the release function
> + *
> + * @inode: pointer to inode structure
> + * @file: pointer to file structure
> + *
> + * returns 0 on success, <0 on error
> + */
> +static int heci_release(struct inode *inode, struct file *file)
> +{
> + int rets = 0;
> + int if_num = iminor(inode);
> + struct heci_file_private *file_ext = file->private_data;
> + struct heci_cb_private *priv_cb = NULL;
> + struct iamt_heci_device *dev;
> +
> + if (!heci_device)
> + return -ENODEV;
> +
> + dev = pci_get_drvdata(heci_device);
> + if ((if_num != HECI_MINOR_NUMBER) || (!dev) || (!file_ext))

?

> + return -ENODEV;
> +
> + if (file_ext != &dev->iamthif_file_ext) {

I don't understand what this is all about.

> + spin_lock(&file_ext->file_lock);
> + if (file_ext->state == HECI_FILE_CONNECTED) {
> + file_ext->state = HECI_FILE_DISCONNECTING;
> + spin_unlock(&file_ext->file_lock);
> + DBG("disconnecting client host client = %d, "
> + "ME client = %d\n",
> + file_ext->host_client_id,
> + file_ext->me_client_id);
> + rets = heci_disconnect_host_client(dev, file_ext);
> + spin_lock(&file_ext->file_lock);
> + }
> + spin_lock_bh(&dev->device_lock);
> + heci_flush_queues(dev, file_ext);
> + DBG("remove client host client = %d, ME client = %d\n",
> + file_ext->host_client_id,
> + file_ext->me_client_id);
> +
> + if (dev->open_handle_count > 0) {
> + dev->heci_host_clients[file_ext->host_client_id / 8] &=
> + ~(1 << (file_ext->host_client_id % 8));

I think you wanted __set_bit() and __clear_bit() here.

> + dev->open_handle_count--;
> + }
> + heci_remove_client_from_file_list(dev,
> + file_ext->host_client_id);
> +
> + /* free read cb */
> + if (file_ext->read_cb != NULL) {
> + priv_cb = find_read_list_entry(dev, file_ext);
> + /* Remove entry from read list */
> + if (priv_cb != NULL)
> + list_del(&priv_cb->cb_list);
> +
> + priv_cb = file_ext->read_cb;
> + file_ext->read_cb = NULL;
> + }
> +
> + spin_unlock_bh(&dev->device_lock);
> + file->private_data = NULL;

Is this needed?

> + spin_unlock(&file_ext->file_lock);

Surely there's nothing to lock against in the ->release() handler?

> + if (priv_cb != NULL)
> + heci_free_cb_private(priv_cb);
> +
> + kfree(file_ext);
> + } else {
> + spin_lock_bh(&dev->device_lock);
> +
> + if (dev->open_handle_count > 0)
> + dev->open_handle_count--;

Is the test needed? Surely it would be a huge bug for
open_handle_count to go negative, and we want to know about huge bugs,
not hide them.

> + if (dev->iamthif_file_object == file
> + && dev->iamthif_state != HECI_IAMTHIF_IDLE) {
> + DBG("pthi canceled iamthif state %d\n",
> + dev->iamthif_state);
> + dev->iamthif_canceled = 1;
> + if (dev->iamthif_state == HECI_IAMTHIF_READ_COMPLETE) {
> + DBG("run next pthi iamthif cb\n");
> + run_next_iamthif_cmd(dev);
> + }
> + }
> +
> + if (heci_clear_lists(dev, file))
> + dev->iamthif_state = HECI_IAMTHIF_IDLE;
> +
> + spin_unlock_bh(&dev->device_lock);
> + }
> + return rets;
> +}
> +
> +static struct heci_cb_private *find_read_list_entry(
> + struct iamt_heci_device *dev,
> + struct heci_file_private *file_ext)
> +{
> + struct heci_cb_private *priv_cb_pos = NULL;
> + struct heci_cb_private *priv_cb_next = NULL;
> + struct heci_file_private *file_ext_list_temp;
> +
> + if (dev->read_list.status == 0
> + && !list_empty(&dev->read_list.heci_cb.cb_list)) {
> + DBG("remove read_list CB \n");
> + list_for_each_entry_safe(priv_cb_pos,
> + priv_cb_next,
> + &dev->read_list.heci_cb.cb_list, cb_list) {
> +
> + file_ext_list_temp = (struct heci_file_private *)
> + priv_cb_pos->file_private;
> +
> + if ((file_ext_list_temp != NULL) &&
> + heci_fe_same_id(file_ext, file_ext_list_temp))
> + return priv_cb_pos;
> +
> + }
> + }
> + return NULL;
> +}
> +
> +/**
> + * heci_read - the read client message function.
> + *
> + * @file: pointer to file structure
> + * @ubuf: pointer to user buffer
> + * @length: buffer length
> + * @offset: data offset in buffer
> + *
> + * returns >=0 data length on success , <0 on error
> + */
> +static ssize_t heci_read(struct file *file, char __user *ubuf,
> + size_t length, loff_t *offset)
> +{
> + int i;
> + int rets = 0, err = 0;
> + int if_num = iminor(file->f_dentry->d_inode);
> + struct heci_file_private *file_ext = file->private_data;
> + struct heci_cb_private *priv_cb_pos = NULL;
> + struct heci_cb_private *priv_cb = NULL;
> + struct iamt_heci_device *dev;
> +
> + if (!heci_device)

?

> + return -ENODEV;
> +
> + dev = pci_get_drvdata(heci_device);
> + if ((if_num != HECI_MINOR_NUMBER) || (!dev) || (!file_ext))

?

> + return -ENODEV;
> +
> + spin_lock_bh(&dev->device_lock);
> + if (dev->heci_state != HECI_ENABLED) {
> + spin_unlock_bh(&dev->device_lock);
> + return -ENODEV;
> + }
> + spin_unlock_bh(&dev->device_lock);

Is this racy? Wat prevents dev->heci_state from getting set to
something other than HECI_ENABLED just after we dropped the lock?

If that's a cant-happen, then is the locking needed?

> + spin_lock(&file_ext->file_lock);
> + if ((file_ext->sm_state & HECI_WD_STATE_INDEPENDENCE_MSG_SENT) == 0) {
> + spin_unlock(&file_ext->file_lock);
> + /* Do not allow to read watchdog client */
> + for (i = 0; i < dev->num_heci_me_clients; i++) {
> + if (memcmp(&heci_wd_guid,
> + &dev->me_clients[i].props.protocol_name,
> + sizeof(struct guid)) == 0) {
> + if (file_ext->me_client_id ==
> + dev->me_clients[i].client_id)
> + return -EBADF;
> + }
> + }
> + } else {
> + file_ext->sm_state &= ~HECI_WD_STATE_INDEPENDENCE_MSG_SENT;
> + spin_unlock(&file_ext->file_lock);
> + }
> +
> + if (file_ext == &dev->iamthif_file_ext) {

I wonder what this "file_ext" concept is.

> + rets = pthi_read(dev, if_num, file, ubuf, length, offset);
> + goto out;
> + }
> +
> + if (file_ext->read_cb && file_ext->read_cb->information > *offset) {
> + priv_cb = file_ext->read_cb;
> + goto copy_buffer;
> + } else if (file_ext->read_cb && file_ext->read_cb->information > 0 &&
> + file_ext->read_cb->information <= *offset) {
> + priv_cb = file_ext->read_cb;
> + rets = 0;
> + goto free;
> + } else if ((!file_ext->read_cb || file_ext->read_cb->information == 0)
> + && *offset > 0) {
> + /*Offset needs to be cleaned for contingous reads*/
> + *offset = 0;
> + rets = 0;
> + goto out;
> + }
> +
> + spin_lock(&file_ext->read_io_lock);
> + err = heci_start_read(dev, if_num, file_ext);
> + if (err != 0 && err != -EBUSY) {
> + DBG("heci start read failure with status = %d\n", err);
> + spin_unlock(&file_ext->read_io_lock);
> + rets = err;
> + goto out;
> + }
> + if (HECI_READ_COMPLETE != file_ext->reading_state
> + && !waitqueue_active(&file_ext->rx_wait)) {
> + if (file->f_flags & O_NONBLOCK) {
> + rets = -EAGAIN;
> + spin_unlock(&file_ext->read_io_lock);
> + goto out;
> + }
> + spin_unlock(&file_ext->read_io_lock);
> +
> + if (wait_event_interruptible(file_ext->rx_wait,
> + (HECI_READ_COMPLETE == file_ext->reading_state
> + || HECI_FILE_INITIALIZING == file_ext->state
> + || HECI_FILE_DISCONNECTED == file_ext->state
> + || HECI_FILE_DISCONNECTING == file_ext->state))) {
> + if (signal_pending(current)) {
> + rets = -EINTR;
> + goto out;
> + }
> + return -ERESTARTSYS;

Should we be returning -ERESTARTSYS if !signal_pending()?

> + }
> +
> + if (HECI_FILE_INITIALIZING == file_ext->state ||
> + HECI_FILE_DISCONNECTED == file_ext->state ||
> + HECI_FILE_DISCONNECTING == file_ext->state) {
> + rets = -EBUSY;
> + goto out;
> + }
> + spin_lock(&file_ext->read_io_lock);
> + }
> +
> + priv_cb = file_ext->read_cb;
> +
> + if (!priv_cb) {
> + spin_unlock(&file_ext->read_io_lock);
> + return -ENODEV;
> + }
> + if (file_ext->reading_state != HECI_READ_COMPLETE) {
> + spin_unlock(&file_ext->read_io_lock);
> + return 0;
> + }
> + spin_unlock(&file_ext->read_io_lock);
> + /* now copy the data to user space */
> +copy_buffer:
> + DBG("priv_cb->response_buffer size - %d\n",
> + priv_cb->response_buffer.size);
> + DBG("priv_cb->information - %lu\n",
> + priv_cb->information);
> + if (length == 0 || ubuf == NULL ||
> + *offset > priv_cb->information) {

I don't think `length == 0' is possible here.

If the user passed in a NULL pointer then the appropriate return is
-EFAULT, not -EMSGSIZE.

But we shouldn't assume that zero is an illegal address. Just let
copy_to_user() take care of it.

> + rets = -EMSGSIZE;
> + goto free;
> + }
> +
> + /* length is being turncated to PAGE_SIZE, however, */
> + /* information size may be longer */
> + length = (length < (priv_cb->information - *offset) ?
> + length : (priv_cb->information - *offset));
> +
> + if (copy_to_user(ubuf,
> + priv_cb->response_buffer.data + *offset,
> + length)) {
> + rets = -EFAULT;
> + goto free;

We don't handle partial copies?

The usual read() behaviour is to return a short read, and only return
-EFOO if zero bytes were copied out to userspace.

> + }
> +
> + rets = length;
> + *offset += length;
> + if ((unsigned long)*offset < priv_cb->information)
> + goto out;
> +
> +free:
> + spin_lock_bh(&dev->device_lock);
> + priv_cb_pos = find_read_list_entry(dev, file_ext);
> + /* Remove entry from read list */
> + if (priv_cb_pos != NULL)
> + list_del(&priv_cb_pos->cb_list);
> + spin_unlock_bh(&dev->device_lock);
> + heci_free_cb_private(priv_cb);
> + spin_lock(&file_ext->read_io_lock);
> + file_ext->reading_state = HECI_IDLE;
> + file_ext->read_cb = NULL;
> + file_ext->read_pending = 0;
> + spin_unlock(&file_ext->read_io_lock);
> +out: DBG("end heci read rets= %d\n", rets);
> + return rets;
> +}
> +
> +/**
> + * heci_write - the write function.
> + *
> + * @file: pointer to file structure
> + * @ubuf: pointer to user buffer
> + * @length: buffer length
> + * @offset: data offset in buffer
> + *
> + * returns >=0 data length on success , <0 on error
> + */
> +static ssize_t heci_write(struct file *file, const char __user *ubuf,
> + size_t length, loff_t *offset)
> +{
> + int rets = 0;
> + __u8 i;
> + int if_num = iminor(file->f_dentry->d_inode);
> + struct heci_file_private *file_ext = file->private_data;
> + struct heci_cb_private *priv_write_cb = NULL;
> + struct heci_msg_hdr heci_hdr;
> + struct iamt_heci_device *dev;
> + unsigned long currtime = get_seconds();
> +
> + if (!heci_device)

?

> + return -ENODEV;
> +
> + dev = pci_get_drvdata(heci_device);
> +
> + if ((if_num != HECI_MINOR_NUMBER) || (!dev) || (!file_ext))

?

> + return -ENODEV;
> +
> + spin_lock_bh(&dev->device_lock);
> +
> + if (dev->heci_state != HECI_ENABLED) {
> + spin_unlock_bh(&dev->device_lock);
> + return -ENODEV;
> + }
> + if (file_ext == &dev->iamthif_file_ext) {
> + priv_write_cb = find_pthi_read_list_entry(dev, file);
> + if ((priv_write_cb != NULL) &&
> + (((currtime - priv_write_cb->read_time) >
> + IAMTHIF_READ_TIMER) ||
> + (file_ext->reading_state == HECI_READ_COMPLETE))) {
> + (*offset) = 0;
> + list_del(&priv_write_cb->cb_list);
> + heci_free_cb_private(priv_write_cb);
> + priv_write_cb = NULL;
> + }
> + }
> +
> + /* free entry used in read */
> + if (file_ext->reading_state == HECI_READ_COMPLETE) {
> + *offset = 0;
> + priv_write_cb = find_read_list_entry(dev, file_ext);
> + if (priv_write_cb != NULL) {
> + list_del(&priv_write_cb->cb_list);
> + heci_free_cb_private(priv_write_cb);
> + priv_write_cb = NULL;
> + spin_lock(&file_ext->read_io_lock);
> + file_ext->reading_state = HECI_IDLE;
> + file_ext->read_cb = NULL;
> + file_ext->read_pending = 0;
> + spin_unlock(&file_ext->read_io_lock);
> + }
> + } else if (file_ext->reading_state == HECI_IDLE &&
> + file_ext->read_pending == 0)
> + (*offset) = 0;
> +
> + spin_unlock_bh(&dev->device_lock);
> +
> + priv_write_cb = kzalloc(sizeof(struct heci_cb_private), GFP_KERNEL);
> + if (!priv_write_cb)
> + return -ENOMEM;
> +
> + priv_write_cb->file_object = file;
> + priv_write_cb->file_private = file_ext;
> + priv_write_cb->request_buffer.data = kmalloc(length, GFP_KERNEL);
> + if (!priv_write_cb->request_buffer.data) {
> + kfree(priv_write_cb);
> + return -ENOMEM;
> + }
> + DBG("length =%d\n", (int) length);
> +
> + if (copy_from_user(priv_write_cb->request_buffer.data,
> + ubuf, length)) {
> + rets = -EFAULT;
> + goto fail;
> + }

Various dittoes.

> + spin_lock(&file_ext->file_lock);
> + file_ext->sm_state = 0;
> + if ((length == 4) &&
> + ((memcmp(heci_wd_state_independence_msg[0], ubuf, 4) == 0) ||
> + (memcmp(heci_wd_state_independence_msg[1], ubuf, 4) == 0) ||
> + (memcmp(heci_wd_state_independence_msg[2], ubuf, 4) == 0)))
> + file_ext->sm_state |= HECI_WD_STATE_INDEPENDENCE_MSG_SENT;
> + spin_unlock(&file_ext->file_lock);
> +
> + INIT_LIST_HEAD(&priv_write_cb->cb_list);
> + if (file_ext == &dev->iamthif_file_ext) {
> + priv_write_cb->response_buffer.data =
> + kmalloc(IAMTHIF_MTU, GFP_KERNEL);
> + if (!priv_write_cb->response_buffer.data) {
> + rets = -ENOMEM;
> + goto fail;
> + }
> + spin_lock_bh(&dev->device_lock);
> + if (dev->heci_state != HECI_ENABLED) {
> + spin_unlock_bh(&dev->device_lock);
> + rets = -ENODEV;
> + goto fail;
> + }
> + for (i = 0; i < dev->num_heci_me_clients; i++) {
> + if (dev->me_clients[i].client_id ==
> + dev->iamthif_file_ext.me_client_id)
> + break;
> + }
> +
> + BUG_ON(dev->me_clients[i].client_id != file_ext->me_client_id);
> + if ((i == dev->num_heci_me_clients) ||
> + (dev->me_clients[i].client_id !=
> + dev->iamthif_file_ext.me_client_id)) {
> +
> + spin_unlock_bh(&dev->device_lock);
> + rets = -ENODEV;
> + goto fail;
> + } else if ((length > dev->me_clients[i].props.max_msg_length)
> + || (length <= 0)) {
> + spin_unlock_bh(&dev->device_lock);
> + rets = -EMSGSIZE;
> + goto fail;

Are you sure none of these error paths are leaking resources?

> + }
> +
> +
> + priv_write_cb->response_buffer.size = IAMTHIF_MTU;
> + priv_write_cb->major_file_operations = HECI_IOCTL;
> + priv_write_cb->information = 0;
> + priv_write_cb->request_buffer.size = length;
> + if (dev->iamthif_file_ext.state != HECI_FILE_CONNECTED) {
> + spin_unlock_bh(&dev->device_lock);
> + rets = -ENODEV;
> + goto fail;
> + }
> +
> + if (!list_empty(&dev->pthi_cmd_list.heci_cb.cb_list)
> + || dev->iamthif_state != HECI_IAMTHIF_IDLE) {
> + DBG("pthi_state = %d\n", (int) dev->iamthif_state);
> + DBG("add PTHI cb to pthi cmd waiting list\n");
> + list_add_tail(&priv_write_cb->cb_list,
> + &dev->pthi_cmd_list.heci_cb.cb_list);
> + rets = length;
> + } else {
> + DBG("call pthi write\n");
> + rets = pthi_write(dev, priv_write_cb);
> +
> + if (rets != 0) {
> + DBG("pthi write failed with status = %d\n",
> + rets);
> + spin_unlock_bh(&dev->device_lock);
> + goto fail;
> + }
> + rets = length;
> + }
> + spin_unlock_bh(&dev->device_lock);
> + return rets;
> + }
> +
> + priv_write_cb->major_file_operations = HECI_WRITE;
> + /* make sure information is zero before we start */
> +
> + priv_write_cb->information = 0;
> + priv_write_cb->request_buffer.size = length;
> +
> + spin_lock(&file_ext->write_io_lock);
> + DBG("host client = %d, ME client = %d\n",
> + file_ext->host_client_id, file_ext->me_client_id);
> + if (file_ext->state != HECI_FILE_CONNECTED) {
> + rets = -ENODEV;
> + DBG("host client = %d, is not connected to ME client = %d",
> + file_ext->host_client_id,
> + file_ext->me_client_id);
> +
> + goto unlock;
> + }
> + for (i = 0; i < dev->num_heci_me_clients; i++) {
> + if (dev->me_clients[i].client_id ==
> + file_ext->me_client_id)

Please don't put line breaks where they are unneeded.

This doesn't look very efficient.

> + break;
> + }
> + BUG_ON(dev->me_clients[i].client_id != file_ext->me_client_id);
> + if (i == dev->num_heci_me_clients) {
> + rets = -ENODEV;
> + goto unlock;
> + }
> + if (length > dev->me_clients[i].props.max_msg_length || length <= 0) {
> + rets = -EINVAL;
> + goto unlock;
> + }

Somewhat strange behaviour for a write() handler, but understandable.
It would all benefit from some documentation.

> + priv_write_cb->file_private = file_ext;
> +
> + spin_lock_bh(&dev->device_lock);
> + if (flow_ctrl_creds(dev, file_ext) &&
> + dev->host_buffer_is_empty) {
> + spin_unlock_bh(&dev->device_lock);
> + dev->host_buffer_is_empty = 0;
> + if (length > ((((dev->host_hw_state & H_CBD) >> 24) *
> + sizeof(__u32)) - sizeof(struct heci_msg_hdr))) {
> +
> + heci_hdr.length =
> + (((dev->host_hw_state & H_CBD) >> 24) *
> + sizeof(__u32)) -
> + sizeof(struct heci_msg_hdr);
> + heci_hdr.msg_complete = 0;
> + } else {
> + heci_hdr.length = length;
> + heci_hdr.msg_complete = 1;
> + }
> + heci_hdr.host_addr = file_ext->host_client_id;
> + heci_hdr.me_addr = file_ext->me_client_id;
> + heci_hdr.reserved = 0;
> + DBG("call heci_write_message header=%08x.\n",
> + *((__u32 *) &heci_hdr));
> + spin_unlock(&file_ext->write_io_lock);
> + /* protect heci low level write */
> + spin_lock_bh(&dev->device_lock);
> + if (!heci_write_message(dev, &heci_hdr,
> + (unsigned char *) (priv_write_cb->request_buffer.data),
> + heci_hdr.length)) {
> +
> + spin_unlock_bh(&dev->device_lock);
> + heci_free_cb_private(priv_write_cb);
> + rets = -ENODEV;
> + priv_write_cb->information = 0;
> + return rets;
> + }
> + file_ext->writing_state = HECI_WRITING;
> + priv_write_cb->information = heci_hdr.length;
> + if (heci_hdr.msg_complete) {
> + flow_ctrl_reduce(dev, file_ext);
> + list_add_tail(&priv_write_cb->cb_list,
> + &dev->write_waiting_list.heci_cb.cb_list);
> + } else {
> + list_add_tail(&priv_write_cb->cb_list,
> + &dev->write_list.heci_cb.cb_list);
> + }
> + spin_unlock_bh(&dev->device_lock);
> +
> + } else {
> +
> + spin_unlock_bh(&dev->device_lock);
> + priv_write_cb->information = 0;
> + file_ext->writing_state = HECI_WRITING;
> + spin_unlock(&file_ext->write_io_lock);
> + list_add_tail(&priv_write_cb->cb_list,
> + &dev->write_list.heci_cb.cb_list);
> + }
> + return length;
> +
> +unlock:
> + spin_unlock(&file_ext->write_io_lock);
> +fail:
> + heci_free_cb_private(priv_write_cb);
> + return rets;
> +
> +}
> +
> +/**
> + * heci_ioctl - the IOCTL function
> + *
> + * @inode: pointer to inode structure
> + * @file: pointer to file structure
> + * @cmd: ioctl command
> + * @data: pointer to heci message structure
> + *
> + * returns 0 on success , <0 on error
> + */
> +static int heci_ioctl(struct inode *inode, struct file *file,
> + unsigned int cmd, unsigned long data)
> +{
> + int rets = 0;
> + int if_num = iminor(inode);
> + struct heci_file_private *file_ext = file->private_data;
> + /* in user space */
> + struct heci_message_data *u_msg = (struct heci_message_data *) data;

Shouldn't this be a __user pointer?

This matters. Please get the __user annotations correct, then check it
all with sparse. That includes heci_message_data.data.

> + struct heci_message_data k_msg; /* all in kernel on the stack */
> + struct iamt_heci_device *dev;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + if (!heci_device)

?

> + return -ENODEV;
> +
> + dev = pci_get_drvdata(heci_device);
> + if ((if_num != HECI_MINOR_NUMBER) || (!dev) || (!file_ext))

?

> + return -ENODEV;
> +
> + spin_lock_bh(&dev->device_lock);
> + if (dev->heci_state != HECI_ENABLED) {
> + spin_unlock_bh(&dev->device_lock);
> + return -ENODEV;
> + }
> + spin_unlock_bh(&dev->device_lock);
> +
> + /* first copy from user all data needed */
> + if (copy_from_user(&k_msg, u_msg, sizeof(k_msg))) {

Is all of this untrusted data coming from userspace exhaustively checked?

> + DBG("first copy from user all data needed filled\n");
> + return -EFAULT;
> + }
> + DBG("user message size is %d\n", k_msg.size);
> +
> + switch (cmd) {
> + case IOCTL_HECI_GET_VERSION:
> + DBG(": IOCTL_HECI_GET_VERSION\n");
> + rets = heci_ioctl_get_version(dev, if_num, u_msg, k_msg,
> + file_ext);
> + break;
> +
> + case IOCTL_HECI_CONNECT_CLIENT:
> + DBG(": IOCTL_HECI_CONNECT_CLIENT.\n");
> + rets = heci_ioctl_connect_client(dev, if_num, u_msg, k_msg,
> + file);
> + break;
> +
> + case IOCTL_HECI_WD:
> + DBG(": IOCTL_HECI_WD.\n");
> + rets = heci_ioctl_wd(dev, if_num, k_msg, file_ext);
> + break;
> +
> + case IOCTL_HECI_BYPASS_WD:
> + DBG(": IOCTL_HECI_BYPASS_WD.\n");
> + rets = heci_ioctl_bypass_wd(dev, if_num, k_msg, file_ext);
> + break;
> +
> + default:
> + rets = -EINVAL;

-ENOTTY, I believe.

> + break;
> + }
> + return rets;
> +}
> +
> +/**
> + * heci_poll - the poll function
> + *
> + * @file: pointer to file structure
> + * @wait: pointer to poll_table structure
> + *
> + * returns poll mask
> + */
> +static unsigned int heci_poll(struct file *file, poll_table *wait)
> +{
> + int if_num = iminor(file->f_dentry->d_inode);
> + unsigned int mask = 0;
> + struct heci_file_private *file_ext = file->private_data;
> + struct iamt_heci_device *dev;
> +
> + if (!heci_device)

?

> + return mask;
> +
> + dev = pci_get_drvdata(heci_device);
> +
> + if ((if_num != HECI_MINOR_NUMBER) || (!dev) || (!file_ext))

?

> + return mask;
> +
> + spin_lock_bh(&dev->device_lock);
> + if (dev->heci_state != HECI_ENABLED) {
> + spin_unlock_bh(&dev->device_lock);
> + return mask;
> + }
> + spin_unlock_bh(&dev->device_lock);
> +
> + if (file_ext == &dev->iamthif_file_ext) {
> + poll_wait(file, &dev->iamthif_file_ext.wait, wait);
> + spin_lock(&dev->iamthif_file_ext.file_lock);
> + if (dev->iamthif_state == HECI_IAMTHIF_READ_COMPLETE
> + && dev->iamthif_file_object == file) {
> + mask |= (POLLIN | POLLRDNORM);
> + spin_lock_bh(&dev->device_lock);
> + DBG("run next pthi cb\n");
> + run_next_iamthif_cmd(dev);
> + spin_unlock_bh(&dev->device_lock);
> + }
> + spin_unlock(&dev->iamthif_file_ext.file_lock);
> +

Stray newline

> + } else{

checkpatch missed this coding-style error.

> + poll_wait(file, &file_ext->tx_wait, wait);
> + spin_lock(&file_ext->write_io_lock);
> + if (HECI_WRITE_COMPLETE == file_ext->writing_state)
> + mask |= (POLLIN | POLLRDNORM);
> +
> + spin_unlock(&file_ext->write_io_lock);
> + }
> +
> + return mask;
> +}
> +
> +#ifdef CONFIG_PM
> +static int heci_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> + struct iamt_heci_device *dev = pci_get_drvdata(pdev);
> + int err = 0;
> +
> + spin_lock_bh(&dev->device_lock);
> + if (dev->reinit_tsk != NULL) {
> + kthread_stop(dev->reinit_tsk);

can't run kthread_stop() under spinlock.

> + dev->reinit_tsk = NULL;
> + }
> + spin_unlock_bh(&dev->device_lock);
> +
> + /* Stop watchdog if exists */
> + del_timer_sync(&dev->wd_timer);
> + if (dev->wd_file_ext.state == HECI_FILE_CONNECTED
> + && dev->wd_timeout) {
> + spin_lock_bh(&dev->device_lock);
> + g_sus_wd_timeout = dev->wd_timeout;
> + dev->wd_timeout = 0;
> + dev->wd_due_counter = 0;
> + memcpy(dev->wd_data, heci_stop_wd_params,
> + HECI_WD_PARAMS_SIZE);
> + dev->stop = 1;
> + if (dev->host_buffer_is_empty &&
> + flow_ctrl_creds(dev, &dev->wd_file_ext)) {
> + dev->host_buffer_is_empty = 0;
> + if (!heci_send_wd(dev))
> + DBG("send stop WD failed\n");
> + else
> + flow_ctrl_reduce(dev, &dev->wd_file_ext);
> +
> + dev->wd_pending = 0;
> + } else {
> + dev->wd_pending = 1;
> + }
> + spin_unlock_bh(&dev->device_lock);
> + dev->wd_stoped = 0;
> +
> + err = wait_event_interruptible_timeout(dev->wait_stop_wd,
> + (dev->wd_stoped),

> + 10 * HZ);
> + if (!dev->wd_stoped)
> + DBG("stop wd failed to complete.\n");
> + else {
> + DBG("stop wd complete %d.\n", err);
> + err = 0;
> + }

Could we use plain old kthread_stop() here?

> + }
> + /* Set new heci state */
> + spin_lock_bh(&dev->device_lock);
> + if (dev->heci_state == HECI_ENABLED ||
> + dev->heci_state == HECI_RECOVERING_FROM_RESET) {
> + dev->heci_state = HECI_POWER_DOWN;
> + heci_reset(dev, 0);
> + }
> + spin_unlock_bh(&dev->device_lock);
> +
> + pci_save_state(pdev);
> +
> + pci_disable_device(pdev);
> + free_irq(pdev->irq, dev);
> +
> + pci_set_power_state(pdev, PCI_D3hot);
> +
> + return err;
> +}
> +
> +static int heci_resume(struct pci_dev *pdev)
> +{
> + struct iamt_heci_device *dev;
> + int err = 0;
> +
> + pci_set_power_state(pdev, PCI_D0);
> + pci_restore_state(pdev);
> +
> + dev = pci_get_drvdata(pdev);
> + if (!dev)
> + return -ENODEV;
> +
> + /* request and enable interrupt */
> + err = request_irq(pdev->irq, heci_isr_interrupt, IRQF_SHARED,
> + heci_driver_name, dev);
> + if (err) {
> + printk(KERN_ERR "heci: Request_irq failure. irq = %d \n",
> + pdev->irq);
> + return err;
> + }
> +
> + spin_lock_bh(&dev->device_lock);
> + dev->heci_state = HECI_POWER_UP;
> + heci_reset(dev, 1);
> + spin_unlock_bh(&dev->device_lock);
> +
> + /* Start watchdog if stopped in suspend */
> + if (g_sus_wd_timeout != 0) {
> + dev->wd_timeout = g_sus_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));
> + dev->wd_due_counter = 1;
> +
> + if (dev->wd_timeout)
> + mod_timer(&dev->wd_timer, jiffies);
> +
> + g_sus_wd_timeout = 0;
> + }
> + return err;
> +}
> +#endif
> +
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_DESCRIPTION("Intel(R) Management Engine Interface");
> +MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_VERSION(HECI_DRIVER_VERSION);
> diff --git a/drivers/char/heci/heci_version.h b/drivers/char/heci/heci_version.h
> new file mode 100644
> index 0000000..797e84e
> --- /dev/null
> +++ b/drivers/char/heci/heci_version.h
> @@ -0,0 +1,54 @@
> +/*
> + * 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_VERSION_H
> +#define HECI_VERSION_H
> +
> +#define MAJOR_VERSION 5
> +#define MINOR_VERSION 0
> +#define QUICK_FIX_NUMBER 0
> +#define VER_BUILD 30
> +
> +#define HECI_DRV_VER1 __stringify(MAJOR_VERSION) "." __stringify(MINOR_VERSION)
> +#define HECI_DRV_VER2 __stringify(QUICK_FIX_NUMBER) "." __stringify(VER_BUILD)
> +
> +#define HECI_DRIVER_VERSION HECI_DRV_VER1 "." HECI_DRV_VER2

There's really little point in doing this. As soon as the driver hits
mainline, the version number becomes meaningless. Other people will
change it and won't update the version number. And even if this
driver's source remains unchanged, other parts of the kernel upon which
it depends will change.

So this string becomes useless and the only interesting question
becomes "which kernel version". Because that is the only means by
which support people can recreate the supportee's driver source.

> +#endif
> diff --git a/drivers/char/heci/interrupt.c b/drivers/char/heci/interrupt.c
> new file mode 100644
> index 0000000..aacd262
> --- /dev/null
> +++ b/drivers/char/heci/interrupt.c
> @@ -0,0 +1,1553 @@
> +/*
> + * 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/kthread.h>
> +
> +#include "heci.h"
> +#include "heci_interface.h"
> +
> +/*
> + * interrupt function prototypes
> + */
> +static void heci_bh_handler(struct work_struct *work);
> +static int heci_bh_read_handler(struct io_heci_list *complete_list,
> + struct iamt_heci_device *dev,
> + __s32 *slots);
> +static int heci_bh_write_handler(struct io_heci_list *complete_list,
> + struct iamt_heci_device *dev,
> + __s32 *slots);
> +static void heci_bh_read_bus_message(struct iamt_heci_device *dev,
> + struct heci_msg_hdr *heci_hdr);
> +static int heci_bh_read_pthi_message(struct io_heci_list *complete_list,
> + struct iamt_heci_device *dev,
> + struct heci_msg_hdr *heci_hdr);
> +static int heci_bh_read_client_message(struct io_heci_list *complete_list,
> + struct iamt_heci_device *dev,
> + struct heci_msg_hdr *heci_hdr);
> +static void heci_client_connect_response(struct iamt_heci_device *dev,
> + struct hbm_client_connect_response *connect_res);
> +static void heci_client_disconnect_response(struct iamt_heci_device *dev,
> + struct hbm_client_connect_response *disconnect_res);
> +static void heci_client_flow_control_response(struct iamt_heci_device *dev,
> + struct hbm_flow_control *flow_control);
> +static void heci_client_disconnect_request(struct iamt_heci_device *dev,
> + struct hbm_client_disconnect_request *disconnect_req);
> +
> +
> +/**
> + * heci_isr_interrupt - The ISR of the HECI device
> + *
> + * @irq: The irq number
> + * @dev_id: pointer to the device structure
> + *
> + * returns irqreturn_t
> + */
> +irqreturn_t heci_isr_interrupt(int irq, void *dev_id)
> +{
> + int err;
> + struct iamt_heci_device *dev = (struct iamt_heci_device *) dev_id;

Unneeded and undesirable cast of void*.

> + dev->host_hw_state = read_heci_register(dev, H_CSR);
> +
> + if ((dev->host_hw_state & H_IS) != H_IS)
> + return IRQ_NONE;
> +
> + /* disable interrupts */
> + heci_csr_disable_interrupts(dev);
> +
> + /*
> + * Our device interrupted, schedule work the heci_bh_handler
> + * to handle the interrupt processing. This needs to be a
> + * workqueue item since the handler can sleep.
> + */
> + PREPARE_WORK(&dev->work, heci_bh_handler);
> + DBG("schedule work the heci_bh_handler.\n");
> + err = schedule_work(&dev->work);
> + if (!err) {
> + printk(KERN_ERR "heci: schedule the heci_bh_handler"
> + " failed error=%x\n", err);
> + }
> + return IRQ_HANDLED;
> +}
> +
> +/**
> + * _heci_cmpl - process completed operation.
> + *
> + * @file_ext: private data of the file object.
> + * @priv_cb_pos: callback block.
> + */
> +static void _heci_cmpl(struct heci_file_private *file_ext,
> + struct heci_cb_private *priv_cb_pos)
> +{
> + if (priv_cb_pos->major_file_operations == HECI_WRITE) {
> + heci_free_cb_private(priv_cb_pos);
> + DBG("completing write call back.\n");
> + file_ext->writing_state = HECI_WRITE_COMPLETE;
> + if ((&file_ext->tx_wait) &&
> + waitqueue_active(&file_ext->tx_wait))
> + wake_up_interruptible(&file_ext->tx_wait);
> +
> + } else if (priv_cb_pos->major_file_operations == HECI_READ
> + && HECI_READING == file_ext->reading_state) {
> + DBG("completing read call back information= %lu\n",
> + priv_cb_pos->information);
> + file_ext->reading_state = HECI_READ_COMPLETE;
> + if ((&file_ext->rx_wait) &&
> + waitqueue_active(&file_ext->rx_wait))
> + wake_up_interruptible(&file_ext->rx_wait);
> +
> + }
> +}
> +
> +/**
> + * _heci_cmpl_iamthif - process completed iamthif operation.
> + *
> + * @dev: Device object for our driver.
> + * @priv_cb_pos: callback block.
> + */
> +static void _heci_cmpl_iamthif(struct iamt_heci_device *dev,
> + struct heci_cb_private *priv_cb_pos)
> +{
> + if (dev->iamthif_canceled != 1) {
> + dev->iamthif_state = HECI_IAMTHIF_READ_COMPLETE;
> + dev->iamthif_stall_timer = 0;
> + memcpy(priv_cb_pos->response_buffer.data,
> + dev->iamthif_msg_buf,
> + dev->iamthif_msg_buf_index);
> + list_add_tail(&priv_cb_pos->cb_list,
> + &dev->pthi_read_complete_list.heci_cb.cb_list);
> + DBG("pthi read completed.\n");
> + } else {
> + run_next_iamthif_cmd(dev);
> + }
> + if (&dev->iamthif_file_ext.wait) {
> + DBG("completing pthi call back.\n");
> + wake_up_interruptible(&dev->iamthif_file_ext.wait);
> + }
> +}
> +/**
> + * heci_bh_handler - function called after ISR to handle the interrupt
> + * processing.
> + *
> + * @work: pointer to the work structure
> + *
> + * NOTE: This function is called by schedule work
> + */
> +static void heci_bh_handler(struct work_struct *work)

"bh_handler" is not an appropriate name. A "bottom half" is an old
Linux name for something similar to a softirq. It is run in sort-of
interrupt context.

Hence this reader gets alarmed when he sees things like kthread_run()
being called from something which appears to be "bottom half" context.

> +{
> + struct iamt_heci_device *dev =
> + container_of(work, struct iamt_heci_device, work);
> + struct io_heci_list complete_list;
> + __s32 slots;
> + int rets;
> + struct heci_cb_private *cb_pos = NULL, *cb_next = NULL;
> + struct heci_file_private *file_ext;
> + int bus_message_received = 0;
> + struct task_struct *tsk;
> +
> + DBG("function called after ISR to handle the interrupt processing.\n");
> + /* initialize our complete list */
> + spin_lock_bh(&dev->device_lock);
> + heci_initialize_list(&complete_list, dev);
> + dev->host_hw_state = read_heci_register(dev, H_CSR);
> + dev->me_hw_state = read_heci_register(dev, ME_CSR_HA);
> +
> + /* check if ME wants a reset */
> + if (((dev->me_hw_state & ME_RDY_HRA) == 0)
> + && (dev->heci_state != HECI_RESETING)
> + && (dev->heci_state != HECI_INITIALIZING)) {
> + DBG("FW not ready.\n");
> + heci_reset(dev, 1);
> + spin_unlock_bh(&dev->device_lock);
> + return;
> + }
> +
> + /* check if we need to start the dev */
> + if ((dev->host_hw_state & H_RDY) == 0) {
> + if ((dev->me_hw_state & ME_RDY_HRA) == ME_RDY_HRA) {
> + DBG("we need to start the dev.\n");
> + dev->host_hw_state |= (H_IE | H_IG | H_RDY);
> + heci_set_csr_register(dev);
> + if (dev->heci_state == HECI_INITIALIZING) {
> + dev->recvd_msg = 1;
> + spin_unlock_bh(&dev->device_lock);
> + wake_up_interruptible(&dev->wait_recvd_msg);
> + return;
> +
> + } else {
> + spin_unlock_bh(&dev->device_lock);
> + tsk = kthread_run(heci_task_initialize_clients,
> + dev, "heci_reinit");
> + if (IS_ERR(tsk)) {
> + int rc = PTR_ERR(tsk);
> + printk(KERN_WARNING "heci: Unable to"
> + "start the heci thread: %d\n", rc);
> + }

So... we schedule this function via schedule_work() then from here,
kick off a short-lived kernel thread?

> + return;
> + }
> + } else {
> + DBG("enable interrupt FW not ready.\n");
> + heci_csr_enable_interrupts(dev);
> + spin_unlock_bh(&dev->device_lock);
> + return;
> + }
> + }
> + /* check slots avalable for reading */
> + slots = count_full_read_slots(dev);
> + DBG("slots =%08x extra_write_index =%08x.\n",
> + slots, dev->extra_write_index);
> + while ((slots > 0) && (!dev->extra_write_index)) {

This driver has lots of over-parenthesisation.

> + DBG("slots =%08x extra_write_index =%08x.\n", slots,
> + dev->extra_write_index);
> + DBG("call heci_bh_read_handler.\n");
> + rets = heci_bh_read_handler(&complete_list, dev, &slots);
> + if (rets != 0)
> + goto end;
> + }
> + rets = heci_bh_write_handler(&complete_list, dev, &slots);
> +end:
> + DBG("end of bottom half function.\n");
> + dev->host_hw_state = read_heci_register(dev, H_CSR);
> + dev->host_buffer_is_empty = host_buffer_is_empty(dev);
> +
> + if ((dev->host_hw_state & H_IS) == H_IS) {
> + /* acknowledge interrupt and disable interrupts */
> + heci_csr_disable_interrupts(dev);
> +
> + PREPARE_WORK(&dev->work, heci_bh_handler);
> + DBG("schedule work the heci_bh_handler.\n");
> + rets = schedule_work(&dev->work);

it's usually the case that code which adds schedule_work() should also
add a cancel_work_sync() somewhere.

> + if (!rets) {
> + printk(KERN_ERR "heci: schedule the heci_bh_handler"
> + " failed error=%x\n", rets);
> + }
> + } else {
> + heci_csr_enable_interrupts(dev);
> + }
> +
> + if (dev->recvd_msg && waitqueue_active(&dev->wait_recvd_msg)) {
> + DBG("received waiting bus message\n");
> + bus_message_received = 1;
> + }
> + spin_unlock_bh(&dev->device_lock);
> + if (bus_message_received) {
> + DBG("wake up dev->wait_recvd_msg\n");
> + wake_up_interruptible(&dev->wait_recvd_msg);
> + bus_message_received = 0;
> + }
> + if ((complete_list.status != 0)
> + || list_empty(&complete_list.heci_cb.cb_list))
> + return;
> +
> +
> + list_for_each_entry_safe(cb_pos, cb_next,
> + &complete_list.heci_cb.cb_list, cb_list) {
> + file_ext = (struct heci_file_private *)cb_pos->file_private;
> + list_del(&cb_pos->cb_list);
> + if (file_ext != NULL) {
> + if (file_ext != &dev->iamthif_file_ext) {
> + DBG("completing call back.\n");
> + _heci_cmpl(file_ext, cb_pos);
> + cb_pos = NULL;
> + } else if (file_ext == &dev->iamthif_file_ext) {
> + _heci_cmpl_iamthif(dev, cb_pos);
> + }
> + }
> + }
> +}
> +
> +
> +/**
> + * heci_bh_read_handler - bottom half read routine after ISR to
> + * handle the read processing.

This isn't a "bottom half" function.

> + * @cmpl_list: An instance of our list structure
> + * @dev: Device object for our driver
> + * @slots: slots to read.
> + *
> + * returns 0 on success, <0 on failure.
> + */
> +static int heci_bh_read_handler(struct io_heci_list *cmpl_list,
> + struct iamt_heci_device *dev,
> + __s32 *slots)
> +{
> + struct heci_msg_hdr *heci_hdr;
> + int ret = 0;
> + struct heci_file_private *file_pos = NULL;
> + struct heci_file_private *file_next = NULL;
> +
> + if (!dev->rd_msg_hdr) {
> + dev->rd_msg_hdr = read_heci_register(dev, ME_CB_RW);
> + DBG("slots=%08x.\n", *slots);
> + (*slots)--;
> + DBG("slots=%08x.\n", *slots);
> + }
> + heci_hdr = (struct heci_msg_hdr *) &dev->rd_msg_hdr;
> + DBG("heci_hdr->length =%d\n", heci_hdr->length);
> +
> + if ((heci_hdr->reserved) || !(dev->rd_msg_hdr)) {
> + DBG("corrupted message header.\n");
> + ret = -ECORRUPTED_MESSAGE_HEADER;
> + goto end;
> + }
> +
> + if ((heci_hdr->host_addr) || (heci_hdr->me_addr)) {
> + list_for_each_entry_safe(file_pos, file_next,
> + &dev->file_list, link) {
> + DBG("list_for_each_entry_safe read host"
> + " client = %d, ME client = %d\n",
> + file_pos->host_client_id,
> + file_pos->me_client_id);
> + if ((file_pos->host_client_id == heci_hdr->host_addr)
> + && (file_pos->me_client_id == heci_hdr->me_addr))
> + break;
> + }
> +
> + if (&file_pos->link == &dev->file_list) {
> + DBG("corrupted message header\n");
> + ret = -ECORRUPTED_MESSAGE_HEADER;
> + goto end;
> + }
> + }
> + if (((*slots) * sizeof(__u32)) < heci_hdr->length) {
> + DBG("we can't read the message slots=%08x.\n", *slots);
> + /* we can't read the message */
> + ret = -ERANGE;
> + goto end;
> + }
> +
> + /* decide where to read the message too */
> + if (!heci_hdr->host_addr) {
> + DBG("call heci_bh_read_bus_message.\n");
> + heci_bh_read_bus_message(dev, heci_hdr);
> + DBG("end heci_bh_read_bus_message.\n");
> + } else if ((heci_hdr->host_addr == dev->iamthif_file_ext.host_client_id)
> + && (HECI_FILE_CONNECTED == dev->iamthif_file_ext.state)
> + && (dev->iamthif_state == HECI_IAMTHIF_READING)) {
> + DBG("call heci_bh_read_iamthif_message.\n");
> + DBG("heci_hdr->length =%d\n", heci_hdr->length);
> + ret = heci_bh_read_pthi_message(cmpl_list, dev, heci_hdr);
> + if (ret != 0)
> + goto end;
> +
> + } else {
> + DBG("call heci_bh_read_client_message.\n");
> + ret = heci_bh_read_client_message(cmpl_list, dev, heci_hdr);
> + if (ret != 0)
> + goto end;
> +
> + }
> +
> + /* reset the number of slots and header */
> + *slots = count_full_read_slots(dev);
> + dev->rd_msg_hdr = 0;
> +
> + if (*slots == -ESLOTS_OVERFLOW) {
> + /* overflow - reset */
> + DBG("reseting due to slots overflow.\n");
> + /* set the event since message has been read */
> + ret = -ERANGE;
> + goto end;
> + }
> +end:
> + return ret;
> +}
> +
> +
> +/**
> + * heci_bh_read_bus_message - bottom half read routine after ISR to
> + * handle the read bus message cmd processing.
> + *
> + * @dev: Device object for our driver
> + * @heci_hdr: header of bus message
> + */
> +static void heci_bh_read_bus_message(struct iamt_heci_device *dev,
> + struct heci_msg_hdr *heci_hdr)
> +{
> + struct heci_bus_message *heci_msg;
> + struct hbm_host_version_response *version_res;
> + struct hbm_client_connect_response *connect_res;
> + struct hbm_client_connect_response *disconnect_res;
> + struct hbm_flow_control *flow_control;
> + struct hbm_props_response *props_res;
> + struct hbm_host_enum_response *enum_res;
> + struct hbm_client_disconnect_request *disconnect_req;
> + struct hbm_host_stop_request *h_stop_req;
> + int i;
> + unsigned char *buffer;
> +
> + /* read the message to our buffer */
> + buffer = (unsigned char *) dev->rd_msg_buf;
> + BUG_ON(heci_hdr->length >= sizeof(dev->rd_msg_buf));
> + heci_read_slots(dev, buffer, heci_hdr->length);
> + heci_msg = (struct heci_bus_message *) buffer;
> +
> + switch (*(__u8 *) heci_msg) {
> + case HOST_START_RES_CMD:
> + version_res = (struct hbm_host_version_response *) heci_msg;

This code does rather a lot of typecasting.

Is it all _really_ necessary? The C type/struct/union system is pretty
powerful and is easily mapped onto all sorts of real-world things.

> + if (version_res->host_version_supported) {
> + dev->version.major_version = HBM_MAJOR_VERSION;
> + dev->version.minor_version = HBM_MINOR_VERSION;
> + } else {
> + dev->version = version_res->me_max_version;
> + }
> + dev->recvd_msg = 1;
> + DBG("host start response message received.\n");
> + break;
> +
> + case CLIENT_CONNECT_RES_CMD:
> + connect_res =
> + (struct hbm_client_connect_response *) heci_msg;
> + heci_client_connect_response(dev, connect_res);
> + DBG("client connect response message received.\n");
> + wake_up(&dev->wait_recvd_msg);
> + break;
> +
> + case CLIENT_DISCONNECT_RES_CMD:
> + disconnect_res =
> + (struct hbm_client_connect_response *) heci_msg;
> + heci_client_disconnect_response(dev, disconnect_res);
> + DBG("client disconnect response message received.\n");
> + wake_up(&dev->wait_recvd_msg);
> + break;
> +
> + case HECI_FLOW_CONTROL_CMD:
> + flow_control = (struct hbm_flow_control *) heci_msg;
> + heci_client_flow_control_response(dev, flow_control);
> + DBG("client flow control response message received.\n");
> + break;
> +
> + case HOST_CLIENT_PROPERTEIS_RES_CMD:
> + props_res = (struct hbm_props_response *) heci_msg;
> + if (props_res->status != 0) {
> + BUG();
> + break;
> + }
> + for (i = 0; i < dev->num_heci_me_clients; i++) {
> + if (dev->me_clients[i].client_id ==
> + props_res->address) {
> + dev->me_clients[i].props =
> + props_res->client_properties;
> + break;
> + }
> +
> + }
> + dev->recvd_msg = 1;
> + break;
> +
> + case HOST_ENUM_RES_CMD:
> + enum_res = (struct hbm_host_enum_response *) heci_msg;
> + memcpy(dev->heci_me_clients, enum_res->valid_addresses, 32);
> + dev->recvd_msg = 1;
> + break;
> +
> + case HOST_STOP_RES_CMD:
> + dev->heci_state = HECI_DISABLED;
> + DBG("reseting because of FW stop response.\n");
> + heci_reset(dev, 1);
> + break;
> +
> + case CLIENT_DISCONNECT_REQ_CMD:
> + /* search for client */
> + disconnect_req =
> + (struct hbm_client_disconnect_request *) heci_msg;
> + heci_client_disconnect_request(dev, disconnect_req);
> + break;
> +
> + case ME_STOP_REQ_CMD:
> + /* prepare stop request */
> + heci_hdr = (struct heci_msg_hdr *) &dev->ext_msg_buf[0];
> + 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;
> + h_stop_req =
> + (struct hbm_host_stop_request *) &dev->ext_msg_buf[1];
> + memset(h_stop_req, 0, sizeof(struct hbm_host_stop_request));
> + h_stop_req->cmd.cmd = HOST_STOP_REQ_CMD;
> + h_stop_req->reason = DRIVER_STOP_REQUEST;
> + h_stop_req->reserved[0] = 0;
> + h_stop_req->reserved[1] = 0;
> + dev->extra_write_index = 2;
> + break;
> +
> + default:
> + BUG();
> + break;
> +
> + }
> +}
> +
> +/**
> + * heci_bh_read_pthi_message - bottom half read routine after ISR to
> + * handle the read pthi message data processing.
> + *
> + * @complete_list: An instance of our list structure
> + * @dev: Device object for our driver
> + * @heci_hdr: header of pthi message
> + *
> + * returns 0 on success, <0 on failure.
> + */
> +static int heci_bh_read_pthi_message(struct io_heci_list *complete_list,
> + struct iamt_heci_device *dev,
> + struct heci_msg_hdr *heci_hdr)
> +{
> + struct heci_file_private *file_ext;
> + struct heci_cb_private *priv_cb;
> + unsigned char *buffer;
> +
> + BUG_ON(heci_hdr->me_addr != dev->iamthif_file_ext.me_client_id);
> + BUG_ON(dev->iamthif_state != HECI_IAMTHIF_READING);
> +
> + buffer = (unsigned char *) (dev->iamthif_msg_buf +
> + dev->iamthif_msg_buf_index);
> + BUG_ON(sizeof(dev->iamthif_msg_buf) <
> + (dev->iamthif_msg_buf_index + heci_hdr->length));
> +
> + heci_read_slots(dev, buffer, heci_hdr->length);
> +
> + dev->iamthif_msg_buf_index += heci_hdr->length;
> +
> + if (!(heci_hdr->msg_complete))
> + return 0;
> +
> + DBG("pthi_message_buffer_index=%d\n", heci_hdr->length);
> + DBG("completed pthi read.\n ");
> + if (!dev->iamthif_current_cb)
> + return -ENODEV;
> +
> + priv_cb = dev->iamthif_current_cb;
> + dev->iamthif_current_cb = NULL;
> +
> + file_ext = (struct heci_file_private *)priv_cb->file_private;
> + if (!file_ext)
> + return -ENODEV;
> +
> + dev->iamthif_stall_timer = 0;
> + priv_cb->information = dev->iamthif_msg_buf_index;
> + priv_cb->read_time = get_seconds();
> + if ((dev->iamthif_ioctl) && (file_ext == &dev->iamthif_file_ext)) {
> + /* found the iamthif cb */
> + DBG("complete the pthi read cb.\n ");
> + if (&dev->iamthif_file_ext) {
> + DBG("add the pthi read cb to complete.\n ");
> + list_add_tail(&priv_cb->cb_list,
> + &complete_list->heci_cb.cb_list);
> + }
> + }
> + return 0;
> +}

My attention span just expired, sorry.
>
> ...
>
> + dev->heci_host_clients[file_ext->host_client_id / 8] &=
> + ~(1 << (file_ext->host_client_id % 8));

__set_bit()?

>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Andrew Morton: "Re: [-mm][PATCH 1/2] mm owner fix race between swap and exit"
Previous message: Krzysztof Helt: "Re: [PATCH RESEND] agp: fix SIS 5591/5592 wrong PCI id"
In reply to: Marcin Obara: "Re: [PATCH] Intel Management Engine Interface"
Next in thread: Jiri Slaby: "Re: [PATCH] Intel Management Engine Interface"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]