On Sat, 23 Jun 2007 12:52:43 +0200 Michal Januszewski [off-list ref] wrote:

These dependencies are insufficient.

Only x86 has mtrr.h: you just broke allmodconfig on all other
architectures.

+#include "edid.h"
+
+static struct cb_id uvesafb_cn_id = {
+	.idx = CN_IDX_V86D,
+	.val = CN_VAL_V86D_UVESAFB
+};
+static struct sock *nls;
+static char v86d_path[PATH_MAX] = "/sbin/v86d";

Remove the PATH_MAX, save some memory.

Oh, it gets set via sysfs.  hrm.

+static char v86d_started = 0;	/* has v86d been started by uvesafb? */

Unneeded initialisation-to-zero.

scripts/checkpatch.pl would have reported this.  It in fact generates 93
warnings against this patch and from a quick scan, they all look
legitimate.

+static void uvesafb_cn_callback(void *data)
+{
+	struct cn_msg *msg = (struct cn_msg *)data;

unneeded cast of void*

+	struct uvesafb_task *utask = (struct uvesafb_task *)msg->data;
+	struct uvesafb_ktask *task;
+
+	if (msg->seq >= UVESAFB_TASKS_MAX)
+		return;
+
+	task = uvfb_tasks[msg->seq];
+
+	if (!task || msg->ack != task->ack)
+		return;
+
+	memcpy(&task->t, utask, sizeof(struct uvesafb_task));
+
+	if (task->t.buf_len && task->buf)
+		memcpy(task->buf, ((u8*)utask) + sizeof(struct uvesafb_task),
+						task->t.buf_len);

Isn't this

	memcpy(task->buf, utask + 1, task->t.buf_len);
?

+
+	complete(task->done);
+	return;
+}
+
+static int uvesafb_helper_start(void)
+{
+	char *envp[] = {
+		"HOME=/",
+		"PATH=/sbin:/bin",
+		NULL,
+	};
+
+	char *argv[] = {
+		v86d_path,
+		NULL,
+	};
+
+	return call_usermodehelper(v86d_path, argv, envp, 1);
+}

Now I'm wondering what this code actually does.  It would be nice to have
an overall design and implementation description in the changelog so we
don't have to reverse-engineer your thoughts from your implementation...

The comment over uvesafb_exec() helps.

+static struct uvesafb_ktask *uvesafb_prep(void)
+{
+	struct uvesafb_ktask *task;
+
+	task = kzalloc(sizeof(struct uvesafb_ktask), GFP_ATOMIC);
+	if (task) {
+		task->done = kzalloc(sizeof(struct completion), GFP_ATOMIC);
+		if (!task->done) {
+			kfree(task);
+			task = NULL;
+		}
+	}
+	return task;
+}

GFP_ATOMIC is unreliable and should be strenuously avoided.  I suspect all
callers can use GFP_KERNEL here.  If not, we should at least pass the gfp_t
into this function as an argument so that we can use GFP_KERNEL from as
many callsites as possible.

