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@xxxxxxxxx>
> ---
> 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@xxxxxxx +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)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
