On Mon, 11 Aug 2008 21:23:01 +0200 (CEST)
Marcin Obara <marcin_obara@xxxxxxxxxxxxxxxxxxxxx> wrote:
+/*
+ * 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?
+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.
+/* 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.
+ 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.