Why lock_kernel() in drivers/pci/proc.c (2.6.0-test9)??

Why lock_kernel() in drivers/pci/proc.c (2.6.0-test9)??

[Peter Chubb]

Hi,
	What is the BKL protecting in
drivers/pci/proc.c:proc_bus_pci_lseek() ?

It looks useless to me, as file->f_pos is changed outside the lock
anyway, and all the other variables inside the locked region are
effectively constants for the purpose of this code.

So unless something subtle's going on, I suggest this:



--- linux-2.6.0-test9/drivers/pci/proc.c 2003-11-19 13:28:10.000000000 +1100
+++ new/drivers/pci/proc.c	2003-11-19 13:28:00.000000000 +1100
@@ -25,7 +25,6 @@
 {
 	loff_t new = -1;
 
-	lock_kernel();
 	switch (whence) {
 	case 0:
 		new = off;
@@ -37,7 +36,7 @@
 		new = PCI_CFG_SPACE_SIZE + off;
 		break;
 	}
-	unlock_kernel();
+
 	if (new < 0 || new > PCI_CFG_SPACE_SIZE)
 		return -EINVAL;
	return (file->f_pos = new);

Re: Why lock_kernel() in drivers/pci/proc.c (2.6.0-test9)??

[Andrew Morton]

Peter Chubb <peterc@gelato.unsw.edu.au> wrote:
>
> 
> Hi,
> 	What is the BKL protecting in
> drivers/pci/proc.c:proc_bus_pci_lseek() ?

nothing much.

> It looks useless to me, as file->f_pos is changed outside the lock
> anyway, and all the other variables inside the locked region are
> effectively constants for the purpose of this code.
> 
> So unless something subtle's going on, I suggest this:
> 

We normally use i_sem to prevent parallel lseeks against the same fd from
scribbling on f_pos.  Which seems fairly pointless since parallel reads
aren't using i_sem.

But this would be a more conventional debklification.  Does it work OK?


 drivers/pci/proc.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff -puN drivers/pci/proc.c~proc_bus_pci_lseek-remove-lock_kernel drivers/pci/proc.c
--- 25/drivers/pci/proc.c~proc_bus_pci_lseek-remove-lock_kernel	2003-11-18 18:47:20.000000000 -0800
+++ 25-akpm/drivers/pci/proc.c	2003-11-18 18:48:16.000000000 -0800
@@ -25,7 +25,7 @@ proc_bus_pci_lseek(struct file *file, lo
 {
 	loff_t new = -1;
 
-	lock_kernel();
+	down(&file->f_dentry->d_inode->i_sem);
 	switch (whence) {
 	case 0:
 		new = off;
@@ -37,10 +37,12 @@ proc_bus_pci_lseek(struct file *file, lo
 		new = PCI_CFG_SPACE_SIZE + off;
 		break;
 	}
-	unlock_kernel();
 	if (new < 0 || new > PCI_CFG_SPACE_SIZE)
-		return -EINVAL;
-	return (file->f_pos = new);
+		new = -EINVAL;
+	else
+		file->f_pos = new;
+	up(&file->f_dentry->d_inode->i_sem);
+	return new;
 }
 
 static ssize_t

_