+/*
+ * Execute a uvesafb task.
+ *
+ * Returns 0 if the task is executed successfully.
+ *
+ * A message sent to the userspace consists of the uvesafb_task
+ * struct and (optionally) a buffer. The uvesafb_task struct is
+ * a simplified version of uvesafb_ktask (its kernel counterpart)
+ * containing only the register values, flags and the length of
+ * the buffer.
+ *
+ * Each message is assigned a sequence number (increased linearly)
+ * and a random ack number. The sequence number is used as a key
+ * for the uvfb_tasks array which holds pointers to uvesafb_ktask
+ * structs for all requests.
+ */
+static int uvesafb_exec(struct uvesafb_ktask *task)
+{
+	static int seq = 0;
+	struct cn_msg *m;
+	int err;
+	int len = sizeof(task->t) + task->t.buf_len;
+
+	/* Check whether the message isn't longer than the maximum
+	 * allowed by connector */
+	if (sizeof(*m) + len > CONNECTOR_MAX_MSG_SIZE) {
+		printk(KERN_WARNING "uvesafb: message too long (%d), "
+				"can't exec task\n", (int)(sizeof(*m) + len));
+		return -E2BIG;
+	}
+
+	m = kmalloc(sizeof(*m) + len, GFP_ATOMIC);

More GFP_ATOMIC badness

+	if (!m)
+		return -ENOMEM;
+
+	init_completion(task->done);
+
+	memset(m, 0, sizeof(*m) + len);

kzalloc()

+	memcpy(&m->id, &uvesafb_cn_id, sizeof(m->id));
+	m->seq = seq;
+	m->len = len;
+	m->ack = random32();
+
+	/* uvesafb_task structure */
+	memcpy(m + 1, &task->t, sizeof(task->t));
+
+	/* Buffer */
+	memcpy((u8*)m + sizeof(task->t) + sizeof(*m),
+		task->buf, task->t.buf_len);
+
+	/* Save the message ack number so that we can find the kernel
+	 * part of this task when a reply is received from userspace. */
+	task->ack = m->ack;
+
+	/* If all slots are taken -- bail out. */
+	if (uvfb_tasks[seq])
+		return -EBUSY;
+
+	/* Save a pointer to the kernel part of the task struct. */
+	uvfb_tasks[seq] = task;
+
+	err = cn_netlink_send(m, 0, gfp_any());
+	if (err == -ESRCH) {
+		/* Try to start the userspace helper if sending
+		 * the request failed the first time. */
+		err = uvesafb_helper_start();
+		if (err) {
+			printk(KERN_ERR "uvesafb: failed to execute %s\n",
+					v86d_path);
+			printk(KERN_ERR "uvesafb: make sure that the v86d"
+					"helper is installed and executable\n");
+		} else {
+			v86d_started = 1;
+			err = cn_netlink_send(m, 0, gfp_any());
+		}
+	}
+	kfree(m);
+
+	if (!err && !(task->t.flags & TF_EXIT))
+		err = !wait_for_completion_timeout(task->done,
+				msecs_to_jiffies(UVESAFB_TIMEOUT));

If you can run wait_for_completion() here then you can use GFP_KERNEL up
there.

+	uvfb_tasks[seq] = NULL;
+
+	seq++;
+	if (seq >= UVESAFB_TASKS_MAX)
+		seq = 0;
+
+	return err;
+}

...

+static int __devinit uvesafb_vbe_getinfo(struct uvesafb_ktask *task,
+	struct uvesafb_par *par)
+{
+	int err;
+
+	task->t.regs.eax = 0x4f00;
+	task->t.flags = TF_VBEIB;
+	task->t.buf_len = sizeof(struct vbe_ib);
+	task->buf = (u8*) &par->vbe_ib;
+	strncpy(par->vbe_ib.vbe_signature, "VBE2", 4);
+
+	err = uvesafb_exec(task);
+	if (err || (task->t.regs.eax & 0xffff) != 0x004f) {
+		printk(KERN_ERR "uvesafb: Getting VBE info block failed "
+				"(eax=0x%x, err=%x)\n", (u32)task->t.regs.eax,
+				err);
+		return -EINVAL;
+	}
+
+	if (par->vbe_ib.vbe_version < 0x0200) {
+		printk(KERN_ERR "uvesafb: Sorry, pre-VBE 2.0 cards are "
+				"not supported.\n");
+		return -EINVAL;
+	}
+
+	if (!par->vbe_ib.mode_list_ptr) {
+	    printk(KERN_ERR "uvesafb: Missing mode list!\n");
+	    return -EINVAL;

whitespace went wonky here.

+	}
+
+	printk(KERN_INFO "uvesafb: ");
+
+	/* Convert string pointers and the mode list pointer into
+	 * usable addresses. Print informational messages about the
+	 * video adapter and its vendor. */

Commenting style is

	/* Convert string pointers and the mode list pointer into
	 * usable addresses. Print informational messages about the
	 * video adapter and its vendor.
	 */

or

	/*
	 * Convert string pointers and the mode list pointer into
	 * usable addresses. Print informational messages about the
	 * video adapter and its vendor.
	 */

+	if (par->vbe_ib.oem_vendor_name_ptr)
+		printk("%s, ",
+			(char*)(task->buf + par->vbe_ib.oem_vendor_name_ptr));
+
+	if (par->vbe_ib.oem_product_name_ptr)
+		printk("%s, ",
+			(char*)(task->buf + par->vbe_ib.oem_product_name_ptr));
+
+	if (par->vbe_ib.oem_product_rev_ptr)
+		printk("%s, ",
+			(char*)(task->buf + par->vbe_ib.oem_product_rev_ptr));
+
+	if (par->vbe_ib.oem_string_ptr)
+		printk("OEM: %s, ",
+			(char*)(task->buf + par->vbe_ib.oem_string_ptr));
+
+	printk("VBE v%d.%d\n", ((par->vbe_ib.vbe_version & 0xff00) >> 8),
+			par->vbe_ib.vbe_version & 0xff);
+
+	return 0;
+}
+
+static int __devinit uvesafb_vbe_getmodes(struct uvesafb_ktask *task,
+		struct uvesafb_par *par)
+{
+	int off = 0, err;
+	u16 *mode;
+
+	par->vbe_modes_cnt = 0;
+
+	/* Count available modes. */
+	mode = (u16*) (((u8*)&par->vbe_ib) + par->vbe_ib.mode_list_ptr);
+	while (*mode != 0xffff) {
+		par->vbe_modes_cnt++;
+		mode++;
+	}
+
+	par->vbe_modes = kzalloc(sizeof(struct vbe_mode_ib) *
+				par->vbe_modes_cnt, GFP_KERNEL);
+	if (!par->vbe_modes)
+		return -ENOMEM;
+
+	/* Get info about all available modes. */
+	mode = (u16*) (((u8*)&par->vbe_ib) + par->vbe_ib.mode_list_ptr);
+	while (*mode != 0xffff) {
+		struct vbe_mode_ib *mib;
+
+		uvesafb_reset(task);
+		task->t.regs.eax = 0x4f01;
+		task->t.regs.ecx = (u32) *mode;
+		task->t.flags = TF_BUF_RET | TF_BUF_ESDI;
+		task->t.buf_len = sizeof(struct vbe_mode_ib);
+		task->buf = (u8*) (par->vbe_modes + off);
+
+		err = uvesafb_exec(task);
+		if (err || (task->t.regs.eax & 0xffff) != 0x004f) {
+			printk(KERN_ERR "uvesafb: Getting mode info block "
+				"for mode 0x%x failed (eax=0x%x, err=%x)\n",
+				*mode, (u32)task->t.regs.eax, err);
+			return -EINVAL;
+		}
+
+		mib = (struct vbe_mode_ib*)task->buf;

Perhaps uvesafb_ktask.buf should be void* so we can lose some typecasts.

+		mib->mode_id = *mode;
+
+		/* We only want modes that are supported with the current
+		 * hardware configuration, color, graphics and that have
+		 * support for the LFB. */
+		if ((mib->mode_attr & VBE_MODE_MASK) == VBE_MODE_MASK &&
+		     mib->bits_per_pixel >= 8) {
+			off++;
+		} else {
+			par->vbe_modes_cnt--;
+		}
+		mode++;
+		mib->depth = mib->red_len + mib->green_len + mib->blue_len;
+
+		/* Handle 8bpp modes and modes with broken color component
+		 * lengths. */
+		if (mib->depth == 0 || (mib->depth == 24 &&
+					mib->bits_per_pixel == 32))
+			mib->depth = mib->bits_per_pixel;
+	}
+
+	return 0;
+}
+
+#ifdef __i386__

CONFIG_X86 would be more typical.

Be aware that CONFIG_X86 is true for both i386 and x86_64.  You don't state
whether this code works on x86_64.  If it can, it should.

+static int __devinit uvesafb_vbe_getpmi(struct uvesafb_ktask *task,
+		struct uvesafb_par *par)
+{
+	int i, err;
+
+	uvesafb_reset(task);
+	task->t.regs.eax = 0x4f0a;
+	task->t.regs.ebx = 0x0;
+	err = uvesafb_exec(task);
+
+	if ((task->t.regs.eax & 0xffff) != 0x4f || task->t.regs.es < 0xc000) {
+		par->pmi_setpal = par->ypan = 0;
+	} else {
+		par->pmi_base = (u16*)phys_to_virt(((u32)task->t.regs.es << 4)
+						+ task->t.regs.edi);
+		par->pmi_start = (void*)((char*)par->pmi_base +
+						par->pmi_base[1]);
+		par->pmi_pal = (void*)((char*)par->pmi_base +
+						par->pmi_base[2]);
+		printk(KERN_INFO "uvesafb: protected mode interface info at "
+				 "%04x:%04x\n",
+				 (u16)task->t.regs.es, (u16)task->t.regs.edi);
+		printk(KERN_INFO "uvesafb: pmi: set display start = %p, "
+				 "set palette = %p\n", par->pmi_start,
+				 par->pmi_pal);
+
+		if (par->pmi_base[3]) {
+			printk(KERN_INFO "uvesafb: pmi: ports = ");
+			for (i = par->pmi_base[3]/2;
+					par->pmi_base[i] != 0xffff; i++)
+				printk("%x ", par->pmi_base[i]);
+			printk("\n");
+
+			if (par->pmi_base[i] != 0xffff) {
+				printk(KERN_INFO "uvesafb: can't handle memory"
+						 " requests, pmi disabled\n");
+				par->ypan = par->pmi_setpal = 0;
+			}
+		}
+	}
+	return 0;
+}
+#endif

You might care to cc "H.  Peter Anvin" [off-list ref] on the next version
of this patch - he's a whizz at this sort of low-level x86 bios
communication stuff.

+/* Check whether a video mode is supported by the Video BIOS and is
+ * compatible with the monitor limits. */
+static int __devinit uvesafb_is_valid_mode(struct fb_videomode *mode,
+		struct fb_info *info)
+{
+	if (info->monspecs.gtf) {
+		fb_videomode_to_var(&info->var, mode);
+		if (fb_validate_mode(&info->var, info))
+			return -1;
+	}
+
+	if (uvesafb_vbe_find_mode(info->par, mode->xres, mode->yres, 8,
+				UVESAFB_NEED_EXACT_RES) == -1)
+		return -1;
+
+	return 0;
+}
+
+static int __devinit uvesafb_vbe_getedid(struct uvesafb_ktask *task,
+		struct fb_info *info)
+{
+	struct uvesafb_par *par = (struct uvesafb_par *)info->par;

fb_info.par has type void* hence all casts such as the above can and should
be removed.

+	int err = 0;
+
+	if (noedid || par->vbe_ib.vbe_version < 0x0300)
+		return -EINVAL;
+
+	task->t.regs.eax = 0x4f15;
+	task->t.regs.ebx = 0;
+	task->t.regs.ecx = 0;
+	task->t.buf_len = 0;
+	task->t.flags = 0;
+
+	err = uvesafb_exec(task);
+
+	if ((task->t.regs.eax & 0xffff) != 0x004f || err)
+		return -EINVAL;
+
+	if ((task->t.regs.ebx & 0x3) == 3) {
+		printk(KERN_INFO "uvesafb: VBIOS/hardware supports both "
+				 "DDC1 and DDC2 transfers\n");
+	} else if ((task->t.regs.ebx & 0x3) == 2) {
+		printk(KERN_INFO "uvesafb: VBIOS/hardware supports DDC2 "
+				 "transfers\n");
+	} else if ((task->t.regs.ebx & 0x3) == 1) {
+		printk(KERN_INFO "uvesafb: VBIOS/hardware supports DDC1 "
+				 "transfers\n");
+	} else {
+		printk(KERN_INFO "uvesafb: VBIOS/hardware doesn't support "
+				 "DDC transfers\n");
+		return -EINVAL;
+	}
+
+	task->t.regs.eax = 0x4f15;
+	task->t.regs.ebx = 1;
+	task->t.regs.ecx = task->t.regs.edx = 0;
+	task->t.flags = TF_BUF_RET | TF_BUF_ESDI;
+	task->t.buf_len = EDID_LENGTH;
+	task->buf = kzalloc(EDID_LENGTH, GFP_KERNEL);
+
+	err = uvesafb_exec(task);
+
+	if ((task->t.regs.eax & 0xffff) == 0x004f && !err) {
+		fb_edid_to_monspecs(task->buf, &info->monspecs);
+
+		if (info->monspecs.vfmax && info->monspecs.hfmax) {
+			/* If the maximum pixel clock wasn't specified in
+			 * the EDID block, set it to 300 MHz. */
+			if (info->monspecs.dclkmax == 0)
+				info->monspecs.dclkmax = 300 * 1000000;
+			info->monspecs.gtf = 1;
+		}
+	} else {
+		err = -EINVAL;
+	}
+
+	kfree(task->buf);
+	return err;
+}
+
+static void __devinit uvesafb_vbe_getmonspecs(struct uvesafb_ktask *task,
+		struct fb_info *info)
+{
+	struct uvesafb_par *par = info->par;
+	int i;
+	memset(&info->monspecs, 0, sizeof(struct fb_monspecs));

Missing a newline above.

+	/* If we don't get all necessary data from the EDID block,
+	 * mark it as incompatible with the GTF. */
+	if (uvesafb_vbe_getedid(task, info))
+		info->monspecs.gtf = 0;
+
+	/* Kernel command line overrides. */
+	if (maxclk)
+		info->monspecs.dclkmax = maxclk * 1000000;
+	if (maxvf)
+		info->monspecs.vfmax = maxvf;
+	if (maxhf)
+		info->monspecs.hfmax = maxhf * 1000;
+
+	/* In case DDC transfers are not supported the user can provide
+	 * monitor limits manually. Lower limits are set to "safe" values. */
+	if (info->monspecs.gtf == 0 && maxclk && maxvf && maxhf) {
+		info->monspecs.dclkmin = 0;
+		info->monspecs.vfmin = 60;
+		info->monspecs.hfmin = 29000;
+		info->monspecs.gtf = 1;
+	}
+
+	if (info->monspecs.gtf)
+		printk(KERN_INFO
+			"uvesafb: monitor limits: vf = %d Hz, hf = %d kHz, "
+			"clk = %d MHz\n", info->monspecs.vfmax,
+			(int)(info->monspecs.hfmax / 1000),
+			(int)(info->monspecs.dclkmax / 1000000));
+	else
+		printk(KERN_INFO "uvesafb: no monitor limits have been set\n");
+
+	/* Add VBE modes to the modelist. */
+	for (i = 0; i < par->vbe_modes_cnt; i++) {
+		struct fb_var_screeninfo var;
+		struct vbe_mode_ib *mode;
+		struct fb_videomode vmode;
+
+		mode = &par->vbe_modes[i];
+		memset(&var, 0, sizeof(var));
+
+		var.xres = mode->x_res;
+		var.yres = mode->y_res;
+
+		fb_get_mode(FB_VSYNCTIMINGS | FB_IGNOREMON, 60, &var, info);
+		fb_var_to_videomode(&vmode, &var);
+		fb_add_videomode(&vmode, &info->modelist);
+	}
+
+	/* Add valid VESA modes to our modelist. */
+	for (i = 0; i < VESA_MODEDB_SIZE; i++) {
+		if (!uvesafb_is_valid_mode((struct fb_videomode*)
+						&vesa_modes[i], info))
+			fb_add_videomode(&vesa_modes[i], &info->modelist);
+	}
+
+	for (i = 0; i < info->monspecs.modedb_len; i++) {
+		if (!uvesafb_is_valid_mode(&info->monspecs.modedb[i], info))
+			fb_add_videomode(&info->monspecs.modedb[i],
+					&info->modelist);
+	}
+
+	return;
+}
+
+static void __devinit uvesafb_vbe_getstatesize(struct uvesafb_ktask *task,
+		struct uvesafb_par *par)
+{
+	int err;
+	uvesafb_reset(task);

Here also.

+	/* Get the VBE state buffer size. We want all available
+	 * hardware state data (CL = 0x0f). */
+	task->t.regs.eax = 0x4f04;
+	task->t.regs.ecx = 0x000f;
+	task->t.regs.edx = 0x0000;
+	task->t.flags = 0;
+
+	err = uvesafb_exec(task);
+
+	if (err || (task->t.regs.eax & 0xffff) != 0x004f) {
+		printk(KERN_WARNING "uvesafb: VBE state buffer size "
+			"cannot be determined (eax=0x%x, err=%d)\n",
+			task->t.regs.eax, err);
+		par->vbe_state_size = 0;
+		return;
+	}
+
+	par->vbe_state_size = 64 * (task->t.regs.ebx & 0xffff);
+}
+
+static int __devinit uvesafb_vbe_init(struct fb_info *info)
+{
+	struct uvesafb_ktask *task = NULL;
+	struct uvesafb_par *par = info->par;
+	int err;
+
+	task = uvesafb_prep();
+	if (!task)
+		return -ENOMEM;
+
+	if ((err = uvesafb_vbe_getinfo(task, par)))

checkpatch.pl will do my work here..

+		goto out;
+	if ((err = uvesafb_vbe_getmodes(task, par)))
+		goto out;
+	par->nocrtc = nocrtc;
+#ifdef __i386__
+	par->pmi_setpal = pmi_setpal;
+	par->ypan = ypan;
+
+	if (par->pmi_setpal || par->ypan)
+		uvesafb_vbe_getpmi(task, par);
+#else
+	/* The protected mode interface is not available on non-x86. */
+	par->pmi_setpal = par->ypan = 0;
+#endif
+
+	INIT_LIST_HEAD(&info->modelist);
+	uvesafb_vbe_getmonspecs(task, info);
+	uvesafb_vbe_getstatesize(task, par);
+
+out:	uvesafb_free(task);
+	return err;
+}
+
+static int __devinit uvesafb_vbe_init_mode(struct fb_info *info)
+{
+	struct list_head *pos;
+	struct fb_modelist *modelist;
+	struct fb_videomode *mode;
+	struct uvesafb_par *par = info->par;
+	int i, modeid;
+
+	/* Has the user requested a specific VESA mode? */
+	if (vbemode) {
+		for (i = 0; i < par->vbe_modes_cnt; i++) {
+			if (par->vbe_modes[i].mode_id == vbemode) {
+				fb_get_mode(FB_VSYNCTIMINGS | FB_IGNOREMON, 60,
+							&info->var, info);
+				/* With pixclock set to 0, the default BIOS
+				 * timings will be used in set_par(). */
+				info->var.pixclock = 0;
+				modeid = i;
+				goto gotmode;
+			}
+		}
+		printk(KERN_INFO "uvesafb: requested VBE mode 0x%x is "
+				 "unavailable\n", vbemode);
+		vbemode = 0;
+	}
+
+	i = 0;
+	list_for_each(pos, &info->modelist) {
+		i++;
+	}

Unneeded braces

+	mode = kzalloc(i * sizeof(*mode), GFP_KERNEL);

Check for and handle the allocation failure, please.

+	i = 0;
+	list_for_each(pos, &info->modelist) {
+		modelist = list_entry(pos, struct fb_modelist, list);
+		mode[i] = modelist->mode;
+		i++;
+	}
+
+	if (!mode_option)
+		mode_option = UVESAFB_DEFAULT_MODE;
+
+	i = fb_find_mode(&info->var, info, mode_option, mode, i,
+		NULL, 8);
+
+	kfree(mode);
+
+	/* fb_find_mode() failed */
+	if (i == 0 || i >= 3) {
+		info->var.xres = 640;
+		info->var.yres = 480;
+		mode = (struct fb_videomode*)
+				fb_find_best_mode(&info->var, &info->modelist);
+
+		if (mode) {
+			fb_videomode_to_var(&info->var, mode);
+		} else {
+			modeid = par->vbe_modes[0].mode_id;
+			fb_get_mode(FB_VSYNCTIMINGS | FB_IGNOREMON, 60,
+				    &info->var, info);
+			goto gotmode;
+		}
+	}
+
+	/* Look for a matching VBE mode. */
+	modeid = uvesafb_vbe_find_mode(par, info->var.xres, info->var.yres,
+			info->var.bits_per_pixel, UVESAFB_NEED_EXACT_RES);
+
+	if (modeid == -1)
+		return -EINVAL;
+
+gotmode:
+	uvesafb_setup_var(&info->var, info, &par->vbe_modes[modeid]);
+
+	/* If we are not VBE3.0+ compliant, we're done -- the BIOS will
+	 * ignore our mode timings anyway. */
+	if (par->vbe_ib.vbe_version < 0x0300)
+		fb_get_mode(FB_VSYNCTIMINGS | FB_IGNOREMON, 60,
+					&info->var, info);
+
+	return modeid;
+}
+
+static int uvesafb_setpalette(struct uvesafb_pal_entry *entries, int count,
+			     int start, struct fb_info *info)
+{
+	struct uvesafb_ktask *task;
+	struct uvesafb_par *par = info->par;
+	int i = par->mode_idx;
+	int err = 0;
+
+	/* We support palette modifications for 8 bpp modes only, so
+	 * there can never be more than 256 entries. */
+	if (start + count > 256)
+		return -EINVAL;
+
+	/* Use VGA registers if mode is VGA-compatible. */
+	if (i >= 0 && i < par->vbe_modes_cnt &&
+	    par->vbe_modes[i].mode_attr & VBE_MODE_VGACOMPAT) {
+		for (i = 0; i < count; i++) {
+			outb_p(start + i,        dac_reg);
+			outb_p(entries[i].red,   dac_val);
+			outb_p(entries[i].green, dac_val);
+			outb_p(entries[i].blue,  dac_val);
+		}
+	} else if (par->pmi_setpal) {
+		__asm__ __volatile__(
+		"call *(%%esi)"
+		: /* no return value */
+		: "a" (0x4f09),         /* EAX */
+		  "b" (0),              /* EBX */
+		  "c" (count),          /* ECX */
+		  "d" (start),          /* EDX */
+		  "D" (entries),        /* EDI */
+		  "S" (&par->pmi_pal)); /* ESI */

uh, so we're CONFIG_X86_32-only, yes?

+	} else {
+		task = uvesafb_prep();
+		if (!task)
+			return -ENOMEM;
+
+		task->t.regs.eax = 0x4f09;
+		task->t.regs.ebx = 0x0;
+		task->t.regs.ecx = count;
+		task->t.regs.edx = start;
+		task->t.flags = TF_BUF_ESDI;
+		task->t.buf_len = sizeof(struct uvesafb_pal_entry) * count;
+		task->buf = (u8*) entries;
+
+		err = uvesafb_exec(task);
+		if ((task->t.regs.eax & 0xffff) != 0x004f)
+			err = 1;
+
+		uvesafb_free(task);
+	}
+	return err;
+}
+

...

+
+static struct device_attribute device_attrs[] = {
+	__ATTR(vbe_version, S_IRUGO, uvesafb_show_vbe_ver, NULL),
+	__ATTR(vbe_modes, S_IRUGO, uvesafb_show_vbe_modes, NULL),
+	__ATTR(oem_vendor, S_IRUGO, uvesafb_show_vendor, NULL),
+	__ATTR(oem_product_name, S_IRUGO, uvesafb_show_product_name, NULL),
+	__ATTR(oem_product_rev, S_IRUGO, uvesafb_show_product_rev, NULL),
+	__ATTR(oem_string, S_IRUGO, uvesafb_show_oem_string, NULL),
+	__ATTR(nocrtc, S_IRUGO | S_IWUSR, uvesafb_show_nocrtc,
+		uvesafb_store_nocrtc),
+};

Can we use an attribute group here?

+static void __devinit uvesafb_create_sysfs(struct device *dev,
+		struct fb_info *info)
+{
+	int i, err = 0;
+
+	for (i = 0; i < ARRAY_SIZE(device_attrs); i++) {
+		err |= device_create_file(dev, &device_attrs[i]);
+	}

Unneeded braces

+	if (err != 0)
+		printk(KERN_WARNING "fb%d: failed to register attributes\n",
+			info->node);
+}
+
+static void __devinit uvesafb_remove_sysfs(struct device *dev)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(device_attrs); i++) {
+		device_remove_file(dev, &device_attrs[i]);
+	}

ditto

+}
+
+static int __devinit uvesafb_probe(struct platform_device *dev)
+{
+	struct fb_info *info;
+	struct vbe_mode_ib *mode = NULL;
+	struct uvesafb_par *par;
+	int err = 0, i;
+
+	info = framebuffer_alloc(sizeof(*par) +	sizeof(u32) * 256, &dev->dev);
+	if (!info)
+		return -ENOMEM;
+
+	par = info->par;
+
+	if ((err = uvesafb_vbe_init(info))) {
+		printk(KERN_ERR "uvesafb: vbe_init() failed with %d\n", err);
+		goto out;
+	}
+
+	info->fbops = &uvesafb_ops;
+
+	i = uvesafb_vbe_init_mode(info);
+	if (i < 0) {
+		err = -EINVAL;
+		goto out;
+	} else {
+		mode = &par->vbe_modes[i];
+	}
+
+	if (fb_alloc_cmap(&info->cmap, 256, 0) < 0) {
+		err = -ENXIO;
+		goto out;
+	}
+
+	uvesafb_init_info(info, mode);
+
+	if (!request_mem_region(info->fix.smem_start, info->fix.smem_len,
+	    "uvesafb")) {
+		printk(KERN_WARNING "uvesafb: cannot reserve video memory at "
+		       "0x%lx\n", info->fix.smem_start);
+		/* We cannot make this fatal. Sometimes this comes from magic
+		   spaces our resource handlers simply don't know about. */

so...  what happens?  The driver starts altering mrmory regions which it
doesn't own?

+	}
+
+	info->screen_base = ioremap(info->fix.smem_start, info->fix.smem_len);
+
+	if (!info->screen_base) {
+		printk(KERN_ERR
+		       "uvesafb: abort, cannot ioremap 0x%x bytes of video "
+		       "memory at 0x%lx\n",
+		       info->fix.smem_len, info->fix.smem_start);
+		err = -EIO;
+		goto out_mem;
+	}
+
+	if (!request_region(0x3c0, 32, "uvesafb")) {
+		printk(KERN_ERR "uvesafb: request region 0x3c0-0x3e0 failed\n");
+		err = -EIO;
+		goto out_unmap;
+	}
+
+	uvesafb_init_mtrr(info);
+	platform_set_drvdata(dev, info);
+
+	if (register_framebuffer(info) < 0) {
+		printk(KERN_ERR
+		       "uvesafb: failed to register framebuffer device\n");
+		err = -EINVAL;
+		goto out_reg;
+	}
+
+	printk(KERN_INFO "uvesafb: framebuffer at 0x%lx, mapped to 0x%p, "
+		       "using %dk, total %dk\n", info->fix.smem_start,
+		       info->screen_base, info->fix.smem_len/1024,
+		       par->vbe_ib.total_memory * 64);
+	printk(KERN_INFO "fb%d: %s frame buffer device\n", info->node,
+		       info->fix.id);
+
+	uvesafb_create_sysfs(&dev->dev, info);
+	return 0;
+
+out_reg:
+	release_region(0x3c0, 32);
+out_unmap:
+	iounmap(info->screen_base);
+out_mem:
+	release_mem_region(info->fix.smem_start, info->fix.smem_len);
+	if (!list_empty(&info->modelist))
+		fb_destroy_modelist(&info->modelist);
+	fb_destroy_modedb(info->monspecs.modedb);
+	fb_dealloc_cmap(&info->cmap);
+out:
+	if (par->vbe_modes)
+		kfree(par->vbe_modes);
+
+	framebuffer_release(info);
+	return err;
+}

