[PATCH] drivers: scsi: aic7xxx: Fix positive error codes.

[PATCH] drivers: scsi: aic7xxx: Fix positive error codes.

[Jason Wilkes]

Note:
There are more instances of the problem described below, but I
thought I'd explain the first one in detail, to make sure it's
worth fixing the others (and to make sure I didn't do anything
stupid, which I may have. I'm new to this :-). Alright, here we go!

A few drivers seem to return positive error codes internally, in
order to signal something to a static function in the same driver.
(see, for example, drivers/staging/lustre/lnet/lnet/lib-move.c)
That's unusual, but seems okay as long as they're consistent between
return codes and return code checks. However, a problem might arise
if +ERRORCODE rather than -ERRORCODE is returned to other layers
of the kernel (e.g., to the driver core). Let's do some exploring...

Here's a function that returns +ENOMEM
FILE: drivers/scsi/aic7xxx/aic7770_osm.c
FUNC: aic7770_map_registers

int
aic7770_map_registers(struct ahc_softc *ahc, u_int port)
{
... (clipped for brevity) ..

	if (!request_region(port, AHC_EISA_IOSIZE, "aic7xxx"))
		return (ENOMEM);

... (clipped for brevity) ..
}

This may not be a problem, unless we're passing the value
outside the current module. So who calls this function?
$ grep -r -C20 aic7770_map_registers

Looks like it's only ever called in:
FILE: drivers/scsi/aic7xxx/aic7770.c
FUNC: aic7770_config

int aic7770_config(struct ahc_softc *ahc, struct aic7770_identity *entry, u_int io)
{
... (code and stuff) ...

	error = aic7770_map_registers(ahc, io);
	if (error != 0)
		return (error);

... (code and stuff) ...
}

Hmm... Let's see who calls this guy.
$ grep -r -C20 aic7770_config

This too is only called once, in:
FILE: drivers/scsi/aic7xxx/aic7770_osm.c
FUNC: aic7770_probe

static int
aic7770_probe(struct device *dev)
{
... (blah blah blah) ...

	error = aic7770_config(ahc, aic7770_ident_table + edev->id.driver_data,
			       eisaBase);
	if (error != 0) {
		ahc->bsh.ioport = 0;
		ahc_free(ahc);
		return (error);
	}

... (blah blah blah) ...
}

Same deal as before. Okay, so who calls aic7770_probe?

Well, as expected, no one calls it, at least not by name.
It's just the .probe function in the following struct:

FILE: drivers/scsi/aic7xxx/aic7770_osm.c

static struct eisa_driver aic7770_driver = {
	.id_table	= aic7770_ids,
	.driver = {
		.name   = "aic7xxx",
		.probe  = aic7770_probe,
		.remove = aic7770_remove,
	}
};

So the return code *is* being passed to another layer of the kernel,
in which case it should probably be negative.

There are lots of similar examples in the same driver. I'd be happy
to fix them up, but I thought I should send this patch first, to make
sure I'm not doing anything obviously wrong.

Ooh! One more thing, just to be safe. Above, I assumed that grepping
the kernel tree would give us all the places where an identifier shows
up, but maybe this driver does something clever with the preprocessor.

$ grep -r '##' drivers/scsi/aic7xxx/ || cowsay hooray
 ________
< hooray >
 --------
        \   ^__^
         \  (oo)\_______
            (__)\       )\/\
                ||----w |
                ||     ||

Okay good, so there shouldn't be any ## magic messing with our greps.
Alright, I guess I should send this patch off now. Thanks for reading,
and apologies in advance if I'm a moron :-)

Signed-off-by: Jason Wilkes <letshaveanadventure@gmail.com>
---
 drivers/scsi/aic7xxx/aic7770.c     | 2 +-
 drivers/scsi/aic7xxx/aic7770_osm.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/aic7xxx/aic7770.c b/drivers/scsi/aic7xxx/aic7770.c
index 5000bd6..cecdea9 100644
--- a/drivers/scsi/aic7xxx/aic7770.c
+++ b/drivers/scsi/aic7xxx/aic7770.c
@@ -171,7 +171,7 @@ aic7770_config(struct ahc_softc *ahc, struct aic7770_identity *entry, u_int io)
 		break;
 	default:
 		printk("aic7770_config: invalid irq setting %d\n", intdef);
-		return (ENXIO);
+		return -ENXIO;
 	}
 
 	if ((intdef & EDGE_TRIG) != 0)
diff --git a/drivers/scsi/aic7xxx/aic7770_osm.c b/drivers/scsi/aic7xxx/aic7770_osm.c
index 3d401d0..50f030d 100644
--- a/drivers/scsi/aic7xxx/aic7770_osm.c
+++ b/drivers/scsi/aic7xxx/aic7770_osm.c
@@ -51,7 +51,7 @@ aic7770_map_registers(struct ahc_softc *ahc, u_int port)
 	 * Lock out other contenders for our i/o space.
 	 */
 	if (!request_region(port, AHC_EISA_IOSIZE, "aic7xxx"))
-		return (ENOMEM);
+		return -ENOMEM;
 	ahc->tag = BUS_SPACE_PIO;
 	ahc->bsh.ioport = port;
 	return (0);
@@ -87,10 +87,10 @@ aic7770_probe(struct device *dev)
 	sprintf(buf, "ahc_eisa:%d", eisaBase >> 12);
 	name = kstrdup(buf, GFP_ATOMIC);
 	if (name == NULL)
-		return (ENOMEM);
+		return -ENOMEM;
 	ahc = ahc_alloc(&aic7xxx_driver_template, name);
 	if (ahc == NULL)
-		return (ENOMEM);
+		return -ENOMEM;
 	error = aic7770_config(ahc, aic7770_ident_table + edev->id.driver_data,
 			       eisaBase);
 	if (error != 0) {
-- 
2.1.3

Re: [PATCH] drivers: scsi: aic7xxx: Fix positive error codes.

[Hannes Reinecke]

On 12/03/2014 01:03 AM, Jason Wilkes wrote:
> Note:
> There are more instances of the problem described below, but I
> thought I'd explain the first one in detail, to make sure it's
> worth fixing the others (and to make sure I didn't do anything
> stupid, which I may have. I'm new to this :-). Alright, here we go!
> 
> A few drivers seem to return positive error codes internally, in
> order to signal something to a static function in the same driver.
> (see, for example, drivers/staging/lustre/lnet/lnet/lib-move.c)
> That's unusual, but seems okay as long as they're consistent between
> return codes and return code checks. However, a problem might arise
> if +ERRORCODE rather than -ERRORCODE is returned to other layers
> of the kernel (e.g., to the driver core). Let's do some exploring...
> 
> Here's a function that returns +ENOMEM
> FILE: drivers/scsi/aic7xxx/aic7770_osm.c
> FUNC: aic7770_map_registers
> 
> int
> aic7770_map_registers(struct ahc_softc *ahc, u_int port)
> {
> ... (clipped for brevity) ..
> 
> 	if (!request_region(port, AHC_EISA_IOSIZE, "aic7xxx"))
> 		return (ENOMEM);
> 
> ... (clipped for brevity) ..
> }
> 
> This may not be a problem, unless we're passing the value
> outside the current module. So who calls this function?
> $ grep -r -C20 aic7770_map_registers
> 
> Looks like it's only ever called in:
> FILE: drivers/scsi/aic7xxx/aic7770.c
> FUNC: aic7770_config
> 
> int aic7770_config(struct ahc_softc *ahc, struct aic7770_identity *entry, u_int io)
> {
> ... (code and stuff) ...
> 
> 	error = aic7770_map_registers(ahc, io);
> 	if (error != 0)
> 		return (error);
> 
> ... (code and stuff) ...
> }
> 
> Hmm... Let's see who calls this guy.
> $ grep -r -C20 aic7770_config
> 
> This too is only called once, in:
> FILE: drivers/scsi/aic7xxx/aic7770_osm.c
> FUNC: aic7770_probe
> 
> static int
> aic7770_probe(struct device *dev)
> {
> ... (blah blah blah) ...
> 
> 	error = aic7770_config(ahc, aic7770_ident_table + edev->id.driver_data,
> 			       eisaBase);
> 	if (error != 0) {
> 		ahc->bsh.ioport = 0;
> 		ahc_free(ahc);
> 		return (error);
> 	}
> 
> ... (blah blah blah) ...
> }
> 
> Same deal as before. Okay, so who calls aic7770_probe?
> 
> Well, as expected, no one calls it, at least not by name.
> It's just the .probe function in the following struct:
> 
> FILE: drivers/scsi/aic7xxx/aic7770_osm.c
> 
> static struct eisa_driver aic7770_driver = {
> 	.id_table	= aic7770_ids,
> 	.driver = {
> 		.name   = "aic7xxx",
> 		.probe  = aic7770_probe,
> 		.remove = aic7770_remove,
> 	}
> };
> 
> So the return code *is* being passed to another layer of the kernel,
> in which case it should probably be negative.
> 
> There are lots of similar examples in the same driver. I'd be happy
> to fix them up, but I thought I should send this patch first, to make
> sure I'm not doing anything obviously wrong.
> 
> Ooh! One more thing, just to be safe. Above, I assumed that grepping
> the kernel tree would give us all the places where an identifier shows
> up, but maybe this driver does something clever with the preprocessor.
> 
> $ grep -r '##' drivers/scsi/aic7xxx/ || cowsay hooray
>  ________
> < hooray >
>  --------
>         \   ^__^
>          \  (oo)\_______
>             (__)\       )\/\
>                 ||----w |
>                 ||     ||
> 
> Okay good, so there shouldn't be any ## magic messing with our greps.
> Alright, I guess I should send this patch off now. Thanks for reading,
> and apologies in advance if I'm a moron :-)
> 
> Signed-off-by: Jason Wilkes <letshaveanadventure@gmail.com>
> ---
>  drivers/scsi/aic7xxx/aic7770.c     | 2 +-
>  drivers/scsi/aic7xxx/aic7770_osm.c | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/aic7xxx/aic7770.c b/drivers/scsi/aic7xxx/aic7770.c
> index 5000bd6..cecdea9 100644
> --- a/drivers/scsi/aic7xxx/aic7770.c
> +++ b/drivers/scsi/aic7xxx/aic7770.c
> @@ -171,7 +171,7 @@ aic7770_config(struct ahc_softc *ahc, struct aic7770_identity *entry, u_int io)
>  		break;
>  	default:
>  		printk("aic7770_config: invalid irq setting %d\n", intdef);
> -		return (ENXIO);
> +		return -ENXIO;
>  	}
>  
>  	if ((intdef & EDGE_TRIG) != 0)
> diff --git a/drivers/scsi/aic7xxx/aic7770_osm.c b/drivers/scsi/aic7xxx/aic7770_osm.c
> index 3d401d0..50f030d 100644
> --- a/drivers/scsi/aic7xxx/aic7770_osm.c
> +++ b/drivers/scsi/aic7xxx/aic7770_osm.c
> @@ -51,7 +51,7 @@ aic7770_map_registers(struct ahc_softc *ahc, u_int port)
>  	 * Lock out other contenders for our i/o space.
>  	 */
>  	if (!request_region(port, AHC_EISA_IOSIZE, "aic7xxx"))
> -		return (ENOMEM);
> +		return -ENOMEM;
>  	ahc->tag = BUS_SPACE_PIO;
>  	ahc->bsh.ioport = port;
>  	return (0);
> @@ -87,10 +87,10 @@ aic7770_probe(struct device *dev)
>  	sprintf(buf, "ahc_eisa:%d", eisaBase >> 12);
>  	name = kstrdup(buf, GFP_ATOMIC);
>  	if (name == NULL)
> -		return (ENOMEM);
> +		return -ENOMEM;
>  	ahc = ahc_alloc(&aic7xxx_driver_template, name);
>  	if (ahc == NULL)
> -		return (ENOMEM);
> +		return -ENOMEM;
>  	error = aic7770_config(ahc, aic7770_ident_table + edev->id.driver_data,
>  			       eisaBase);
>  	if (error != 0) {
> 
Looks okay so far, but have you validated all callers to ensure they
use the new syntax?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)

Re: [PATCH] drivers: scsi: aic7xxx: Fix positive error codes.

[Jason Wilkes]

I'm pretty sure I did. I'd normally just say "yes I'm sure," but the
kernel folks do extremely clever things with the compiler, linker, and
basically everything else, so I'm reluctant to assume that *anything*
I know about C in userspace or C in less-cleverly-written code holds
here. To that end, I'll just describe why I think that I did what you
asked, so you'll be able to tell whether my reasoning is flawed, or
whether I totally misinterpreted what you said. I promise I won't
always be this cautious/obnoxious, but it seems like the responsible
thing to do for my first kernel patch.

Okay, here's why I think I did what you asked.

For each function foo in which I changed a return code:
(1) at most one other function calls foo, and
(2) all callers of foo (i.e., just one) only call it like this:

error = foo(args);
if (error != 0) {
        [possibly some cleanup];
        return (error);
}

Because of this, all the return codes I changed should only ever be
passed up the stack until we hit the driver core (I think).
They're never passed to some other function in the same driver that
checks their specific return value in order to decide how to behave.
All that's ever checked is whether they're nonzero.

Here are some details, which you can skip if the above is sufficient.
If you feel like skipping the details, goto out; (see below).

### Functions I modified, and how they're called: ###

# aic7770_map_registers: only called by aic7770_config.
# Here's how that call happens:
error = aic7770_map_registers(ahc, io);
if (error != 0)
        return (error);

# aic7770_config: only called by aic7770_probe.
# Here's how that call happens:
error = aic7770_config(ahc, aic7770_ident_table +
edev->id.driver_data, eisaBase);
if (error != 0) {
        ahc->bsh.ioport = 0;
        ahc_free(ahc);
        return (error);
}

# aic7770_probe: never called directly. It's presumably only called by
the driver core, in some line of the general form:

ret = drv->probe(dev);

Since the driver core doesn't know about this particular driver's
unusual behavior of sometimes returning positive error codes, the fact
that drv->probe(dev) (or however probe ends up getting called) may be
positive is arguably a bug.

out:

Alrighty! Sorry for the verbosity, but hopefully that answered your question.
One more caveat: I don't have this hardware, so if by "validate" you
meant "test on real hardware," then no. If that fact is sufficient for
you to drop the patch, I totally understand. However, I've watched a
bunch of Greg KH's videos, and I got the general impression that
simple fixes (like the above) are okay to submit if you don't have the
hardware. Let me know if that's not the case.

I just figured that since the driver is already inconsistent in its
use of return codes, fixing a self-contained subset of them would be a
good way to learn how to submit kernel patches.

If you got this far, thanks for reading :-)
-Jason

Re: [PATCH] drivers: scsi: aic7xxx: Fix positive error codes.

[Christoph Hellwig]

Hannes, does the patch looks good to you with this explanation?

On Wed, Dec 03, 2014 at 01:34:42PM -0800, Jason Wilkes wrote:
> I'm pretty sure I did. I'd normally just say "yes I'm sure," but the
> kernel folks do extremely clever things with the compiler, linker, and
> basically everything else, so I'm reluctant to assume that *anything*
> I know about C in userspace or C in less-cleverly-written code holds
> here. To that end, I'll just describe why I think that I did what you
> asked, so you'll be able to tell whether my reasoning is flawed, or
> whether I totally misinterpreted what you said. I promise I won't
> always be this cautious/obnoxious, but it seems like the responsible
> thing to do for my first kernel patch.
> 
> Okay, here's why I think I did what you asked.
> 
> For each function foo in which I changed a return code:
> (1) at most one other function calls foo, and
> (2) all callers of foo (i.e., just one) only call it like this:
> 
> error = foo(args);
> if (error != 0) {
>         [possibly some cleanup];
>         return (error);
> }
> 
> Because of this, all the return codes I changed should only ever be
> passed up the stack until we hit the driver core (I think).
> They're never passed to some other function in the same driver that
> checks their specific return value in order to decide how to behave.
> All that's ever checked is whether they're nonzero.
> 
> Here are some details, which you can skip if the above is sufficient.
> If you feel like skipping the details, goto out; (see below).
> 
> ### Functions I modified, and how they're called: ###
> 
> # aic7770_map_registers: only called by aic7770_config.
> # Here's how that call happens:
> error = aic7770_map_registers(ahc, io);
> if (error != 0)
>         return (error);
> 
> # aic7770_config: only called by aic7770_probe.
> # Here's how that call happens:
> error = aic7770_config(ahc, aic7770_ident_table +
> edev->id.driver_data, eisaBase);
> if (error != 0) {
>         ahc->bsh.ioport = 0;
>         ahc_free(ahc);
>         return (error);
> }
> 
> # aic7770_probe: never called directly. It's presumably only called by
> the driver core, in some line of the general form:
> 
> ret = drv->probe(dev);
> 
> Since the driver core doesn't know about this particular driver's
> unusual behavior of sometimes returning positive error codes, the fact
> that drv->probe(dev) (or however probe ends up getting called) may be
> positive is arguably a bug.
> 
> out:
> 
> Alrighty! Sorry for the verbosity, but hopefully that answered your question.
> One more caveat: I don't have this hardware, so if by "validate" you
> meant "test on real hardware," then no. If that fact is sufficient for
> you to drop the patch, I totally understand. However, I've watched a
> bunch of Greg KH's videos, and I got the general impression that
> simple fixes (like the above) are okay to submit if you don't have the
> hardware. Let me know if that's not the case.
> 
> I just figured that since the driver is already inconsistent in its
> use of return codes, fixing a self-contained subset of them would be a
> good way to learn how to submit kernel patches.
> 
> If you got this far, thanks for reading :-)
> -Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
---end quoted text---