Re: [PATCH] Intel Management Engine Interface

From: Jiri Slaby
Date: Tue May 20 2008 - 18:28:24 EST


On 05/20/2008 02:11 AM, Anas Nashif wrote:
We have addressed more issues raised on lkml after initial submission,
especially the legacy device support issue which was removed in this
patch.

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.
[...]
diff --git a/drivers/char/Makefile b/drivers/char/Makefile
index 5407b76..26c81a0 100644
--- a/drivers/char/Makefile
+++ b/drivers/char/Makefile
@@ -106,6 +106,7 @@ obj-$(CONFIG_IPMI_HANDLER) += ipmi/

obj-$(CONFIG_HANGCHECK_TIMER) += hangcheck-timer.o
obj-$(CONFIG_TCG_TPM) += tpm/
+obj-$(CONFIG_HECI) += heci/

Who sets this config up?


obj-$(CONFIG_PS3_FLASH) += ps3flash.o

[...]
diff --git a/drivers/char/heci/heci_init.c b/drivers/char/heci/heci_init.c
new file mode 100644
index 0000000..ffd4f7f
--- /dev/null
+++ b/drivers/char/heci/heci_init.c
@@ -0,0 +1,1104 @@
[...]
+static int heci_wait_event_int_timeout(struct iamt_heci_device *dev,
+ long timeout)
+{
+ int err = 0;
+
+ err = wait_event_interruptible_timeout(dev->wait_recvd_msg,
+ (dev->recvd_msg), timeout);
+ return err;
+}

return wait_event_interruptible_timeout();

[...]
+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];
+ 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(host_start_req));

wrong memset

+ host_start_req->cmd.cmd = HOST_START_REQ_CMD;
+ host_start_req->reserved = 0;
+ 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),
+ 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(host_stop_req));

same here

+ host_stop_req->cmd.cmd = HOST_STOP_REQ_CMD;
+ host_stop_req->reason = DRIVER_STOP_REQUEST;
+ memset(host_stop_req->reserved, 0,
+ sizeof(host_stop_req->reserved));
+ heci_write_message(dev, heci_hdr,
+ (unsigned char *) (host_stop_req),
+ heci_hdr->length);
+ DBG("version mismatch.\n");
+ return -ENODEV;
+ }
+
+ return 0;
+}
[...]
+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;
+
+ heci_check_asf_mode(dev);
+ if (dev->asf_mode) {
+ memcpy(dev->wd_data, 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, 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;

setup_timer?

+ spin_unlock_bh(&dev->device_lock);
+}
[...]
+struct heci_file_private *alloc_priv(struct file *file)

not a good name.

+{
[...]
+int heci_disconnect_host_client(struct iamt_heci_device *dev,
+ struct heci_file_private *file_ext)
+{
+ int rets = 0, err = 0;

no need to init them

+ long timeout = 15; /*15 second */
+ struct heci_cb_private *priv_cb = NULL;
[...]
diff --git a/drivers/char/heci/heci_interface.c b/drivers/char/heci/heci_interface.c
new file mode 100644
index 0000000..6369dd0
--- /dev/null
+++ b/drivers/char/heci/heci_interface.c
@@ -0,0 +1,497 @@
[...]
+int flow_ctrl_creds(struct iamt_heci_device *dev,
+ 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_ON(1);

So just BUG();

+ return 0;
+}
+
+/**
+ * flow_ctrl_reduce - reduce flow_control .
+ *
+ * @dev -Device object for our driver
+ * @file_ext -extension of the file object
+ */
+void flow_ctrl_reduce(struct iamt_heci_device *dev,
+ 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_ON(1);

ditto

+}
+
+/**
+ * heci_send_flow_control - send flow control to fw.
+ *
+ * @dev -Device object for our driver
+ * @file_ext -extension of the file object
+ *
+ * @return :
+ * 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));

again?

+ 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 = FLOW_CONTROL_CMD;
[...]
diff --git a/drivers/char/heci/heci_interface.h b/drivers/char/heci/heci_interface.h
new file mode 100644
index 0000000..4081fd3
--- /dev/null
+++ b/drivers/char/heci/heci_interface.h
@@ -0,0 +1,169 @@
[...]
+/* IOCTL commands */
+#define HECI_IOCTL_LETTER 'H'

'H' all linux/hiddev.h

add

+#define IOCTL_HECI_GET_VERSION \
+ _IOWR(HECI_IOCTL_LETTER , 0x800, struct heci_message_data)
+#define IOCTL_HECI_CONNECT_CLIENT \
+ _IOWR(HECI_IOCTL_LETTER , 0x801, struct heci_message_data)
+#define IOCTL_HECI_WD \
+ _IOWR(HECI_IOCTL_LETTER , 0x802, struct heci_message_data)
+#define IOCTL_HECI_BYPASS_WD \
+ _IOWR(HECI_IOCTL_LETTER , 0x810, struct heci_message_data)

struct heci_message_data is not 32/64-bit friendly
_IOC_NRBITS is 8, 0x800 is far higher than that

[...]
diff --git a/drivers/char/heci/heci_main.c b/drivers/char/heci/heci_main.c
new file mode 100644
index 0000000..608e1c7
--- /dev/null
+++ b/drivers/char/heci/heci_main.c
@@ -0,0 +1,1544 @@
[...]
+#ifdef HECI_DEBUG
+int heci_debug = 1;
+#else
+int heci_debug; /* initialized to 0 automatically.*/

we know that, comment unneeded

+#endif
+MODULE_PARM_DESC(heci_debug, "Debug enabled or not");

so why not bool?

+module_param(heci_debug, int, 0644);
+
+
[...]
+/* heci_pci_tbl - PCI Device ID Table */
+static struct pci_device_id heci_pci_tbl[] = {
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID01)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID02)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID03)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID04)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID05)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID06)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID07)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID08)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID09)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID10)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID11)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID12)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID13)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID14)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID15)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID16)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID17)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID18)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID19)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID20)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID21)},
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_HECI_DEVICE_ID22)},
+ /* required last entry */
+ {0, }

PCI_VDEVICE?

[...]
+static struct pci_driver heci_driver = {
+ .name = heci_driver_name,
+ .id_table = heci_pci_tbl,
+ .probe = heci_probe,
+ .remove = heci_remove,

__devexit_p()

+ .shutdown = heci_remove,

you'll trigger remove twice, I think.

+#ifdef CONFIG_PM
+ .suspend = heci_suspend,
+ .resume = heci_resume
+#endif

define them as null on !PM to avoid these ifdefs.

+};
[...]
+static int heci_registration_cdev(struct cdev *dev, int hminor,
+ struct file_operations *fops)
+{
+ int ret = 0, devno = MKDEV(heci_major, hminor);

do not init ret

+
+ cdev_init(dev, fops);
+ dev->owner = THIS_MODULE;
+ dev->ops = fops;

no need to re-set

+ ret = cdev_add(dev, devno, 1);
+ /* Fail gracefully if need be */
+ if (ret) {
+ kobject_put(&dev->kobj);

why?

+ printk(KERN_ERR "%s: Error %d registering heci device %d\n",
+ THIS_MODULE->name, 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);
+
+/**
+ * heci_sysfs_create - creates a struct class to contain heci info
+ * @owner - pointer to the module that is to "own" heci sysfs class
+ * @name - pointer to a string for the name of this class

the format is:
(* @parameterx(space)*: (description of parameter x)?)*

+ *
+ * @return :
+ * valid pointer to a struct class on success
+ * false pointer on failure
+ */
+static struct class *heci_sysfs_create(struct module *owner, char *name)
[...]
+/**
+ * heci_sysfs_device_create - adds two devices to sysfs for chararcter devices
+ * @cs - pointer to the struct class that the device to be registered on
+ *
+ * @return :
+ * 0 on success,
+ * negative on failure
+ */
+static int heci_sysfs_device_create(struct class *cs)
+{
+ int err = 0;
+
+ if ((cs == NULL) || (IS_ERR(cs))) {
+ err = -EINVAL;
+ goto err_out;
+ }
+
+ heci_class_dev = class_device_create(cs, NULL,
+ heci_cdev.dev,
+ NULL,
+ HECI_DEV_NAME);

device_create

+ if (IS_ERR(heci_class_dev))
+ err = PTR_ERR(heci_class_dev);
+
+err_out:
+ return err;
+}
[...]
+static int __init heci_init_module(void)
+{
+ int ret = 0;
+ dev_t dev;
+
+ printk(KERN_INFO "%s: %s - version %s\n",
+ THIS_MODULE->name, heci_driver_string, heci_driver_version);
+ printk(KERN_INFO "%s: %s\n", THIS_MODULE->name, heci_copyright);
+
+ /* init pci module */
+ ret = pci_register_driver(&heci_driver);
+ if (ret < 0)
+ goto end;
+
+ /* registration char devices */
+ ret = alloc_chrdev_region(&dev, HECI_MINORS_BASE, HECI_MINORS_COUNT,
+ HECI_DRIVER_NAME);

just one minor? why not misc_register?

+
+ heci_major = MAJOR(dev);
[...]
+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;
+ }
+ /* enable pci dev */
+ err = pci_enable_device(pdev);
+ if (err) {
+ printk(KERN_ERR "%s: Failed to enable pci device.\n",
+ THIS_MODULE->name);
+ 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 "%s: Failed to get pci regions.\n",
+ THIS_MODULE->name);
+ 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 = BAR_0; i <= BAR_5; i++) {
+ if (pci_resource_len(pdev, i) == 0)
+ continue;
+ if (pci_resource_flags(pdev, i) & IORESOURCE_IO) {
+ printk(KERN_ERR "%s: heci has an IO ports.\n",
+ THIS_MODULE->name);
+ goto free_device;
+ } else if (pci_resource_flags(pdev, i) & IORESOURCE_MEM) {
+ if (dev->mem_base) {
+ printk(KERN_ERR "%s: Too many mem addresses.\n",
+ THIS_MODULE->name);
+ 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 "%s: No address to use.\n", THIS_MODULE->name);
+ err = -ENODEV;
+ goto free_device;
+ }
+ dev->mem_addr = ioremap_nocache(dev->mem_base,
+ dev->mem_length);
+ if (!dev->mem_addr) {
+ printk(KERN_ERR "%s: Remap IO device memory failure.\n",
+ THIS_MODULE->name);
+ err = -ENOMEM;
+ goto free_device;
+ }
+ /* request and enable interrupt */
+ dev->irq = pdev->irq;
+ err = request_irq(dev->irq, heci_isr_interrupt, IRQF_SHARED,
+ heci_driver_name, dev);
+ if (err) {
+ printk(KERN_ERR "%s: Request_irq failure. irq = %d \n",
+ THIS_MODULE->name, dev->irq);
+ goto unmap_memory;
+ }
+
+ if (heci_hw_init(dev)) {
+ printk(KERN_ERR "%s: Init hw failure.\n", THIS_MODULE->name);

This can be built tristate, right? What if THIS_MODULE is 0.

Use dev_* prints anyway.

+ err = -ENODEV;
+ goto release_irq;
+ }
+ init_timer(&dev->wd_timer);

not needed, if heci_initialize_clients called setup_timer

+ msleep(100); /* FW needs time to be ready to talk with us */
+ heci_initialize_clients(dev);
+ if (dev->heci_state != HECI_ENABLED) {
+ err = -ENODEV;
+ goto release_hw;
+ }
[...]
+static void __devexit heci_remove(struct pci_dev *pdev)
+{
+ struct iamt_heci_device *dev = pci_get_drvdata(pdev);
+ int err = 0;
+
+ if (heci_device != pdev)
+ return;
+
+ if (dev == NULL)
+ return;
+
+ spin_lock_bh(&dev->device_lock);
+ if (dev->reinit_tsk != NULL) {
+ kthread_stop(dev->reinit_tsk);
+ dev->reinit_tsk = NULL;
+ }
+ spin_unlock_bh(&dev->device_lock);
+
+ del_timer_sync(&dev->wd_timer);
+ if (dev->wd_file_ext.state == HECI_FILE_CONNECTED
+ && dev->wd_timeout) {
+ spin_lock_bh(&dev->device_lock);
+ dev->wd_timeout = 0;
+ dev->wd_due_counter = 0;
+ memcpy(dev->wd_data, 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);

What is the retval for?

+ if (!dev->wd_stoped)
+ DBG("stop wd failed to complete.\n");
+ else
+ DBG("stop wd complete.\n");
+
+ }
+
+ heci_device = NULL;
+ if (dev->iamthif_file_ext.status == HECI_FILE_CONNECTED) {
+ dev->iamthif_file_ext.status = HECI_FILE_DISCONNECTING;
+ heci_disconnect_host_client(dev,
+ &dev->iamthif_file_ext);
+ }
+ if (dev->wd_file_ext.status == HECI_FILE_CONNECTED) {
+ dev->wd_file_ext.status = 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 */
+ dev->host_hw_state &= ~H_IE;
+ /* acknowledge interrupt and stop interupts */
+ write_heci_register(dev, H_CSR, dev->host_hw_state);

PCI posting?

+ 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);
+}
[...]
+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 = MINOR((file->f_dentry->d_inode->i_rdev));

iminor?

+ 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 = NULL;
+
+ 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);
+
+ 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) {
+ 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;
+ }
+
+ 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) {
+ 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;
+ } else {

no need to 'else' here

+ 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.
+ */
+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 = MINOR((file->f_dentry->d_inode->i_rdev));

...

+ 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 = NULL;
+ unsigned long currtime = get_seconds();
[...]
+static int heci_ioctl(struct inode *inode, struct file *file,
+ unsigned int cmd, unsigned long data)

->unlocked_ioctl

+{
+ int rets = 0;
+ int if_num = MINOR(inode->i_rdev);
+ struct heci_file_private *file_ext = file->private_data;
+ /* in user space */
+ struct heci_message_data *u_msg = (struct heci_message_data *) data;
+ struct heci_message_data k_msg; /* all in kernel on the stack */
+ struct iamt_heci_device *dev = NULL;
[...]
+#ifdef CONFIG_PM
+static int heci_suspend(struct pci_dev *pdev, pm_message_t state)
+{
+ struct iamt_heci_device *device = pci_get_drvdata(pdev);
+ int err = 0;
+
+ spin_lock_bh(&device->device_lock);
+ if (device->reinit_tsk != NULL) {
+ kthread_stop(device->reinit_tsk);
+ device->reinit_tsk = NULL;
+ }
+ spin_unlock_bh(&device->device_lock);
+
+ /* Stop watchdog if exists */
+ del_timer_sync(&device->wd_timer);
+ if (device->wd_file_ext.state == HECI_FILE_CONNECTED
+ && device->wd_timeout) {
+ spin_lock_bh(&device->device_lock);
+ g_sus_wd_timeout = device->wd_timeout;
+ device->wd_timeout = 0;
+ device->wd_due_counter = 0;
+ memcpy(device->wd_data, stop_wd_params, HECI_WD_PARAMS_SIZE);
+ device->stop = 1;
+ if (device->host_buffer_is_empty &&
+ flow_ctrl_creds(device, &device->wd_file_ext)) {
+ device->host_buffer_is_empty = 0;
+ if (!heci_send_wd(device))
+ DBG("send stop WD failed\n");
+ else
+ flow_ctrl_reduce(device, &device->wd_file_ext);
+
+ device->wd_pending = 0;
+ } else {
+ device->wd_pending = 1;
+ }
+ spin_unlock_bh(&device->device_lock);
+ device->wd_stoped = 0;
+
+ err = wait_event_interruptible_timeout(device->wait_stop_wd,
+ (device->wd_stoped),
+ 10 * HZ);
+ if (!device->wd_stoped)
+ DBG("stop wd failed to complete.\n");
+ else {
+ DBG("stop wd complete %d.\n", err);
+ err = 0;
+ }
+ }
+ /* Set new heci state */
+ spin_lock_bh(&device->device_lock);
+ if (device->heci_state == HECI_ENABLED ||
+ device->heci_state == HECI_RECOVERING_FROM_RESET) {
+ device->heci_state = HECI_POWER_DOWN;
+ heci_reset(device, 0);
+ }
+ spin_unlock_bh(&device->device_lock);
+
+ pci_save_state(pdev);
+
+ pci_disable_device(pdev);
+ free_irq(pdev->irq, device);

no device irq disabling?

+
+ pci_set_power_state(pdev, PCI_D3hot);
+
+ return err;
+}
+
+static int heci_resume(struct pci_dev *pdev)
+{
+ struct iamt_heci_device *device = NULL;
+ int err = 0;
+
+ pci_set_power_state(pdev, PCI_D0);
+ pci_restore_state(pdev);
+
+ device = pci_get_drvdata(pdev);
+ if (!device)
+ return -ENODEV;
+
+ /* request and enable interrupt */
+ device->irq = pdev->irq;

so why do you need the copy of an irq?

+ err = request_irq(device->irq, heci_isr_interrupt, IRQF_SHARED,
+ heci_driver_name, device);
+ if (err) {
+ printk(KERN_ERR "%s: Request_irq failure. irq = %d \n",
+ THIS_MODULE->name, device->irq);
+ return err;
+ }
[...]
diff --git a/drivers/char/heci/heci_version.h b/drivers/char/heci/heci_version.h
new file mode 100644
index 0000000..6cbb5e2
--- /dev/null
+++ b/drivers/char/heci/heci_version.h
@@ -0,0 +1,56 @@
[...]
+#ifndef HECI_VERSION_H
+#define HECI_VERSION_H
+
+#define MAJOR_VERSION 5
+#define MINOR_VERSION 0
+#define QUICK_FIX_NUMBER 0
+#define VER_BUILD 1
+
+#define str(s) name(s)
+#define name(s) #s
+#define DRIVER_V1 str(MAJOR_VERSION) "." str(MINOR_VERSION)
+#define DRIVER_V2 str(QUICK_FIX_NUMBER) "." str(VER_BUILD)

hmm, __stringify()

+
+#define DRIVER_VERSION DRIVER_V1 "." DRIVER_V2

not good macro names at all

+
+#endif
diff --git a/drivers/char/heci/interrupt.c b/drivers/char/heci/interrupt.c
new file mode 100644
index 0000000..c2fc1d0
--- /dev/null
+++ b/drivers/char/heci/interrupt.c
@@ -0,0 +1,1551 @@
[...]
+irqreturn_t heci_isr_interrupt(int irq, void *dev_id)
+{
+ int err;
+ struct iamt_heci_device *device = (struct iamt_heci_device *) dev_id;
+ device->host_hw_state = read_heci_register(device, H_CSR);
+
+ if ((device->host_hw_state & H_IS) != H_IS)
+ return IRQ_NONE;
+
+ /* disable interrupts */
+ device->host_hw_state &= ~H_IE;
+ /* acknowledge interrupt and stop interupts */
+ write_heci_register(device, H_CSR, device->host_hw_state);
+ /**
+ * 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(&device->work, heci_bh_handler);

this is racy with bh?

+ DBG("schedule work the heci_bh_handler.\n");
+ err = schedule_work(&device->work);
+ if (!err) {

some ratelimit?

+ printk(KERN_ERR "%s: schedule the heci_bh_handler"
+ " failed error=%x\n", THIS_MODULE->name, err);
+ }
+ return IRQ_HANDLED;
+}
[...]
+static void heci_bh_handler(struct work_struct *work)
+{
+ 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 = NULL;
+ 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);
+ write_heci_register(dev, H_CSR, dev->host_hw_state);
+ 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);
+ }
+ return;
+ }
+ } else {
+ DBG("enable interrupt FW not ready.\n");
+ dev->host_hw_state |= (H_IE);
+ write_heci_register(dev, H_CSR, dev->host_hw_state);
+ 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)) {
+ 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) {
+ PREPARE_WORK(&dev->work, heci_bh_handler);

racy with hardirq?

+ DBG("schedule work the heci_bh_handler.\n");
+ rets = schedule_work(&dev->work);
+ if (!rets) {
+ printk(KERN_ERR "%s: schedule the heci_bh_handler"
+ " failed error=%x\n", THIS_MODULE->name, rets);
+ }
[...]
+static int _heci_bh_ioctl(struct iamt_heci_device *dev, __s32 *slots,
+ struct heci_cb_private *priv_cb_pos,
+ struct heci_file_private *file_ext,
+ struct io_heci_list *cmpl_list)
+{
+ if ((*slots * sizeof(__u32)) >= (sizeof(struct heci_msg_hdr) +
+ sizeof(struct hbm_client_connect_request))) {
+ file_ext->state = HECI_FILE_CONNECTING;
+ *slots -= (sizeof(struct heci_msg_hdr) +
+ sizeof(struct hbm_client_connect_request) + 3) / 4;
+ if (!heci_connect(dev, file_ext)) {
+ file_ext->status = -ENODEV;
+ priv_cb_pos->information = 0;
+ list_del(&priv_cb_pos->cb_list);
+ return -ENODEV;
+ } else {
+ list_del(&priv_cb_pos->cb_list);
+ list_add_tail(&priv_cb_pos->cb_list,
+ &dev->ctrl_rd_list.heci_cb.cb_list);
+ file_ext->timer_count = CONNECT_TIMEOUT;
+ }
+ } else {
+ /* return the cancel routine */
+ list_del(&priv_cb_pos->cb_list);
+ return -ECORRUPTED_MESSAGE_HEADER;

Sorry, what?

+ }
+
+ return 0;
+}
+
+/**
+ * _heci_bh_cmpl: process completed and no-iamthif operation.
+ * @dev: device object.
+ * @slots: free slotsl.
+ * @priv_cb_pos: callback block.
+ * @file_ext: file extension.
+ * @cmpl_list: complete list.
+ *
+ * @return :
+ * 0, OK; otherwise, error.
+ */
+static int _heci_bh_cmpl(struct iamt_heci_device *dev, __s32 *slots,
+ struct heci_cb_private *priv_cb_pos,
+ struct heci_file_private *file_ext,
+ struct io_heci_list *cmpl_list)
+{
+ struct heci_msg_hdr *heci_hdr = NULL;
+
+ if ((*slots * sizeof(__u32)) >= (sizeof(struct heci_msg_hdr) +
+ (priv_cb_pos->request_buffer.size -
+ priv_cb_pos->information))) {
+ heci_hdr = (struct heci_msg_hdr *) &dev->wr_msg_buf[0];
+ heci_hdr->host_addr = file_ext->host_client_id;
+ heci_hdr->me_addr = file_ext->me_client_id;
+ heci_hdr->length = ((priv_cb_pos->request_buffer.size) -
+ (priv_cb_pos->information));
+ heci_hdr->msg_complete = 1;
+ heci_hdr->reserved = 0;
+ DBG("priv_cb_pos->request_buffer.size =%d"
+ "heci_hdr->msg_complete= %d\n",
+ priv_cb_pos->request_buffer.size,
+ heci_hdr->msg_complete);
+ DBG("priv_cb_pos->information =%lu\n",
+ priv_cb_pos->information);
+ DBG("heci_hdr->length =%d\n",
+ heci_hdr->length);
+ *slots -= (sizeof(struct heci_msg_hdr) +
+ heci_hdr->length + 3) / 4;
+ if (!heci_write_message(dev, heci_hdr,
+ (unsigned char *)
+ (priv_cb_pos->request_buffer.data +
+ priv_cb_pos->information),
+ heci_hdr->length)) {
+ file_ext->status = -ENODEV;
+ list_del(&priv_cb_pos->cb_list);
+ list_add_tail(&priv_cb_pos->cb_list,
+ &cmpl_list->heci_cb.cb_list);

list_move_tail?

+ return -ENODEV;
+ } else {
+ flow_ctrl_reduce(dev, file_ext);
+ file_ext->status = 0;
+ priv_cb_pos->information += heci_hdr->length;
+ list_del(&priv_cb_pos->cb_list);
+ list_add_tail(&priv_cb_pos->cb_list,
+ &dev->write_waiting_list.heci_cb.cb_list);

...

+ }
+ } else if (*slots == ((dev->host_hw_state & H_CBD) >> 24)) {
[...]
+static int same_flow_addr(struct heci_file_private *file,
+ struct hbm_flow_control *flow)
+{
+ if ((file->host_client_id == flow->host_addr)
+ && (file->me_client_id == flow->me_addr))
+ return 1;
+
+ return 0;
+}

just return <the_condition>;

+static void add_single_flow_creds(struct iamt_heci_device *dev,
+ struct hbm_flow_control *flow)
+{
+ struct heci_me_client *client = NULL;
+ int i;
+
+ for (i = 0; i < dev->num_heci_me_clients; i++) {
+ client = &dev->me_clients[i];
+ if (flow->me_addr == client->client_id) {
+ if (client->props.single_recv_buf != 0) {
+ client->flow_ctrl_creds++;
+ DBG("recv flow ctrl msg ME %d (single).\n",
+ flow->me_addr);
+ DBG("flow control credentials=%d.\n",
+ client->flow_ctrl_creds);
+ } else {
+ BUG_ON(1); /* error in flow control */

BUG()

[...]
+static int same_disconn_addr(struct heci_file_private *file,
+ struct hbm_client_disconnect_request *disconn)
+{
+ if ((file->host_client_id == disconn->host_addr)
+ && (file->me_client_id == disconn->me_addr))
+ return 1;
+
+ return 0;
+}

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