Message-ID: <CAFKz9WbOwv+f7QcKe7sKf_9ZqWZHVkgPvK7G2aoga_EzHtct9Q@mail.gmail.com>
Date:	Wed, 3 Dec 2014 13:34:42 -0800
From:	Jason Wilkes <letshaveanadventure@...il.com>
To:	Hannes Reinecke <hare@...e.de>
Cc:	JBottomley@...allels.com, linux-scsi@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drivers: scsi: aic7xxx: Fix positive error codes.

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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists