On Mon, 18 Feb 2008 08:41:26 -0500 Jaya Kumar [off-list ref] wrote:

What locking prevents `page' from being stripped off its mapping here?  y
say munmap or truncate (if it's supported here, which it presumably isn't).

ie: a race with this function?

+int load_waveform(u8 *mem, u8 *metromem, int m, int t, u8 *frame_count)

This didn't need to be a global symbol.  It would be a poorly chosen
idenfitier if it was global.

+{
+	int tta;
+	int wmta;
+	int trn = 0;
+	int i;
+	unsigned char v;
+	u8 cksum;
+	int cksum_idx;
+	int wfm_idx, owfm_idx;
+	int mem_idx = 0;
+	struct waveform_hdr *wfm_hdr;
+
+	wfm_hdr = (struct waveform_hdr *) mem;
+
+	if (wfm_hdr->fvsn != 1) {
+		printk(KERN_ERR "Error: bad fvsn %x\n", wfm_hdr->fvsn);
+		return -EINVAL;
+	}
+	if (wfm_hdr->luts != 0) {
+		printk(KERN_ERR "Error: bad luts %x\n", wfm_hdr->luts);
+		return -EINVAL;
+	}
+	cksum = calc_cksum(32, 47, mem);
+	if (cksum != wfm_hdr->wfm_cs) {
+		printk(KERN_ERR "Error: bad cksum %x != %x\n", cksum,
+					wfm_hdr->wfm_cs);
+		return -EINVAL;
+	}
+	wfm_hdr->mc += 1;
+	wfm_hdr->trc += 1;
+	for (i = 0; i < 5; i++) {
+		if (*(wfm_hdr->stuff2a + i) != 0) {
+			printk(KERN_ERR "Error: unexpected value in padding\n");
+			return -EINVAL;
+		}
+	}
+
+	/* calculating trn. trn is something used to index into
+	the waveform. presumably selecting the right one for the
+	desired temperature. it works out the offset of the first
+	v that exceeds the specified temperature */
+	for (i = sizeof(*wfm_hdr); i <= sizeof(*wfm_hdr) + wfm_hdr->trc; i++) {
+		if (mem[i] > t) {
+			trn = i - sizeof(*wfm_hdr) - 1;
+			break;
+		}
+	}
+
+	/* check temperature range table checksum */
+	cksum_idx = sizeof(*wfm_hdr) + wfm_hdr->trc + 1;
+	cksum = calc_cksum(sizeof(*wfm_hdr), cksum_idx, mem);
+	if (cksum != mem[cksum_idx]) {
+		printk(KERN_ERR "Error: bad temperature range table cksum"
+				" %x != %x\n", cksum, mem[cksum_idx]);
+		return -EINVAL;
+	}
+
+	/* check waveform mode table address checksum */
+	wmta = *((int *) wfm_hdr->wmta) & 0x00FFFFFF;
+	cksum_idx = wmta + m*4 + 3;
+	cksum = calc_cksum(cksum_idx - 3, cksum_idx, mem);
+	if (cksum != mem[cksum_idx]) {
+		printk(KERN_ERR "Error: bad mode table address cksum"
+				" %x != %x\n", cksum, mem[cksum_idx]);
+		return -EINVAL;
+	}
+
+	/* check waveform temperature table address checksum */
+	tta = *((int *) (mem + wmta + m*4)) & 0x00FFFFFF;

Does this code have any unaligned-access issues on non-x86?

+	cksum_idx = tta + trn*4 + 3;
+	cksum = calc_cksum(cksum_idx - 3, cksum_idx, mem);
+	if (cksum != mem[cksum_idx]) {
+		printk(KERN_ERR "Error: bad temperature table address cksum"
+			" %x != %x\n", cksum, mem[cksum_idx]);
+		return -EINVAL;
+	}
+
+	/* here we do the real work of putting the waveform into the
+	metromem buffer. this does runlength decoding of the waveform */
+	owfm_idx = wfm_idx = *((int *) (mem + tta + trn*4)) & 0x00FFFFFF;
+	while (1) {
+		unsigned char rl;
+		v = mem[wfm_idx++];
+		if (v == wfm_hdr->swtb) {
+			while ((v = mem[wfm_idx++]) != wfm_hdr->swtb)
+				metromem[mem_idx++] = v;
+
+			continue;
+		}
+
+		if (v == wfm_hdr->endb)
+			break;
+
+		rl = mem[wfm_idx++];
+		for (i = 0; i <= rl; i++)
+			metromem[mem_idx++] = v;
+	}
+
+	cksum_idx = wfm_idx;
+	cksum = calc_cksum(owfm_idx, cksum_idx, mem);
+	if (cksum != mem[cksum_idx]) {
+		printk(KERN_ERR "Error: bad waveform data cksum"
+				" %x != %x\n", cksum, mem[cksum_idx]);
+		return -EINVAL;
+	}
+	*frame_count = (mem_idx/64);
+
+	return 0;
+}

...

+/* this technique copied from pxafb */
+static void metronome_disable_lcd_controller(struct metronomefb_par *par)
+{
+	DECLARE_WAITQUEUE(wait, current);
+
+	set_current_state(TASK_UNINTERRUPTIBLE);
+	add_wait_queue(&par->waitq, &wait);
+
+	LCSR = 0xffffffff;	/* Clear LCD Status Register */
+	LCCR0 &= ~LCCR0_LDM;	/* Enable LCD Disable Done Interrupt */
+	LCCR0 |= LCCR0_DIS;	/* Disable LCD Controller */
+
+	schedule_timeout(200 * HZ / 1000);
+	remove_wait_queue(&par->waitq, &wait);
+}

I assume that if the schedule_timeout() actually times out, we have an
error condition?  Should it be reported?

+static void metronome_enable_lcd_controller(struct metronomefb_par *par)
+{
+	LCSR = 0xffffffff;
+	FDADR0 = par->metromem_desc_dma;
+	LCCR0 |= LCCR0_ENB;
+}

...

+static int metronome_display_cmd(struct metronomefb_par *par)
+{
+	int i;
+	u16 cs;
+	u16 opcode;
+	static u8 borderval;
+	u8 *ptr;
+
+	/* setup display command
+	we can't immediately set the opcode since the controller
+	will try parse the command before we've set it all up
+	so we just set cs here and set the opcode at the end */
+
+	ptr = par->metromem;
+
+	if (par->metromem_cmd->opcode == 0xCC40)
+		opcode = cs = 0xCC41;
+	else
+		opcode = cs = 0xCC40;
+
+	/* set the args ( 2 bytes ) for display */
+	i = 0;
+	par->metromem_cmd->args[i] = 	1 << 3 /* border update */
+					| ((borderval++ % 4) & 0x0F) << 4
+					| (par->frame_count - 1) << 8;
+	cs += par->metromem_cmd->args[i++];
+
+	/* the rest are 0 */
+	memset((u8 *) (par->metromem_cmd->args + i), 0, (32-i)*2);
+
+	par->metromem_cmd->csum = cs;
+	par->metromem_cmd->opcode = opcode; /* display cmd */
+
+	i = wait_event_interruptible_timeout(par->waitq, (GPLR1 & 0x01), HZ);
+	return i;
+}

wait_event_interruptible_timeout() will return immediately if the calling
task has a signal pending.  Will this ause the driver to malfunction?

+static int __devinit metronome_powerup_cmd(struct metronomefb_par *par)
+{
+	int i;
+	u16 cs;
+
+	/* setup power up command */
+	par->metromem_cmd->opcode = 0x1234; /* pwr up pseudo cmd */
+	cs = par->metromem_cmd->opcode;
+
+	/* set pwr1,2,3 to 1024 */
+	for (i = 0; i < 3; i++) {
+		par->metromem_cmd->args[i] = 1024;
+		cs += par->metromem_cmd->args[i];
+	}
+
+	/* the rest are 0 */
+	memset((u8 *) (par->metromem_cmd->args + i), 0, (32-i)*2);
+
+	par->metromem_cmd->csum = cs;
+
+	msleep(1);
+	metronome_set_gpio_output(RST_GPIO_PIN, 1);
+
+	msleep(1);
+	metronome_set_gpio_output(STDBY_GPIO_PIN, 1);
+
+	i = wait_event_interruptible_timeout(par->waitq, (GPLR1 & 0x01), HZ);
+	return i;
+}

Ditto

+static int __devinit metronome_config_cmd(struct metronomefb_par *par)
+{
+	int i;
+	u16 cs;
+
+	/* setup config command
+	we can't immediately set the opcode since the controller
+	will try parse the command before we've set it all up
+	so we just set cs here and set the opcode at the end */
+
+	cs = 0xCC10;
+
+	/* set the 12 args ( 8 bytes ) for config. see spec for meanings */
+	i = 0;
+	par->metromem_cmd->args[i] = 	15 /* sdlew */
+					| 2 << 8 /* sdosz */
+					| 0 << 11 /* sdor */
+					| 0 << 12 /* sdces */
+					| 0 << 15; /* sdcer */
+	cs += par->metromem_cmd->args[i++];
+
+	par->metromem_cmd->args[i] = 	42 /* gdspl */
+					| 1 << 8 /* gdr1 */
+					| 1 << 9 /* sdshr */
+					| 0 << 15; /* gdspp */
+	cs += par->metromem_cmd->args[i++];
+
+	par->metromem_cmd->args[i] = 	18 /* gdspw */
+					| 0 << 15; /* dispc */
+	cs += par->metromem_cmd->args[i++];
+
+	par->metromem_cmd->args[i] = 	599 /* vdlc */
+					| 0 << 11 /* dsi */
+					| 0 << 12; /* dsic */
+	cs += par->metromem_cmd->args[i++];
+
+	/* the rest are 0 */
+	memset((u8 *) (par->metromem_cmd->args + i), 0, (32-i)*2);
+
+	par->metromem_cmd->csum = cs;
+	par->metromem_cmd->opcode = 0xCC10; /* config cmd */
+
+	i = wait_event_interruptible_timeout(par->waitq, (GPLR1 & 0x01), HZ);
+	return i;
+}

more...

+/*
+ * this is the slow path from userspace. they can seek and write to
+ * the fb.
+ */
+static ssize_t metronomefb_write(struct fb_info *info, const char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	unsigned long p;
+	int err = -EINVAL;
+	struct metronomefb_par *par;
+	unsigned int xres;
+	unsigned int fbmemlength;
+
+	p = *ppos;
+	par = info->par;
+	xres = info->var.xres;
+	fbmemlength = (xres * info->var.yres);
+
+	if (p > fbmemlength)
+		return -ENOSPC;
+
+	err = 0;
+	if ((count + p) > fbmemlength) {
+		count = fbmemlength - p;
+		err = -ENOSPC;
+	}
+
+	if (count) {
+		char *base_addr;
+
+		base_addr = (char __force *)info->screen_base;
+		count -= copy_from_user(base_addr + p, buf, count);
+		*ppos += count;
+		err = -EFAULT;
+	}

This looks odd.  We ignore the return value from copy_from_user(), which
will wreck *ppos.  We also always return -EFAULT.

+	metronomefb_dpy_update(par);
+
+	if (count)
+		return count;
+
+	return err;
+}
+

I don't think I'll apply this one yet.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/