Messages in this thread | | | Date | Tue, 12 Aug 2008 18:15:24 +0200 | From | Jiri Slaby <> | Subject | Re: [PATCH] Intel Management Engine Interface |
| |
On 08/12/2008 06:53 AM, Andrew Morton wrote: > On Mon, 11 Aug 2008 21:23:01 +0200 (CEST) > Marcin Obara <marcin_obara@users.sourceforge.net> 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?
Yes and already pointed out. This leaks to userspace and it's wrong. Please go and read my comments to version #1 once again and if you don't understand anything, please drop a message, but do not silently ignore others' comments.
E.g. unlocked_ioctl switch hasn't been done plus other things mentioned below too (not all of them)
>> +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.
Already commented, left unchanged and without an explanation.
>> +/* 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.
... and conflicts with hid as I commented before. Also ioctl-number.txt update should be done.
>> + 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.
Hm, no, pci_enable_device sets up irq (i.e. even pdev->irq), this is correct sequence. Actually one should not touch pdev before enabling the device it's describing in most cases.
| |