[PATCH] RTAS - adapt procfs interface

[PATCH] RTAS - adapt procfs interface

[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.

Jens

---

From: Maxim Shchetynin <maxim@de.ibm.com>

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 <maxim@de.ibm.com>
Signed-off-by: Jens Osterkamp <jens@de.ibm.com>

Index: linux-2.6/arch/powerpc/kernel/rtas_flash.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/rtas_flash.c
+++ 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;
 	}

Re: [PATCH] RTAS - adapt procfs interface

[Nathan Lynch]

Jens Osterkamp wrote:
> 
> Handling of the proc_dir_entry->count has being changed in 2.6.24-rc5.

Do you know which commit caused the change?


> 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 <maxim@de.ibm.com>
> Signed-off-by: Jens Osterkamp <jens@de.ibm.com>
> 
> Index: linux-2.6/arch/powerpc/kernel/rtas_flash.c
> ===================================================================
> --- linux-2.6.orig/arch/powerpc/kernel/rtas_flash.c
> +++ 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;
>  	}

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

Re: [PATCH] RTAS - adapt procfs interface

[Nathan Lynch]

Nathan Lynch wrote:
> 
> 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?


diff --git a/arch/powerpc/kernel/rtas_flash.c b/arch/powerpc/kernel/rtas_flash.c
index f227659..00bc308 100644
--- a/arch/powerpc/kernel/rtas_flash.c
+++ b/arch/powerpc/kernel/rtas_flash.c
@@ -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;
 }

Re: [PATCH] RTAS - adapt procfs interface

[Jens Osterkamp]

On Tuesday 01 April 2008, Nathan Lynch wrote:
> Jens Osterkamp wrote:
> > 
> > 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 <adobriyan@sw.ru>
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

Re: [PATCH] RTAS - adapt procfs interface

[Jens Osterkamp]

On Tuesday 01 April 2008, Nathan Lynch wrote:
> Nathan Lynch wrote:
> > 
> > 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...

> 
> 
> diff --git a/arch/powerpc/kernel/rtas_flash.c b/arch/powerpc/kernel/rtas_flash.c
> index f227659..00bc308 100644
> --- a/arch/powerpc/kernel/rtas_flash.c
> +++ b/arch/powerpc/kernel/rtas_flash.c
> @@ -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;
>  }
> 

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

Re: [PATCH] RTAS - adapt procfs interface

[Paul Mackerras]

Nathan Lynch writes:

> 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.

Re: [PATCH] RTAS - adapt procfs interface

[Nathan Lynch]

Paul Mackerras wrote:
> Nathan Lynch writes:
> 
> > 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.

Re: [PATCH] RTAS - adapt procfs interface

[Christoph Hellwig]

On Tue, Apr 01, 2008 at 03:12:20PM +0200, Jens Osterkamp wrote:
> 
> 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.

Re: [Cbe-oss-dev] [PATCH] RTAS - adapt procfs interface

[Arnd Bergmann]

On Friday 04 April 2008, Christoph Hellwig wrote:
> On Tue, Apr 01, 2008 at 03:12:20PM +0200, Jens Osterkamp wrote:
> >=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 <><

Re: [Cbe-oss-dev] [PATCH] RTAS - adapt procfs interface

[Christoph Hellwig]

On Fri, Apr 04, 2008 at 07:39:29AM +0200, Arnd Bergmann wrote:
> 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.