...

+#ifndef MODULE
+static int __devinit uvesafb_setup(char *options)
+{
+	char *this_opt;
+
+	if (!options || !*options)
+		return 0;
+
+	while ((this_opt = strsep(&options, ",")) != NULL) {
+		if (!*this_opt) continue;
+
+		if (!strcmp(this_opt, "redraw"))
+			ypan = 0;
+		else if (!strcmp(this_opt, "ypan"))
+			ypan = 1;
+		else if (!strcmp(this_opt, "ywrap"))
+			ypan = 2;
+		else if (!strcmp(this_opt, "vgapal"))
+			pmi_setpal = 0;
+		else if (!strcmp(this_opt, "pmipal"))
+			pmi_setpal = 1;
+		else if (!strncmp(this_opt, "mtrr:", 5))
+			mtrr = simple_strtoul(this_opt+5, NULL, 0);
+		else if (!strcmp(this_opt, "nomtrr"))
+			mtrr = 0;
+		else if (!strcmp(this_opt, "nocrtc"))
+			nocrtc = 1;
+		else if (!strcmp(this_opt, "noedid"))
+			noedid = 1;
+		else if (!strcmp(this_opt, "noblank"))
+			blank = 0;
+		else if (!strncmp(this_opt, "vtotal:", 7))
+			vram_total = simple_strtoul(this_opt + 7, NULL, 0);
+		else if (!strncmp(this_opt, "vremap:", 7))
+			vram_remap = simple_strtoul(this_opt + 7, NULL, 0);
+		else if (!strncmp(this_opt, "maxhf:", 6))
+			maxhf = simple_strtoul(this_opt + 6, NULL, 0);
+		else if (!strncmp(this_opt, "maxvf:", 6))
+			maxvf = simple_strtoul(this_opt + 6, NULL, 0);
+		else if (!strncmp(this_opt, "maxclk:", 7))
+			maxclk = simple_strtoul(this_opt + 7, NULL, 0);
+		else if (!strncmp(this_opt, "vbemode:", 8))
+			vbemode = simple_strtoul(this_opt + 8, NULL,0);
+		else if (this_opt[0] >= '0' && this_opt[0] <= '9') {
+			mode_option = this_opt;
+		} else {
+			printk(KERN_WARNING
+			       "uvesafb: unrecognized option %s\n", this_opt);
+		}
+	}
+
+	return 0;
+}
+#endif /* !MODULE */

The #ifdef MODULE is unpleasing.  Can we use module_param() here?  That'll
work for both modular and non-modular case.

+#define uvesafb_free(task)					\
+do {								\
+	if (task) {						\
+		if (task->done)					\
+			kfree(task->done);			\
+		kfree(task);					\
+		task = NULL;					\
+	}							\
+} while(0)

I don't think this need to be a macro?  Code should be written in C unless
there is some reason why it _must_ be a macro.

Oh, it sneakily zeroes a variable within the caller's context.  Ow.  Can
we not do that please?  Someone who is reading this code will see a call to
a "function" called uvesafb_free() and the last thing they will expect is
that "function" magically zeroed out a local variable.

+#define uvesafb_reset(task)					\
+do {								\
+	struct completion *cpl = task->done;			\
+	memset(task, 0, sizeof(struct uvesafb_ktask));		\
+	task->done = cpl;					\
+} while(0)							\

This can be a regular C function.

+static int uvesafb_exec(struct uvesafb_ktask *tsk);
+
+#define UVESAFB_NEED_EXACT_RES		1
+#define UVESAFB_NEED_EXACT_DEPTH	2
+
+struct uvesafb_par {
+	struct vbe_ib vbe_ib;		/* VBE Info Block */
+	struct vbe_mode_ib *vbe_modes;	/* list of supported VBE modes */
+	int vbe_modes_cnt;
+
+	u8 nocrtc;
+	u8 ypan;			/* 0 - nothing, 1 - ypan, 2 - ywrap */
+	u8 pmi_setpal;			/* pmi for palette changes */
+	u16  *pmi_base;			/* protected mode interface location */
+	void (*pmi_start)(void);
+	void (*pmi_pal)(void);
+
+	u8 *vbe_state_orig;		/* original hardware state, before the driver was loaded */
+	u8 *vbe_state_saved;		/* state saved by fb_save_state */
+	int vbe_state_size;
+	atomic_t ref_count;
+
+	int mode_idx;
+	struct vbe_crtc_ib crtc;
+};
+
+#endif

A comment on the endif would be nice.

+#endif /* _UVESAFB_H */

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/