[PATCH] remove check_region in drivers-char-specialix.c

[PATCH] remove check_region in drivers-char-specialix.c

[Borislav Petkov]

Hi Andrew,

   This is also a pretty simple case. We remove the wrapper and make
   sx__request_io_range return struct resource *. We check its value accordingly
   in the probing routine. It compiles cleanly here.

   Signed-off-by: Borislav Petkov <petkov@uni-muenster.de>


--- 2.6.14-rc2/drivers/char/specialix.c.orig	2005-09-28 10:02:31.000000000 +0200
+++ 2.6.14-rc2/drivers/char/specialix.c	2005-09-28 10:30:24.000000000 +0200
@@ -338,15 +338,9 @@ static inline void sx_wait_CCR_off(struc
  *  specialix IO8+ IO range functions.
  */
 
-static inline int sx_check_io_range(struct specialix_board * bp)
+static inline struct resource * sx_request_io_range(struct specialix_board * bp)
 {
-	return check_region (bp->base, SX_IO_SPACE);
-}
-
-
-static inline void sx_request_io_range(struct specialix_board * bp)
-{
-	request_region(bp->base, 
+	return request_region(bp->base, 
 	               bp->flags&SX_BOARD_IS_PCI?SX_PCI_IO_SPACE:SX_IO_SPACE,
 	               "specialix IO8+" );
 }
@@ -495,7 +489,7 @@ static int sx_probe(struct specialix_boa
 
 	func_enter();
 
-	if (sx_check_io_range(bp)) {
+	if (!sx_request_io_range(bp)) {
 		func_exit();
 		return 1;
 	}
@@ -583,7 +577,6 @@ static int sx_probe(struct specialix_boa
 		return -EIO;
 	}
 
-	sx_request_io_range(bp);
 	bp->flags |= SX_BOARD_PRESENT;
 	
 	/* Chip           revcode   pkgtype

	

	
		
___________________________________________________________ 
Gesendet von Yahoo! Mail - Jetzt mit 1GB Speicher kostenlos - Hier anmelden: http://mail.yahoo.de

Re: [PATCH] remove check_region in drivers-char-specialix.c

[Al Viro]

On Wed, Sep 28, 2005 at 10:37:37AM +0200, Borislav Petkov wrote:
> Hi Andrew,
> 
>    This is also a pretty simple case. We remove the wrapper and make
>    sx__request_io_range return struct resource *. We check its value accordingly
>    in the probing routine. It compiles cleanly here.

NAK.  You've just introduced a pile of leaks - if sx_probe() fails after
that call, you end up with region claimed and not released.

Re: [PATCH] remove check_region in drivers-char-specialix.c

[Borislav Petkov]

On Wed, Sep 28, 2005 at 06:52:44PM +0100, Al Viro wrote:
> On Wed, Sep 28, 2005 at 10:37:37AM +0200, Borislav Petkov wrote:
> > Hi Andrew,
> > 
> >    This is also a pretty simple case. We remove the wrapper and make
> >    sx__request_io_range return struct resource *. We check its value accordingly
> >    in the probing routine. It compiles cleanly here.
> 
> NAK.  You've just introduced a pile of leaks - if sx_probe() fails after
> that call, you end up with region claimed and not released.

Andrew told me already today that Jeff[1] had sent a patch fixing all that. To
prevent the leaks he's calling sx_release_io_range(bp) in every check before
exiting sx_probe so this seems correct. A small question though: After calling
sx_request_io_range() in the if-statement on line 499 is it ok to call
sx_request_io_range() for a second time in a row on line 587?  I think in
this case the second call has to go, no?

[1]rsync://rsync.kernel.org/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git

	

	
		
___________________________________________________________ 
Gesendet von Yahoo! Mail - Jetzt mit 1GB Speicher kostenlos - Hier anmelden: http://mail.yahoo.de

Re: [PATCH] remove check_region in drivers-char-specialix.c

[Al Viro]

On Thu, Sep 29, 2005 at 12:28:22AM +0200, Borislav Petkov wrote:
> Andrew told me already today that Jeff[1] had sent a patch fixing all that. To
> prevent the leaks he's calling sx_release_io_range(bp) in every check before
> exiting sx_probe so this seems correct. A small question though: After calling
> sx_request_io_range() in the if-statement on line 499 is it ok to call
> sx_request_io_range() for a second time in a row on line 587?  I think in
> this case the second call has to go, no?
> 
> [1]rsync://rsync.kernel.org/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git

Huh?  I don't see any specialix patches in that repository right now...

But yes, after successful request_region() you shouldn't call it again -
that would simply fail.

Re: [PATCH] remove check_region in drivers-char-specialix.c

[Andrew Morton]

Al Viro <viro@ftp.linux.org.uk> wrote:
>
> On Thu, Sep 29, 2005 at 12:28:22AM +0200, Borislav Petkov wrote:
>  > Andrew told me already today that Jeff[1] had sent a patch fixing all that. To
>  > prevent the leaks he's calling sx_release_io_range(bp) in every check before
>  > exiting sx_probe so this seems correct. A small question though: After calling
>  > sx_request_io_range() in the if-statement on line 499 is it ok to call
>  > sx_request_io_range() for a second time in a row on line 587?  I think in
>  > this case the second call has to go, no?
>  > 
>  > [1]rsync://rsync.kernel.org/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git
> 
>  Huh?  I don't see any specialix patches in that repository right now...

http://www.spinics.net/lists/kernel/msg399680.html

Re: [PATCH] remove check_region in drivers-char-specialix.c

[Al Viro]

On Wed, Sep 28, 2005 at 06:41:06PM -0700, Andrew Morton wrote:
 
> http://www.spinics.net/lists/kernel/msg399680.html

Ewww...  A lot of chunks consisting only of whitespace removals - great
way to make patch less readable...

And yes, that second call of sx_request_io_range() must die.  BTW,
what's wrong with use of mdelay() instead of that sx_long_delay()
junk?  Replacing both calls of sx_long_delay() with mdelay(50) would do it...

Re: [PATCH] remove check_region in drivers-char-specialix.c

[Rogier Wolff]

On Thu, Sep 29, 2005 at 03:05:10AM +0100, Al Viro wrote:
> On Wed, Sep 28, 2005 at 06:41:06PM -0700, Andrew Morton wrote:
>  
> > http://www.spinics.net/lists/kernel/msg399680.html
> 
> Ewww...  A lot of chunks consisting only of whitespace removals - great
> way to make patch less readable...
> 
> And yes, that second call of sx_request_io_range() must die.  BTW,
> what's wrong with use of mdelay() instead of that sx_long_delay()
> junk?  Replacing both calls of sx_long_delay() with mdelay(50) would do it...

Trust me: mdelay didn't exist when I wrote that.

The code calls the private function that does what I think should've
been kernel infrastructure all along to make it easy to either: 
change the body of the function to call the new infrastructure, or 
replace the call of the private function with the call to the new
infrastructure. 

	Roger. 

-- 
** R.E.Wolff@BitWizard.nl ** http://www.BitWizard.nl/ ** +31-15-2600998 **
*-- BitWizard writes Linux device drivers for any device you may have! --*
Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement. 
Does it sit on the couch all day? Is it unemployed? Please be specific! 
Define 'it' and what it isn't doing. --------- Adapted from lxrbot FAQ

Re: [PATCH] remove check_region in drivers-char-specialix.c

[Christoph Hellwig]

On Thu, Sep 29, 2005 at 03:05:10AM +0100, Al Viro wrote:
> On Wed, Sep 28, 2005 at 06:41:06PM -0700, Andrew Morton wrote:
>  
> > http://www.spinics.net/lists/kernel/msg399680.html
> 
> Ewww...  A lot of chunks consisting only of whitespace removals - great
> way to make patch less readable...
> 
> And yes, that second call of sx_request_io_range() must die.  BTW,
> what's wrong with use of mdelay() instead of that sx_long_delay()
> junk?  Replacing both calls of sx_long_delay() with mdelay(50) would do it...

There's a proper check_region removal for specialix.c patch from me queued
up in the kernel-janitors tree.  Including removal of the silly wrappers.