Re: [PATCH] Intel Management Engine Interface

[Jiri Slaby]

On 08/12/2008 06:53 AM, Andrew Morton wrote:
> 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?

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