* [PATCH] lcd: fix memory leak, code cleanup
@ 2004-12-21 1:50 James Nelson
2004-12-21 12:06 ` Domen Puncer
0 siblings, 1 reply; 3+ messages in thread
From: James Nelson @ 2004-12-21 1:50 UTC (permalink / raw)
To: kernel-janitors, linux-kernel; +Cc: akpm, James Nelson
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.
Diffstat output:
lcd.c | 86 +++++++++++++++++++++++++++++++++---------------------------------
lcd.h | 2 +
2 files changed, 46 insertions(+), 42 deletions(-)
Signed-off-by: James Nelson <james4765@gmail.com>
diff -urN --exclude='*~' linux-2.6.10-rc3-mm1-original/drivers/char/lcd.c linux-2.6.10-rc3-mm1/drivers/char/lcd.c
--- linux-2.6.10-rc3-mm1-original/drivers/char/lcd.c 2004-12-03 16:53:42.000000000 -0500
+++ linux-2.6.10-rc3-mm1/drivers/char/lcd.c 2004-12-20 20:40:14.922386778 -0500
@@ -33,11 +33,14 @@
#include "lcd.h"
+static spinlock_t lcd_lock = SPIN_LOCK_UNLOCKED;
+
static int lcd_ioctl(struct inode *inode, struct file *file,
unsigned int cmd, unsigned long arg);
-static int lcd_present = 1;
+static unsigned int lcd_present = 1;
+/* used in arch/mips/cobalt/reset.c */
int led_state = 0;
#if defined(CONFIG_TULIP) && 0
@@ -63,7 +66,6 @@
{
struct lcd_display button_display;
unsigned long address, a;
- int index;
switch (cmd) {
case LCD_On:
@@ -220,6 +222,7 @@
case LCD_Write:{
struct lcd_display display;
+ unsigned int index;
if (copy_from_user
@@ -316,7 +319,7 @@
// set only bit led_display.leds
case LED_Bit_Set:{
- int i;
+ unsigned int i;
int bit = 1;
struct lcd_display led_display;
@@ -338,7 +341,7 @@
// clear only bit led_display.leds
case LED_Bit_Clear:{
- int i;
+ unsigned int i;
int bit = 1;
struct lcd_display led_display;
@@ -413,6 +416,10 @@
int ctr = 0;
+ if ( !capable(CAP_SYS_ADMIN) ) return -EPERM;
+
+ pr_info(LCD "Erasing Flash\n");
+
// Chip Erase Sequence
WRITE_FLASH(kFlash_Addr1, kFlash_Data1);
WRITE_FLASH(kFlash_Addr2, kFlash_Data2);
@@ -421,21 +428,15 @@
WRITE_FLASH(kFlash_Addr2, kFlash_Data2);
WRITE_FLASH(kFlash_Addr1, kFlash_Erase6);
- printk("Erasing Flash.\n");
-
while ((!dqpoll(0x00000000, 0xFF))
&& (!timeout(0x00000000))) {
ctr++;
}
- printk("\n");
- printk("\n");
- printk("\n");
-
if (READ_FLASH(0x07FFF0) == 0xFF) {
- printk("Erase Successful\r\n");
+ pr_info(LCD "Erase Successful\n");
} else if (timeout) {
- printk("Erase Timed Out\r\n");
+ pr_info(LCD "Erase Timed Out\n");
}
break;
@@ -447,31 +448,35 @@
volatile unsigned long burn_addr;
unsigned long flags;
- int i;
+ unsigned int i, index;
unsigned char *rom;
struct lcd_display display;
+ if ( !capable(CAP_SYS_ADMIN) ) return -EPERM;
+
if (copy_from_user
(&display, (struct lcd_display *) arg,
sizeof(struct lcd_display)))
return -EFAULT;
rom = (unsigned char *) kmalloc((128), GFP_ATOMIC);
if (rom == NULL) {
- printk("broken\n");
- return 1;
+ printk(KERN_ERR LCD "kmalloc() failed in %s\n",
+ __FUNCTION__);
+ return -ENOMEM;
}
- printk("Churning and Burning -");
- save_flags(flags);
+ pr_info(LCD "Starting Flash burn\n");
for (i = 0; i < FLASH_SIZE; i = i + 128) {
if (copy_from_user
- (rom, display.RomImage + i, 128))
+ (rom, display.RomImage + i, 128)) {
+ kfree(rom);
return -EFAULT;
+ }
burn_addr = kFlashBase + i;
- 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);
+ if (* ((volatile unsigned char *)
+ (burn_addr - 1)) ==
+ (volatile unsigned char)
+ rom[index - 1]) {
} else if (timeout) {
- printk("Program timed out\r\n");
+ pr_info(LCD "Flash burn timed out\n");
}
}
kfree(rom);
+ pr_info(LCD "Flash successfully burned\n");
+
break;
}
@@ -515,7 +518,7 @@
unsigned char *user_bytes;
volatile unsigned long read_addr;
- int i;
+ unsigned int i;
user_bytes =
&(((struct lcd_display *) arg)->RomImage[0]);
@@ -524,7 +527,7 @@
(VERIFY_WRITE, user_bytes, FLASH_SIZE))
return -EFAULT;
- printk("Reading Flash");
+ pr_info(LCD "Reading Flash");
for (i = 0; i < FLASH_SIZE; i++) {
unsigned char tmp_byte;
read_addr = kFlashBase + i;
@@ -540,8 +543,7 @@
}
default:
- return 0;
- break;
+ return -EINVAL;
}
@@ -613,7 +615,7 @@
{
unsigned long data;
- printk("%s\n", LCD_DRIVER);
+ pr_info("%s\n", LCD_DRIVER);
misc_register(&lcd_dev);
/* Check region? Naaah! Just snarf it up. */
@@ -623,7 +625,7 @@
data = LCDReadData;
if ((data & 0x000000FF) == (0x00)) {
lcd_present = 0;
- printk("LCD Not Present\n");
+ pr_info(LCD "LCD Not Present\n");
} else {
lcd_present = 1;
WRITE_GAL(kGal_DevBank2PReg, kGal_DevBank2Cfg);
diff -urN --exclude='*~' linux-2.6.10-rc3-mm1-original/drivers/char/lcd.h linux-2.6.10-rc3-mm1/drivers/char/lcd.h
--- linux-2.6.10-rc3-mm1-original/drivers/char/lcd.h 2004-12-03 16:53:47.000000000 -0500
+++ linux-2.6.10-rc3-mm1/drivers/char/lcd.h 2004-12-20 19:08:10.795163702 -0500
@@ -37,6 +37,8 @@
#define LCD_DRIVER "Cobalt LCD Driver v2.10"
+#define LCD "lcd: "
+
#define kLCD_IR 0x0F000000
#define kLCD_DR 0x0F000010
#define kGPI 0x0D000000
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: lcd: fix memory leak, code cleanup
2004-12-21 1:50 [PATCH] lcd: fix memory leak, code cleanup James Nelson
@ 2004-12-21 12:06 ` Domen Puncer
2004-12-22 3:45 ` Jim Nelson
0 siblings, 1 reply; 3+ messages in thread
From: Domen Puncer @ 2004-12-21 12:06 UTC (permalink / raw)
To: James Nelson; +Cc: kernel-janitors, linux-kernel, akpm
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).
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: lcd: fix memory leak, code cleanup
2004-12-21 12:06 ` Domen Puncer
@ 2004-12-22 3:45 ` Jim Nelson
0 siblings, 0 replies; 3+ messages in thread
From: Jim Nelson @ 2004-12-22 3:45 UTC (permalink / raw)
To: Domen Puncer; +Cc: kernel-janitors, linux-kernel, akpm
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2004-12-22 3:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-12-21 1:50 [PATCH] lcd: fix memory leak, code cleanup James Nelson
2004-12-21 12:06 ` Domen Puncer
2004-12-22 3:45 ` Jim Nelson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox