Skip to contentSkip to conversation

RTAS - adapt procfs interface

PATCH In review Jens Osterkamp 5 participants 10 messages Apr 1, 2008 – Apr 4, 2008 PowerPC

lore
Share & tools
lore

Jens Osterkamp

author
#0 lore raw

PATCH 1/1 RTAS - adapt procfs interface

Hi,

rtas_flash was broken since 2.6.24-rc5. This patch fixes it. I think this is a good bugfix candidate for 2.6.25.

Jens

---

From: Maxim Shchetynin <[email protected]>

Handling of the proc_dir_entry->count has being changed in 2.6.24-rc5. After this change the default value for pde->count is 1 and not 0 as it was in earlier kernels. Therefore, if we want to check wether our procfs file is already opened (already in use), we have to check if pde->count is not greater than 2 but not 1.

Signed-off-by Maxim Shchetynin <[email protected]>
Signed-off-by Jens Osterkamp <[email protected]>
arch/powerpc/kernel/rtas_flash.c +1 -1
Index: linux-2.6/arch/powerpc/kernel/rtas_flash.c
===================================================================
@@ -356,7 +356,7 @@ static int rtas_excl_open(struct inode *
 
 	/* Enforce exclusive open with use count of PDE */
 	spin_lock(&flash_file_open_lock);
-	if (atomic_read(&dp->count) > 1) {
+	if (atomic_read(&dp->count) > 2) {
 		spin_unlock(&flash_file_open_lock);
 		return -EBUSY;
 	}
#1 lore raw
Quoted 2 lines · Jens Osterkamp

Handling of the proc_dir_entry->count has being changed in 2.6.24-rc5.

Do you know which commit caused the change?

Quoted 13 lines

After this change the default value for pde->count is 1 and not 0 as it was in earlier kernels. Therefore, if we want to check wether our procfs file is already opened (already in use), we have to check if pde->count is not greater than 2 but not 1.

Signed-off-by Maxim Shchetynin <[email protected]>
Signed-off-by Jens Osterkamp <[email protected]>

Index: linux-2.6/arch/powerpc/kernel/rtas_flash.c
===================================================================

arch/powerpc/kernel/rtas_flash.c static int rtas_excl_open(struct inode * · line 362

Quoted 8 lines
arch/powerpc/kernel/rtas_flash.c +1 -1
@@ -356,7 +356,7 @@ static int rtas_excl_open(struct inode *
 
 	/* Enforce exclusive open with use count of PDE */
 	spin_lock(&flash_file_open_lock);
-	if (atomic_read(&dp->count) > 1) {
+	if (atomic_read(&dp->count) > 2) {
 		spin_unlock(&flash_file_open_lock);
 		return -EBUSY;
 	}

One could argue that the real problem is using the proc_dir_entry's reference count to enforce exclusive open.

#2 lore raw
Quoted 3 lines · Nathan Lynch

One could argue that the real problem is using the proc_dir_entry's reference count to enforce exclusive open.

I think this is better... the way these files are used is lame, but this should preserve the existing behavior. I haven't yet tested this, can you?

arch/powerpc/kernel/rtas_flash.c +6 -15
index f227659..00bc308 100644
@@ -139,7 +139,7 @@ struct rtas_validate_flash_t
 	unsigned int update_results;	/* Update results token */
 };
 
-static DEFINE_SPINLOCK(flash_file_open_lock);
+static atomic_t open_count = ATOMIC_INIT(0);
 static struct proc_dir_entry *firmware_flash_pde;
 static struct proc_dir_entry *firmware_update_pde;
 static struct proc_dir_entry *validate_pde;

@@ -216,7 +216,7 @@ static int rtas_flash_release(struct inode *inode, struct file *file)
 		uf->flist = NULL;
 	}
 
-	atomic_dec(&dp->count);
+	atomic_dec(&open_count);
 	return 0;
 }
 

@@ -352,26 +352,17 @@ static ssize_t rtas_flash_write(struct file *file, const char __user *buffer,
 
 static int rtas_excl_open(struct inode *inode, struct file *file)
 {
-	struct proc_dir_entry *dp = PDE(inode);
-
-	/* Enforce exclusive open with use count of PDE */
-	spin_lock(&flash_file_open_lock);
-	if (atomic_read(&dp->count) > 1) {
-		spin_unlock(&flash_file_open_lock);
+	if (atomic_inc_return(&open_count) > 1) {
+		atomic_dec(&open_count);
 		return -EBUSY;
 	}
 
-	atomic_inc(&dp->count);
-	spin_unlock(&flash_file_open_lock);
-	
 	return 0;
 }
 
 static int rtas_excl_release(struct inode *inode, struct file *file)
 {
-	struct proc_dir_entry *dp = PDE(inode);
-
-	atomic_dec(&dp->count);
+	atomic_dec(&open_count);
 
 	return 0;
 }

@@ -580,7 +571,7 @@ static int validate_flash_release(struct inode *inode, struct file *file)
 	}
 
 	/* The matching atomic_inc was in rtas_excl_open() */
-	atomic_dec(&dp->count);
+	atomic_dec(&open_count);
 
 	return 0;
 }
author
#3 lore raw
Quoted 9 lines · Nathan Lynch
Quoted 3 lines · Nathan Lynch

One could argue that the real problem is using the proc_dir_entry's reference count to enforce exclusive open.

I think this is better... the way these files are used is lame, but this should preserve the existing behavior. I haven't yet tested this, can you?

I did and it works for me...

Quoted 56 lines
arch/powerpc/kernel/rtas_flash.c +5 -14
index f227659..00bc308 100644
@@ -139,7 +139,7 @@ struct rtas_validate_flash_t
 	unsigned int update_results;	/* Update results token */
 };
 
-static DEFINE_SPINLOCK(flash_file_open_lock);
+static atomic_t open_count = ATOMIC_INIT(0);
 static struct proc_dir_entry *firmware_flash_pde;
 static struct proc_dir_entry *firmware_update_pde;
 static struct proc_dir_entry *validate_pde;

@@ -216,7 +216,7 @@ static int rtas_flash_release(struct inode *inode, struct file *file)
 		uf->flist = NULL;
 	}
 
