Re: lcd: fix memory leak, code cleanup

[Jim Nelson]

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++) {
> >
> > 					WRITE_FLASH(kFlash_Addr1,
> > @@ -480,32 +485,30 @@
> > 						    kFlash_Data2);
> > 					WRITE_FLASH(kFlash_Addr1,
> > 						    kFlash_Prog);
> > -					*((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))) { }
> > 					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.
>
> Comments?
>
>
> 	Domen

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.

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