[KJ] [PATCH] lcd: replace cli()/sti() with

[KJ] [PATCH] lcd: replace cli()/sti() with

[James Nelson]

[-- Attachment #1: Type: text/plain, Size: 1191 bytes --]

Remove the cli()/sti() calls in drivers/char/lcd.c

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-17 18:57:10.760197439 -0500
@@ -33,6 +33,8 @@
 
 #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);
 
@@ -464,14 +466,13 @@
 			}
 
 			printk("Churning and Burning -");
-			save_flags(flags);
 			for (i = 0; i < FLASH_SIZE; i = i + 128) {
 
 				if (copy_from_user
 				    (rom, display.RomImage + i, 128))
 					return -EFAULT;
 				burn_addr = kFlashBase + i;
-				cli();
+				spin_lock_irqsave(&lcd_lock, flags);
 				for (index = 0; index < (128); index++) {
 
 					WRITE_FLASH(kFlash_Addr1,
@@ -492,7 +493,7 @@
 					}
 					burn_addr++;
 				}
-				restore_flags(flags);
+				spin_unlock_irqrestore(&lcd_lock, flags);
 				if (*
 				    ((volatile unsigned char *) (burn_addr
 								 - 1)) ==

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[PATCH] lcd: replace cli()/sti() with spin_lock_irqsave()/spin_unlock_irqrestore()

[James Nelson]

Remove the cli()/sti() calls in drivers/char/lcd.c

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-17 18:57:10.760197439 -0500
@@ -33,6 +33,8 @@
 
 #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);
 
@@ -464,14 +466,13 @@
 			}
 
 			printk("Churning and Burning -");
-			save_flags(flags);
 			for (i = 0; i < FLASH_SIZE; i = i + 128) {
 
 				if (copy_from_user
 				    (rom, display.RomImage + i, 128))
 					return -EFAULT;
 				burn_addr = kFlashBase + i;
-				cli();
+				spin_lock_irqsave(&lcd_lock, flags);
 				for (index = 0; index < (128); index++) {
 
 					WRITE_FLASH(kFlash_Addr1,
@@ -492,7 +493,7 @@
 					}
 					burn_addr++;
 				}
-				restore_flags(flags);
+				spin_unlock_irqrestore(&lcd_lock, flags);
 				if (*
 				    ((volatile unsigned char *) (burn_addr
 								 - 1)) ==

[KJ] Re: [PATCH] lcd: replace cli()/sti() with

[Jan Dittmer]

James Nelson wrote:
> Remove the cli()/sti() calls in drivers/char/lcd.c

Why is this cli() there in the first place? ioctl is already
called under lock_kernel.

> 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-17 18:57:10.760197439 -0500
> @@ -33,6 +33,8 @@
>  
>  #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);
>  
> @@ -464,14 +466,13 @@
>  			}
>  
>  			printk("Churning and Burning -");
> -			save_flags(flags);
>  			for (i = 0; i < FLASH_SIZE; i = i + 128) {
>  
>  				if (copy_from_user
>  				    (rom, display.RomImage + i, 128))
>  					return -EFAULT;

The driver is leaking memory, rom is not freed in this case.

Jan
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [PATCH] lcd: replace cli()/sti() with spin_lock_irqsave()/spin_unlock_irqrestore()

[Jan Dittmer]

James Nelson wrote:
> Remove the cli()/sti() calls in drivers/char/lcd.c

Why is this cli() there in the first place? ioctl is already
called under lock_kernel.

> 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-17 18:57:10.760197439 -0500
> @@ -33,6 +33,8 @@
>  
>  #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);
>  
> @@ -464,14 +466,13 @@
>  			}
>  
>  			printk("Churning and Burning -");
> -			save_flags(flags);
>  			for (i = 0; i < FLASH_SIZE; i = i + 128) {
>  
>  				if (copy_from_user
>  				    (rom, display.RomImage + i, 128))
>  					return -EFAULT;

The driver is leaking memory, rom is not freed in this case.

Jan

[KJ] Re: [PATCH] lcd: replace cli()/sti() with

[Jim Nelson]

[-- Attachment #1: Type: text/plain, Size: 1865 bytes --]

Jan Dittmer wrote:
> James Nelson wrote:
> 
>>Remove the cli()/sti() calls in drivers/char/lcd.c
> 
> 
> Why is this cli() there in the first place? ioctl is already
> called under lock_kernel.
> 
> 

First - a warning.  Newbie on the loose, running around, asking for a whack with 
the cluebat.

I had seen other drivers that had cli()/sti() calls in the ioctl functions.  So, 
that is wrong?  Should all of those cli()/sti() calls be removed?

Just to site things properly in my mind, was it always the case that ioctl is 
called under lock_kernel, or is this a relatively recent (2.3+) development?  Some 
of the *really* abandoned drivers haven't been touched in at least that long.

>>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-17 18:57:10.760197439 -0500
>>@@ -33,6 +33,8 @@
>> 
>> #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);
>> 
>>@@ -464,14 +466,13 @@
>> 			}
>> 
>> 			printk("Churning and Burning -");
>>-			save_flags(flags);
>> 			for (i = 0; i < FLASH_SIZE; i = i + 128) {
>> 
>> 				if (copy_from_user
>> 				    (rom, display.RomImage + i, 128))
>> 					return -EFAULT;
> 
> 
> The driver is leaking memory, rom is not freed in this case.

Erm.  Didn't notice that.

> 
> Jan
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [PATCH] lcd: replace cli()/sti() with spin_lock_irqsave()/spin_unlock_irqrestore()

[Jim Nelson]

Jan Dittmer wrote:
> James Nelson wrote:
> 
>>Remove the cli()/sti() calls in drivers/char/lcd.c
> 
> 
> Why is this cli() there in the first place? ioctl is already
> called under lock_kernel.
> 
> 

First - a warning.  Newbie on the loose, running around, asking for a whack with 
the cluebat.

I had seen other drivers that had cli()/sti() calls in the ioctl functions.  So, 
that is wrong?  Should all of those cli()/sti() calls be removed?

Just to site things properly in my mind, was it always the case that ioctl is 
called under lock_kernel, or is this a relatively recent (2.3+) development?  Some 
of the *really* abandoned drivers haven't been touched in at least that long.

>>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-17 18:57:10.760197439 -0500
>>@@ -33,6 +33,8 @@
>> 
>> #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);
>> 
>>@@ -464,14 +466,13 @@
>> 			}
>> 
>> 			printk("Churning and Burning -");
>>-			save_flags(flags);
>> 			for (i = 0; i < FLASH_SIZE; i = i + 128) {
>> 
>> 				if (copy_from_user
>> 				    (rom, display.RomImage + i, 128))
>> 					return -EFAULT;
> 
> 
> The driver is leaking memory, rom is not freed in this case.

Erm.  Didn't notice that.

> 
> Jan
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [KJ] [PATCH] lcd: replace cli()/sti() with

[Matthew Wilcox]

[-- Attachment #1: Type: text/plain, Size: 1180 bytes --]

On Fri, Dec 17, 2004 at 05:59:05PM -0600, James Nelson wrote:
> Remove the cli()/sti() calls in drivers/char/lcd.c

I'm not sure why anyone would bother ...

config COBALT_LCD
        bool "Support for Cobalt LCD"
        depends on MIPS_COBALT

those machines are never SMP.

However, looking at the driver, it doesn't seem to generate interrupts,
so I don't see what good disabling interrupts would do.  I think we can
simply remove the save_flags/cli/restore_flags from this driver.  It needs
a fair chunk of work though -- should be converted to use writeb instead of

                                        *((volatile unsigned char *)
                                          burn_addr) =
                 (volatile unsigned char) rom[index];

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] lcd: replace cli()/sti() with spin_lock_irqsave()/spin_unlock_irqrestore()

[Matthew Wilcox]

On Fri, Dec 17, 2004 at 05:59:05PM -0600, James Nelson wrote:
> Remove the cli()/sti() calls in drivers/char/lcd.c

I'm not sure why anyone would bother ...

config COBALT_LCD
        bool "Support for Cobalt LCD"
        depends on MIPS_COBALT

those machines are never SMP.

However, looking at the driver, it doesn't seem to generate interrupts,
so I don't see what good disabling interrupts would do.  I think we can
simply remove the save_flags/cli/restore_flags from this driver.  It needs
a fair chunk of work though -- should be converted to use writeb instead of

                                        *((volatile unsigned char *)
                                          burn_addr) =
                 (volatile unsigned char) rom[index];

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [KJ] Re: [PATCH] lcd: replace cli()/sti() with

[Matthew Wilcox]

[-- Attachment #1: Type: text/plain, Size: 2556 bytes --]

On Fri, Dec 17, 2004 at 10:02:16PM -0500, Jim Nelson wrote:
> Jan Dittmer wrote:
> >James Nelson wrote:
> >
> >>Remove the cli()/sti() calls in drivers/char/lcd.c
> >
> >
> >Why is this cli() there in the first place? ioctl is already
> >called under lock_kernel.
> 
> First - a warning.  Newbie on the loose, running around, asking for a whack 
> with the cluebat.

OK.  Quick lesson (for both of you actually; Jan's response wasn't accurate).

Once upon a time, we did not support SMP.  We still had to worry about
races.  Interrupts could arrive and touch hardware/variables that
we were in the middle of poking.  So if you disable interrupts (the
instructions are annoying named after the senseless x86 instructions)
around the critical section, you ensure that you can't be interrupted.

Then we introduced the Big Kernel Lock (aka lock_kernel).  As long as
all code that touched the same hardware/variables had the BKL, cli()
was still safe.  We were evil and redefined the cli() function to disable
interrupts on *all* processors, not just the local one.

Thankfully, the ability to disable interrupts globally has been
removed now.  Instead, you must disable _local_ interrupts and then
acquire a lock.  This is spin_lock_irq()/spin_lock_irqsave().  In order
to protect against an interrupt running on a *different* CPU, you have
to take the same lock in the interrupt handler.  This is safe because
a process running on a different CPU will eventually release its lock.

So you still need to disable interrupts, even if you're running under the
BKL -- interrupt routines can't acquire the BKL (because it's magic) and
acquiring the BKL doesn't disable interrupts.

There's some special tricks you can use so you don't have to grab a lock,
but these are advanced magic (eg RCU, seqlocks, atomics), but most drivers
won't use advanced techniques; they're mostly for specialised parts of
the kernel.  In particular drivers shouldn't try to invent their own
locking scheme, they invariably get it subtly wrong.

Hope that clears things up for you a bit; feel free to ask about the
bits that I didn't explain sufficiently.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] Re: [PATCH] lcd: replace cli()/sti() with spin_lock_irqsave()/spin_unlock_irqrestore()

[Matthew Wilcox]

On Fri, Dec 17, 2004 at 10:02:16PM -0500, Jim Nelson wrote:
> Jan Dittmer wrote:
> >James Nelson wrote:
> >
> >>Remove the cli()/sti() calls in drivers/char/lcd.c
> >
> >
> >Why is this cli() there in the first place? ioctl is already
> >called under lock_kernel.
> 
> First - a warning.  Newbie on the loose, running around, asking for a whack 
> with the cluebat.

OK.  Quick lesson (for both of you actually; Jan's response wasn't accurate).

Once upon a time, we did not support SMP.  We still had to worry about
races.  Interrupts could arrive and touch hardware/variables that
we were in the middle of poking.  So if you disable interrupts (the
instructions are annoying named after the senseless x86 instructions)
around the critical section, you ensure that you can't be interrupted.

Then we introduced the Big Kernel Lock (aka lock_kernel).  As long as
all code that touched the same hardware/variables had the BKL, cli()
was still safe.  We were evil and redefined the cli() function to disable
interrupts on *all* processors, not just the local one.

Thankfully, the ability to disable interrupts globally has been
removed now.  Instead, you must disable _local_ interrupts and then
acquire a lock.  This is spin_lock_irq()/spin_lock_irqsave().  In order
to protect against an interrupt running on a *different* CPU, you have
to take the same lock in the interrupt handler.  This is safe because
a process running on a different CPU will eventually release its lock.

So you still need to disable interrupts, even if you're running under the
BKL -- interrupt routines can't acquire the BKL (because it's magic) and
acquiring the BKL doesn't disable interrupts.

There's some special tricks you can use so you don't have to grab a lock,
but these are advanced magic (eg RCU, seqlocks, atomics), but most drivers
won't use advanced techniques; they're mostly for specialised parts of
the kernel.  In particular drivers shouldn't try to invent their own
locking scheme, they invariably get it subtly wrong.

Hope that clears things up for you a bit; feel free to ask about the
bits that I didn't explain sufficiently.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [KJ] Re: [PATCH] lcd: replace cli()/sti() with

[Jan Dittmer]

Matthew Wilcox wrote:
> On Fri, Dec 17, 2004 at 10:02:16PM -0500, Jim Nelson wrote:
> 
>>Jan Dittmer wrote:
>>
>>>James Nelson wrote:
>>>
>>>
>>>>Remove the cli()/sti() calls in drivers/char/lcd.c
>>>
>>>
>>>Why is this cli() there in the first place? ioctl is already
>>>called under lock_kernel.
>>
>>First - a warning.  Newbie on the loose, running around, asking for a whack 
>>with the cluebat.
> 
> 
> OK.  Quick lesson (for both of you actually; Jan's response wasn't accurate).

Thanks.

> So you still need to disable interrupts, even if you're running under the
> BKL -- interrupt routines can't acquire the BKL (because it's magic) and
> acquiring the BKL doesn't disable interrupts.

But still, two ioctl()s can't run in parallel, can they? The driver isn't
using any interrupts (I'm aware that this asumption isn't necessarily
true in the future due to ioctl_unlocked work, etc.).

Jan
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] Re: [PATCH] lcd: replace cli()/sti() with

[Matthew Wilcox]

[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]

On Sat, Dec 18, 2004 at 09:54:49AM +0100, Jan Dittmer wrote:
> > So you still need to disable interrupts, even if you're running under the
> > BKL -- interrupt routines can't acquire the BKL (because it's magic) and
> > acquiring the BKL doesn't disable interrupts.
> 
> But still, two ioctl()s can't run in parallel, can they? The driver isn't
> using any interrupts (I'm aware that this asumption isn't necessarily
> true in the future due to ioctl_unlocked work, etc.).

You're right.  However, let me just clarify that a little due to the
magic nature of the BKL.  It basically simulates being uniprocessor,
and there are other races that exist on a uniprocessor box.  If process A
caalls ioctl() and the ioctl handler does copy_from_user, the kernel may
take a page fault (if the page isn't resident, for example).  If it does,
the kernel will automatically drop the BKL and schedule another process.
If process B then calls ioctl(), you can see the same ioctl be reentered.
Normally this isn't a problem, but it's something to be aware of.  You
can use a semaphore (down()/up()) if you need this to not happen.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors