Re: lcd: fix memory leak, code cleanup

From: Jim Nelson
Date: Tue Dec 21 2004 - 22:47:16 EST

Domen Puncer wrote:
On 20/12/04 19:50 -0600, James Nelson wrote:

This patch addresses the following issues:

Fix log-spamming and cryptic error messages, and add KERN_ constants.
Convert some ints to unsigned ints.
Add checks for CAP_SYS_ADMIN for FLASH_Burn and FLASH_Erase ioctls.
Identify use of global variable.
Fix memory leak in FLASH_Burn ioctl.
Fix error return codes in lcd_ioctl().
Move variable "index" in lcd_ioctl() to smaller scope to reduce memory usage.
Convert cli()/sti() to spin_lock_irqsave()/spin_unlock_irqrestore().
Fix legibility issues in FLASH_Burn ioctl.

- cli();
+ spin_lock_irqsave(&lcd_lock, flags);
for (index = 0; index < (128); index++) {

@@ -480,32 +485,30 @@
- *((volatile unsigned char *)
- burn_addr) =
- (volatile unsigned char) rom[index];
- while ((!dqpoll
- (burn_addr,
- (volatile unsigned char)
- rom[index]))
- && (!timeout(burn_addr))) {
- }
+ *((volatile unsigned char *)burn_addr) =
+ (volatile unsigned char) rom[index];
+ while ((!dqpoll (burn_addr,
+ (volatile unsigned char)
+ rom[index])) &&
+ (!timeout(burn_addr))) { }
- restore_flags(flags);
- if (*
- ((volatile unsigned char *) (burn_addr
- - 1)) ==
- (volatile unsigned char) rom[index -
- 1]) {
+ spin_unlock_irqrestore(&lcd_lock, flags);

Although this will work, i think local_irq_{disable,enable} is the right
solution (we don't protect any data, just make sure timings are right).

Since the Cobalt systems are single-processor, there's no functional difference between disabling global and local interrupts.

For making it SMP safe, easiest/sane solution seems semaphore in
lcd_dev, which is down_interruptible(), at beginning of read, write
and ioctl.



True, but it would requre making struct lcd_display a global variable, and involve reworking just about the whole driver, since it is declared as a local variable in individual ioctl case statements, and others use the variable defined at the top of the ioctl function. OTOH, it would reduce stack usage...

I was trying to minimize any mucking about with the functional parts of the driver, since I don't have the hardware to test it.

Since ioctls are still protected by the BKL right now (and this thing is pretty much nothing but an ioctl interface), I don't see too much of a problem with this driver. More complete drivers, however, would definitely benefit from it, and is something I'll keep in mind.

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at