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 <[email protected]>
--- 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
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.
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
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.
Al Viro <[email protected]> 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
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...
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.
--
** [email protected] ** 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
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.