-	atomic_dec(&dp->count);
+	atomic_dec(&open_count);
 	return 0;
 }
 

@@ -352,26 +352,17 @@ static ssize_t rtas_flash_write(struct file *file, const char __user *buffer,
 
 static int rtas_excl_open(struct inode *inode, struct file *file)
 {
-	struct proc_dir_entry *dp = PDE(inode);
-
-	/* Enforce exclusive open with use count of PDE */
-	spin_lock(&flash_file_open_lock);
-	if (atomic_read(&dp->count) > 1) {
-		spin_unlock(&flash_file_open_lock);
+	if (atomic_inc_return(&open_count) > 1) {
+		atomic_dec(&open_count);
 		return -EBUSY;
 	}
 
-	atomic_inc(&dp->count);
-	spin_unlock(&flash_file_open_lock);
-	
 	return 0;
 }
 
 static int rtas_excl_release(struct inode *inode, struct file *file)
 {
-	struct proc_dir_entry *dp = PDE(inode);
-
-	atomic_dec(&dp->count);
+	atomic_dec(&open_count);
 
 	return 0;
 }

arch/powerpc/kernel/rtas_flash.c static int validate_flash_release(struct inode *inode, struct file *file) · line 577

Quoted 8 lines
arch/powerpc/kernel/rtas_flash.c +1 -1
@@ -580,7 +571,7 @@ static int validate_flash_release(struct inode *inode, struct file *file)
 	}
 
 	/* The matching atomic_inc was in rtas_excl_open() */
-	atomic_dec(&dp->count);
+	atomic_dec(&open_count);
 
 	return 0;
 }

Gruß,

Jens

IBM Deutschland Entwicklung GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Herbert Kircher
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

#4 lore raw
In reply to Nathan Lynch
Quoted 3 lines · Nathan Lynch

I think this is better... the way these files are used is lame, but this should preserve the existing behavior. I haven't yet tested this, can you?

Looks OK -- can I have a proper patch description and a signed-off-by line for this please?

Paul.

#5 lore raw
Quoted 8 lines · Paul Mackerras
Quoted 3 lines · Nathan Lynch

I think this is better... the way these files are used is lame, but this should preserve the existing behavior. I haven't yet tested this, can you?

Looks OK -- can I have a proper patch description and a signed-off-by line for this please?

Actually, my patch has the potentially undesirable consequence of allowing only one of the three flash-related proc files to be open at any time, whereas the previous behavior enforced exclusive open on a per-file basis.

If you want something for 2.6.25, I think the patch Jens posted is of lower risk.

author
#6 lore raw
In reply to Nathan Lynch
Quoted 5 lines · Nathan Lynch
Quoted 2 lines · Jens Osterkamp

Handling of the proc_dir_entry->count has being changed in 2.6.24-rc5.

Do you know which commit caused the change?

Yes, we bisected it to the following commit :

commit 5a622f2d0f86b316b07b55a4866ecb5518dd1cf7

Author: Alexey Dobriyan <[email protected]>
Date:   Tue Dec 4 23:45:28 2007 -0800

proc: fix proc_dir_entry refcounting

Creating PDEs with refcount 0 and "deleted" flag has problems (see below). Switch to usual scheme:
* PDE is created with refcount 1
* every de_get does +1
* every de_put() and remove_proc_entry() do -1
* once refcount reaches 0, PDE is freed.

This elegantly fixes at least two following races (both observed) without introducing new locks, without abusing old locks, without spreading lock_kernel():

[...]

Gruß,

Jens

IBM Deutschland Entwicklung GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Herbert Kircher
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

#7 lore raw
In reply to Jens Osterkamp
Quoted 5 lines · Jens Osterkamp

Hi,

rtas_flash was broken since 2.6.24-rc5. This patch fixes it. I think this is a good bugfix candidate for 2.6.25.

NACK. driver should not poke into the internal count member. Just provide your own inclusion with a new counter or set/test_bit.

#8 lore raw
Quoted 9 lines · Christoph Hellwig
Quoted 5 lines · Jens Osterkamp

=20
Hi,
=20
rtas_flash was broken since 2.6.24-rc5. This patch fixes it. I think this is a good bugfix candidate for 2.6.25.

=20
NACK. =A0driver should not poke into the internal count member. =A0Just provide your own inclusion with a new counter or set/test_bit.

Well we still have the regression against 2.6.23 and I'd like to get that fixed in 2.6.25 if it's not already release by the time we get there.

Would you prefer using Nathan's proper patch for 2.6.25, or do that for 2.6.26 instead, perhaps using Jens' patch as a quick fix, as Nathan suggested?

	Arnd <><
#9 lore raw
Quoted 7 lines · Arnd Bergmann

Well we still have the regression against 2.6.23 and I'd like to get that fixed in 2.6.25 if it's not already release by the time we get there.

Would you prefer using Nathan's proper patch for 2.6.25, or do that for 2.6.26 instead, perhaps using Jens' patch as a quick fix, as Nathan suggested?

Nathan's patch is not correct either because each file needs it's own open count. But something similar would be correct.