Re: [GIT PULL] SCSI fixes for 2.6.32-rc3
Linus Torvalds
On Tue, 6 Oct 2009, James Bottomley wrote:
>
> This is mostly fixes. However, it contains two new drivers: Brocade SAS
> (bfa), the Bladengine2 iSCSI (be2iscsi) under the merge window exemption
Btw, I'm getting less excited about the merge window exemption.
It makes sense for consumer devices that people actually hit and that are
needed for bringup (ie they make a difference between a system that can be
installed and used, and one that cannot), but I'm not at all sure it makes
sense for things like this.
The _reason_ for the driver exemption was the fact that even a broken
driver is better than no driver at all for somebody who just can't get a
working system without it, but that argument really goes away when the
driver is so specialized that it's not about regular hardware any more.
And the whole "driver exemption" seems to have become a by-word for "I can
ignore the merge window for 50% of my code". Which makes me very tired of
it if there aren't real advantages to real users.
So I'm seriously considering a "the driver has to be mass market and also
actually matter to an install" rule for the exemption to be valid.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Randy Dunlap
> On Tue, 2009-10-06 at 08:58 -0700, Linus Torvalds wrote:
> >
> > On Tue, 6 Oct 2009, James Bottomley wrote:
> > >
> > > This is mostly fixes. However, it contains two new drivers: Brocade SAS
> > > (bfa), the Bladengine2 iSCSI (be2iscsi) under the merge window exemption
> >
> > Btw, I'm getting less excited about the merge window exemption.
> >
> > It makes sense for consumer devices that people actually hit and that are
> > needed for bringup (ie they make a difference between a system that can be
> > installed and used, and one that cannot), but I'm not at all sure it makes
> > sense for things like this.
> >
> > The _reason_ for the driver exemption was the fact that even a broken
> > driver is better than no driver at all for somebody who just can't get a
> > working system without it, but that argument really goes away when the
> > driver is so specialized that it's not about regular hardware any more.
>
> OK, so I don't see a huge distinction here. This is a driver for a
> piece of enterprise HW that Linux previously didn't support. To someone
> cursing not being able to use there hardware, it's every bit as
> important as the latest wireless driver.
>
> > And the whole "driver exemption" seems to have become a by-word for "I can
> > ignore the merge window for 50% of my code". Which makes me very tired of
> > it if there aren't real advantages to real users.
>
> While the exemption exists, I can certainly ignore the merge window for
> new drivers, yes ... however, that's not 50% of the code submitted in
> the merge window, and it's one time ... the same driver follows the
> merge window for the next release. I try to police this pretty rigidly
> in SCSI ... as you do at the top.
polices it so rigidly that even simple kernel-doc patches sit in a
scsi git tree for months. :(
---
~Randy
James Bottomley
>
> >
> > This is mostly fixes. However, it contains two new drivers: Brocade SAS
> > (bfa), the Bladengine2 iSCSI (be2iscsi) under the merge window exemption
>
> Btw, I'm getting less excited about the merge window exemption.
>
> It makes sense for consumer devices that people actually hit and that are
> needed for bringup (ie they make a difference between a system that can be
> installed and used, and one that cannot), but I'm not at all sure it makes
> sense for things like this.
>
> The _reason_ for the driver exemption was the fact that even a broken
> driver is better than no driver at all for somebody who just can't get a
> working system without it, but that argument really goes away when the
> driver is so specialized that it's not about regular hardware any more.
OK, so I don't see a huge distinction here. This is a driver for a
piece of enterprise HW that Linux previously didn't support. To someone
cursing not being able to use there hardware, it's every bit as
important as the latest wireless driver.
> And the whole "driver exemption" seems to have become a by-word for "I can
> ignore the merge window for 50% of my code". Which makes me very tired of
> it if there aren't real advantages to real users.
While the exemption exists, I can certainly ignore the merge window for
new drivers, yes ... however, that's not 50% of the code submitted in
the merge window, and it's one time ... the same driver follows the
merge window for the next release. I try to police this pretty rigidly
in SCSI ... as you do at the top.
In fact, for SCSI, these two drivers are the third and fourth merge
window exemptions in our year long history of allowing this.
> So I'm seriously considering a "the driver has to be mass market and also
> actually matter to an install" rule for the exemption to be valid.
OK, so on the policy, let me argue against the above. One of the things
we've been saying about linux is that we facilitate rapid adoption of
new hardware (and that we support more hardware than any other OS). The
Merge window exemption was adopted at the kernel summit last year
specifically to speed our adoption of new hardware. I think it's
valuable for this speed of adoption to be *all* hardware, not
specifically mass market laptop type stuff.
However, even if you want to change the definition, can we please not do
it retroactively?
Thanks,
James
James Bottomley
So what do you want to do about this? I need the fixes in this tree to
go forwards even if you don't want the new drivers ... I also now have a
list of other fixes to put in the next round which I'd like to get into
linux-next but while this is unresolved I can't really add more stuff to
my rc-fixes tree.
Linus Torvalds
On Thu, 8 Oct 2009, James Bottomley wrote:
>
> So what do you want to do about this?
I'm taking it (and the parisc one I was also unhappy with), but I'm a bit
grumpy as usual. The parisc pull came totally outside the merge window,
and the SCSI fix pull is technically perfectly fine, but what makes me
grumpy is that I get the strong feeling that people aren't even _trying_
to hit the merge window with new drivers, because they decide that they
instead can just push them any time.
So I don't think I necessarily want to change the "new driver" policy per
se, but I want people to see the merge window as the _primary_ time you
get any new code in. The "yes, we'll take new drivers" thing should be the
exception rather than the rule. It doesn't seem to be an exception.
Linus
James Bottomley
>
> On Thu, 8 Oct 2009, James Bottomley wrote:
> >
> > So what do you want to do about this?
>
> I'm taking it (and the parisc one I was also unhappy with), but I'm a bit
> grumpy as usual. The parisc pull came totally outside the merge window,
> and the SCSI fix pull is technically perfectly fine, but what makes me
> grumpy is that I get the strong feeling that people aren't even _trying_
> to hit the merge window with new drivers, because they decide that they
> instead can just push them any time.
OK, so we still have a bit of a mismatch here. I *do* tell people who
come to SCSI with new drivers that for the *first* submission they don't
need to worry about the merge window because of the new driver
exception. This allows us to clean them up ready for going in without
the submitter feeling huge pressure to hit the merge window rather than
concentrating on code quality and what we need to make a technically
correct submission.
I think this is the correct thing to do for new drivers (which often
come with new writers) because training people to hit the merge window
is often long an painful (when I say not yet to their important driver
enhancements) ... starting people off with the carrot is better than the
stick.
> So I don't think I necessarily want to change the "new driver" policy per
> se, but I want people to see the merge window as the _primary_ time you
> get any new code in. The "yes, we'll take new drivers" thing should be the
> exception rather than the rule. It doesn't seem to be an exception.
Like I said, this will be my fourth new driver under the exception in
the whole year it's been going. On the other hand, I do have pm8001
still going ... it just got resubmitted after another round of fixes.
If it's ready for the next rc submissions round, it will be my fifth ...
James
Linus Torvalds
On Thu, 8 Oct 2009, Linus Torvalds wrote:
>
> I'm taking it (and the parisc one I was also unhappy with)
Actually, looking at it again, I'm wavering.
That BFA driver isn't a "driver". It's a huge subsystem of it's own. It's
almost 50 _thousand_ lines of code for just a single "driver", and for
rare hardware at that.
Quite frankly, the "bang per line" is almost zero.
What the ^&@* is wrong with "enterprise SCSI" people? The amount of crazy
is overwhelming.
So I've pulled it, but I'm still considering just unpulling it. That
driver is _not_ "just a driver". It's something more. Something dank and
smelly, that has grown in dark and forbidding places.
The whole crazy "high end SCSI" industry needs a f*cking exorcism.
Even if I don't unpull, I don't _ever_ want to see a driver like this
outside the merge window. And dammit, James, you should have realized
that.
James Bottomley
>
> On Thu, 8 Oct 2009, Linus Torvalds wrote:
> >
> > I'm taking it (and the parisc one I was also unhappy with)
>
> Actually, looking at it again, I'm wavering.
>
> That BFA driver isn't a "driver". It's a huge subsystem of it's own. It's
> almost 50 _thousand_ lines of code for just a single "driver", and for
> rare hardware at that.
It's a huge glue layer driver, like the aic7xxx, yes.
So the tradeoff here is that I estimate it would take years to get it to
where a linux driver should be.
Could I remind you that at the last kernel summit I was the one
advocating for holding drivers out of tree until they met our standards
and you were the one who told me not to do this ... Jon even captured
it:
https://lwn.net/Articles/298570/
I'll certainly offer this driver to Greg's drivers project to see if
they can unglue it ... I just think it's going to take a while.
> Quite frankly, the "bang per line" is almost zero.
>
> What the ^&@* is wrong with "enterprise SCSI" people? The amount of crazy
> is overwhelming.
>
> So I've pulled it, but I'm still considering just unpulling it. That
> driver is _not_ "just a driver". It's something more. Something dank and
> smelly, that has grown in dark and forbidding places.
>
> The whole crazy "high end SCSI" industry needs a f*cking exorcism.
>
> Even if I don't unpull, I don't _ever_ want to see a driver like this
> outside the merge window. And dammit, James, you should have realized
> that.
Well certainly in an ideal world ... and in fact the world before KS2008
I wouldn't have accepted it until it was well cleaned up.
James
Linus Torvalds
On Thu, 8 Oct 2009, James Bottomley wrote:
>
> Could I remind you that at the last kernel summit I was the one
> advocating for holding drivers out of tree until they met our standards
> and you were the one who told me not to do this ... Jon even captured
> it:
What the hell is wrong with you?
You're bringing up total red herrings that have nothing to do with
anything.
My point is:
- that's not just another driver. That's FIFTY THOUSANDS LINES OF
LARGELY INFRASTRUCTURE CRAP.
- I'm not arguing that we shouldn't merge the driver
- I'm arguing that you damn well should have used the merge window for
something like this!
What part of "it's already -rc3, and you're pushing 50kloc of crap that
almost nobody will care about" can you not understand?
What part of "merge window" do you have issues with?
What part of "sure, I'll take new drivers after the merge window, but COME
&*^@ ON!" do you have trouble understanding?
Stop bringing up totally irrelevant crap.
Linus
Linus Torvalds
On Thu, 8 Oct 2009, Linus Torvalds wrote:
>
> What part of "sure, I'll take new drivers after the merge window, but COME
> &*^@ ON!" do you have trouble understanding?
Put another way: look at the drivers/staging tree. Look at when it gets
merged. Do this:
git diff --stat v2.6.31-rc1..v2.6.31 drivers/staging/
and then do this:
git diff --stat v2.6.30..v2.6.31-rc1 drivers/staging/
adn thing about the fact that they are _all_ "new drivers". Then consider
the issue of "merge window" vs "not merge window".
It really boils down to the fact that I'm perfectly happy to let new
drivers slip in after the merge window in order to help end users. But
really - there has to be a limit to it. Not just anything.
It has to help end users, and dammit, you have to admit that 50 kloc is
damn well not just "another random driver".
James Bottomley
> On Thu, 8 Oct 2009, James Bottomley wrote:
> >
> > Could I remind you that at the last kernel summit I was the one
> > advocating for holding drivers out of tree until they met our standards
> > and you were the one who told me not to do this ... Jon even captured
> > it:
>
> What the hell is wrong with you?
>
> You're bringing up total red herrings that have nothing to do with
> anything.
>
> My point is:
> - that's not just another driver. That's FIFTY THOUSANDS LINES OF
> LARGELY INFRASTRUCTURE CRAP.
>
> - I'm not arguing that we shouldn't merge the driver
>
> - I'm arguing that you damn well should have used the merge window for
> something like this!
>
> What part of "it's already -rc3, and you're pushing 50kloc of crap that
> almost nobody will care about" can you not understand?
>
> What part of "merge window" do you have issues with?
OK, you're saying the merge window exemption should only apply to
drivers which meet our coding standards.
That's fine, I can live with that ... and that's how we'll operate in
future.
> What part of "sure, I'll take new drivers after the merge window, but COME
> &*^@ ON!" do you have trouble understanding?
What does the ^@ mean again?
> Stop bringing up totally irrelevant crap.
James
Linus Torvalds
On Thu, 8 Oct 2009, James Bottomley wrote:
>
> OK, you're saying the merge window exemption should only apply to
> drivers which meet our coding standards.
Well, to me, it's not even "coding standards". It's more about "letting
things slide so that users get their hands on things earlier, since it
can't really regress". Coding standards are obviously a part of that, but
I think the coding standard question should come into this mainly in the
sense of "should it go through staging or not" kind of sense, not in the
timing sense.
The reason I object to this driver at this point is that I really think
there's a _huge_ difference between some random average driver, and a 50
kloc monster driver that basically seems to implement its own protocol.
Most random new drivers tend to be a few hundred lines of code, in some
cases a few thousand. They don't generally bring in their own subsystem
code, they often just hook into existing things like the libata layer or
the network driver infrastructure etc.
So most drivers are in a totally different class than the one I'm
objecting to in the SCSI tree.
And I also really do think there is a huge difference between some
specialized high-end SCSI driver that is only relevant to enterprise
people and some more average driver that is expected to perhaps exist in
lots of consumer devices. How many people does it affect, and what's their
ability to handle it?
Another way of putting that "consumer" vs "enterprise" thing: how big is
the _upside_ of merging the driver outside fo the merge window? Again, I
simply think pure number of potential users matters for the "should we let
it slide" question.
Linus
Theodore Tso
>
> It really boils down to the fact that I'm perfectly happy to let new
> drivers slip in after the merge window in order to help end users. But
> really - there has to be a limit to it. Not just anything.
>
> It has to help end users, and dammit, you have to admit that 50 kloc is
> damn well not just "another random driver".
So would it be acceptable to merge the 50 kloc of crap _during_ the
merge window? Crap is crap, no matter when it is merged. In this
particular case, the crap is its own subsystem, and wouldn't interfere
with the rest of the kernel tree. So it's no more risky to merge it
outside of merge window; so the decision about whether merge 50 kloc
of crap shouldn't be impacted about whether we are in our out of the
merge window.
Arguably, the question is whether it's better for the end users to
merge this at _any_ time, or whether we force the manufacturer to drop
it into the staging tree provisionally until it can be cleaned up.
- Ted
Linus Torvalds
On Thu, 8 Oct 2009, Theodore Tso wrote:
>
> So would it be acceptable to merge the 50 kloc of crap _during_ the
> merge window?
Yes. I actually looked at the driver (since I had pulled it - I've
unpulled it but am still mulling it over), and while I think it looked
huge and overly complex, it by no means gave me the kinds of vibes I get
from some "obviously-ported-from-windows-with-no-clue" drivers.
So at least from my quick look I didn't get the feeling that the driver
was "evil". For me, it's a timing issue. I hate getting big pull requests
after -rc1 is out, and I really don't like the feeling that people are
just ignoring the merge window.
That said, if somebody wants to look more closely at the driver, and then
wants to convince people that it should have gone through "staging", feel
free. But that's not what I've personally been arguing about.
Linus
Ingo Molnar
* Linus Torvalds <torv...@linux-foundation.org> wrote:
> On Thu, 8 Oct 2009, Theodore Tso wrote:
> >
> > So would it be acceptable to merge the 50 kloc of crap _during_ the
> > merge window?
>
> Yes. I actually looked at the driver (since I had pulled it - I've
> unpulled it but am still mulling it over), and while I think it looked
> huge and overly complex, it by no means gave me the kinds of vibes I
> get from some "obviously-ported-from-windows-with-no-clue" drivers.
>
> So at least from my quick look I didn't get the feeling that the
> driver was "evil". For me, it's a timing issue. I hate getting big
> pull requests after -rc1 is out, and I really don't like the feeling
> that people are just ignoring the merge window.
>
> That said, if somebody wants to look more closely at the driver, and
> then wants to convince people that it should have gone through
> "staging", feel free. But that's not what I've personally been arguing
> about.
Greg, what's your take on the quality of this new driver? Do you have
some time to do a review of this with drivers/staging/ versus drivers/
glasses on? The Git URI is at:
master.kernel.org:/pub/scm/linux/kernel/git/jejb/scsi-rc-fixes-2.6 master
To me, after a very quick skimming of it it's borderline. The driver
looks like proper Linux code at first sight in places, but i can see
_lots_ of (as in thousands of) odd bits - at least odd for a newly
submitted driver:
- 200+ instances of bfa_boolean_t, which is defined via:
enum bfa_boolean {
BFA_FALSE = 0,
BFA_TRUE = 1
};
#define bfa_boolean_t enum bfa_boolean
that should be bool simply.
- the driver is full with misaligned vertical spacing like:
struct bfa_pcidev_s {
int pci_slot;
u8 pci_func;
u16 device_id;
bfa_os_addr_t pci_bar_kva;
};
void
bfa_timer_beat(struct bfa_timer_mod_s *mod)
{
struct list_head *qh = &mod->timer_q;
struct list_head *qe, *qe_next;
struct bfa_timer_s *elem;
struct list_head timedout_q;
which suggests that this driver was treated with a
de-ugly-ifying sed job without a human looking at the result. There's
over a thousand (!) of such instances in the driver.
- various forms of avoidable whitespace damage: for example 873
instances of 'space' character followed by 'tab'.
- bfa_timer.c looks weird - implements a naive timeout mechanism on top
of real timers? Why not use real timers in to begin with?
- Also, the .h files are layed out oddly. Bits of them are in
include/*, bits of them in *.h.
- the 90+ easily avoidable stylistic details attached below in
drivers/scsi/bfa/fcbuild.c.
- accesses to cmnd->host_scribble both seem an ancient method, and
are also somewhat SMP-barrier-unclean. (i'm sure it works and is
correct in practice as there are heavy, serializing functions around
those places, but the casts still look ugly and there are no
barriers.)
- The 290+ instances of bfa_assert() uses should probably be BUG_ON()s
or WARN_ON()s instead of a wrapped panic. Nothing ever defines
BFA_PERF_BUILD so the wrapping seems removable.
havent looked further and these are all easily addressed by just looking
at the code and doing fixes that dont impact the resulting .o - so very
easy to do en masse.
I dont know what's the current mainline inclusion quality threshold for
non-staging Linux drivers - it might be ok. Also, the driver commit has
been rebased a few days ago which makes it hard to see its stability
track record.
Ingo
--------------->
ERROR: return is not a function, parentheses are not required
#191: FILE: fcbuild.c:191:
+ return (FC_PARSE_BUSY);
ERROR: return is not a function, parentheses are not required
#193: FILE: fcbuild.c:193:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#196: FILE: fcbuild.c:196:
+ return (FC_PARSE_OK);
ERROR: return is not a function, parentheses are not required
#198: FILE: fcbuild.c:198:
+ return (FC_PARSE_OK);
CHECK: multiple assignments should be avoided
#226: FILE: fcbuild.c:226:
+ plogi->csp.rxsz = plogi->class3.rxsz = bfa_os_htons(pdu_size);
ERROR: return is not a function, parentheses are not required
#231: FILE: fcbuild.c:231:
+ return (sizeof(struct fc_logi_s));
CHECK: multiple assignments should be avoided
#248: FILE: fcbuild.c:248:
+ flogi->csp.rxsz = flogi->class3.rxsz = bfa_os_htons(pdu_size);
ERROR: return is not a function, parentheses are not required
#270: FILE: fcbuild.c:270:
+ return (sizeof(struct fc_logi_s));
CHECK: multiple assignments should be avoided
#284: FILE: fcbuild.c:284:
+ flogi->csp.rxsz = flogi->class3.rxsz = bfa_os_htons(pdu_size);
ERROR: return is not a function, parentheses are not required
#290: FILE: fcbuild.c:290:
+ return (sizeof(struct fc_logi_s));
CHECK: multiple assignments should be avoided
#305: FILE: fcbuild.c:305:
+ flogi->csp.rxsz = flogi->class3.rxsz = bfa_os_htons(pdu_size);
ERROR: return is not a function, parentheses are not required
#309: FILE: fcbuild.c:309:
+ return (sizeof(struct fc_logi_s));
ERROR: return is not a function, parentheses are not required
#341: FILE: fcbuild.c:341:
+ return (FC_PARSE_BUSY);
ERROR: return is not a function, parentheses are not required
#343: FILE: fcbuild.c:343:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#347: FILE: fcbuild.c:347:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#350: FILE: fcbuild.c:350:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#353: FILE: fcbuild.c:353:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#356: FILE: fcbuild.c:356:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#358: FILE: fcbuild.c:358:
+ return (FC_PARSE_OK);
ERROR: return is not a function, parentheses are not required
#360: FILE: fcbuild.c:360:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#375: FILE: fcbuild.c:375:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#396: FILE: fcbuild.c:396:
+ return (sizeof(struct fc_prli_s));
ERROR: return is not a function, parentheses are not required
#417: FILE: fcbuild.c:417:
+ return (sizeof(struct fc_prli_s));
ERROR: return is not a function, parentheses are not required
#424: FILE: fcbuild.c:424:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#427: FILE: fcbuild.c:427:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#431: FILE: fcbuild.c:431:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#434: FILE: fcbuild.c:434:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#436: FILE: fcbuild.c:436:
+ return (FC_PARSE_OK);
ERROR: return is not a function, parentheses are not required
#443: FILE: fcbuild.c:443:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#446: FILE: fcbuild.c:446:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#449: FILE: fcbuild.c:449:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#451: FILE: fcbuild.c:451:
+ return (FC_PARSE_OK);
ERROR: return is not a function, parentheses are not required
#465: FILE: fcbuild.c:465:
+ return (sizeof(struct fc_logo_s));
ERROR: return is not a function, parentheses are not required
#487: FILE: fcbuild.c:487:
+ return (sizeof(struct fc_adisc_s));
ERROR: return is not a function, parentheses are not required
#514: FILE: fcbuild.c:514:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#517: FILE: fcbuild.c:517:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#520: FILE: fcbuild.c:520:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#522: FILE: fcbuild.c:522:
+ return (FC_PARSE_OK);
ERROR: return is not a function, parentheses are not required
#532: FILE: fcbuild.c:532:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#537: FILE: fcbuild.c:537:
+ return (FC_PARSE_OK);
ERROR: return is not a function, parentheses are not required
#539: FILE: fcbuild.c:539:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#553: FILE: fcbuild.c:553:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#556: FILE: fcbuild.c:556:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#559: FILE: fcbuild.c:559:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#573: FILE: fcbuild.c:573:
+ return (sizeof(struct fchs_s));
ERROR: return is not a function, parentheses are not required
#581: FILE: fcbuild.c:581:
+ return (FC_PARSE_OK);
ERROR: return is not a function, parentheses are not required
#583: FILE: fcbuild.c:583:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#600: FILE: fcbuild.c:600:
+ return (sizeof(struct fc_rrq_s));
ERROR: return is not a function, parentheses are not required
#614: FILE: fcbuild.c:614:
+ return (sizeof(struct fc_els_cmd_s));
ERROR: return is not a function, parentheses are not required
#630: FILE: fcbuild.c:630:
+ return (sizeof(struct fc_ls_rjt_s));
ERROR: return is not a function, parentheses are not required
#646: FILE: fcbuild.c:646:
+ return (sizeof(struct fc_ba_acc_s));
ERROR: return is not a function, parentheses are not required
#657: FILE: fcbuild.c:657:
+ return (sizeof(struct fc_els_cmd_s));
ERROR: return is not a function, parentheses are not required
#699: FILE: fcbuild.c:699:
+ return (bfa_os_ntohs(tprlo_acc->payload_len));
ERROR: return is not a function, parentheses are not required
#724: FILE: fcbuild.c:724:
+ return (bfa_os_ntohs(prlo_acc->payload_len));
ERROR: return is not a function, parentheses are not required
#738: FILE: fcbuild.c:738:
+ return (sizeof(struct fc_rnid_cmd_s));
ERROR: return is not a function, parentheses are not required
#762: FILE: fcbuild.c:762:
+ return (sizeof(struct fc_rnid_acc_s));
ERROR: return is not a function, parentheses are not required
#764: FILE: fcbuild.c:764:
+ return (sizeof(struct fc_rnid_acc_s) -
ERROR: return is not a function, parentheses are not required
#779: FILE: fcbuild.c:779:
+ return (sizeof(struct fc_rpsc_cmd_s));
ERROR: return is not a function, parentheses are not required
#800: FILE: fcbuild.c:800:
+ return (sizeof(struct fc_rpsc2_cmd_s) + ((npids - 1) *
ERROR: return is not a function, parentheses are not required
#822: FILE: fcbuild.c:822:
+ return (sizeof(struct fc_rpsc_acc_s));
CHECK: multiple assignments should be avoided
#855: FILE: fcbuild.c:855:
+ pdisc->csp.rxsz = pdisc->class3.rxsz = bfa_os_htons(pdu_size);
ERROR: return is not a function, parentheses are not required
#859: FILE: fcbuild.c:859:
+ return (sizeof(struct fc_logi_s));
ERROR: return is not a function, parentheses are not required
#868: FILE: fcbuild.c:868:
+ return (FC_PARSE_LEN_INVAL);
ERROR: return is not a function, parentheses are not required
#871: FILE: fcbuild.c:871:
+ return (FC_PARSE_ACC_INVAL);
ERROR: return is not a function, parentheses are not required
#874: FILE: fcbuild.c:874:
+ return (FC_PARSE_PWWN_NOT_EQUAL);
ERROR: return is not a function, parentheses are not required
#877: FILE: fcbuild.c:877:
+ return (FC_PARSE_NWWN_NOT_EQUAL);
ERROR: return is not a function, parentheses are not required
#880: FILE: fcbuild.c:880:
+ return (FC_PARSE_RXSZ_INVAL);
ERROR: return is not a function, parentheses are not required
#882: FILE: fcbuild.c:882:
+ return (FC_PARSE_OK);
ERROR: return is not a function, parentheses are not required
#906: FILE: fcbuild.c:906:
+ return (bfa_os_ntohs(prlo->payload_len));
ERROR: return is not a function, parentheses are not required
#919: FILE: fcbuild.c:919:
+ return (FC_PARSE_FAILURE);
ERROR: return is not a function, parentheses are not required
#939: FILE: fcbuild.c:939:
+ return (FC_PARSE_OK);
ERROR: return is not a function, parentheses are not required
#971: FILE: fcbuild.c:971:
+ return (bfa_os_ntohs(tprlo->payload_len));
ERROR: return is not a function, parentheses are not required
#984: FILE: fcbuild.c:984:
+ return (FC_PARSE_ACC_INVAL);
ERROR: return is not a function, parentheses are not required
#990: FILE: fcbuild.c:990:
+ return (FC_PARSE_NOT_FCP);
ERROR: return is not a function, parentheses are not required
#992: FILE: fcbuild.c:992:
+ return (FC_PARSE_OPAFLAG_INVAL);
ERROR: return is not a function, parentheses are not required
#994: FILE: fcbuild.c:994:
+ return (FC_PARSE_RPAFLAG_INVAL);
ERROR: return is not a function, parentheses are not required
#996: FILE: fcbuild.c:996:
+ return (FC_PARSE_OPA_INVAL);
ERROR: return is not a function, parentheses are not required
#998: FILE: fcbuild.c:998:
+ return (FC_PARSE_RPA_INVAL);
ERROR: return is not a function, parentheses are not required
#1000: FILE: fcbuild.c:1000:
+ return (FC_PARSE_OK);
ERROR: return is not a function, parentheses are not required
#1027: FILE: fcbuild.c:1027:
+ return (sizeof(struct fc_ba_rjt_s));
ERROR: return is not a function, parentheses are not required
#1076: FILE: fcbuild.c:1076:
+ return (sizeof(struct fcgs_gidpn_req_s) + sizeof(struct ct_hdr_s));
ERROR: return is not a function, parentheses are not required
#1093: FILE: fcbuild.c:1093:
+ return (sizeof(fcgs_gpnid_req_t) + sizeof(struct ct_hdr_s));
ERROR: return is not a function, parentheses are not required
#1110: FILE: fcbuild.c:1110:
+ return (sizeof(fcgs_gnnid_req_t) + sizeof(struct ct_hdr_s));
ERROR: return is not a function, parentheses are not required
#1140: FILE: fcbuild.c:1140:
+ return (sizeof(struct fc_scr_s));
ERROR: return is not a function, parentheses are not required
#1160: FILE: fcbuild.c:1160:
+ return (sizeof(struct fc_rscn_pl_s));
ERROR: return is not a function, parentheses are not required
#1191: FILE: fcbuild.c:1191:
+ return (sizeof(struct fcgs_rftid_req_s) + sizeof(struct ct_hdr_s));
ERROR: return is not a function, parentheses are not required
#1213: FILE: fcbuild.c:1213:
+ return (sizeof(struct fcgs_rftid_req_s) + sizeof(struct ct_hdr_s));
ERROR: return is not a function, parentheses are not required
#1234: FILE: fcbuild.c:1234:
+ return (sizeof(struct fcgs_rffid_req_s) + sizeof(struct ct_hdr_s));
ERROR: return is not a function, parentheses are not required
#1256: FILE: fcbuild.c:1256:
+ return (sizeof(struct fcgs_rspnid_req_s) + sizeof(struct ct_hdr_s));
ERROR: return is not a function, parentheses are not required
#1278: FILE: fcbuild.c:1278:
+ return (sizeof(struct fcgs_gidft_req_s) + sizeof(struct ct_hdr_s));
ERROR: return is not a function, parentheses are not required
#1297: FILE: fcbuild.c:1297:
+ return (sizeof(struct fcgs_rpnid_req_s) + sizeof(struct ct_hdr_s));
ERROR: return is not a function, parentheses are not required
#1316: FILE: fcbuild.c:1316:
+ return (sizeof(struct fcgs_rnnid_req_s) + sizeof(struct ct_hdr_s));
ERROR: return is not a function, parentheses are not required
#1335: FILE: fcbuild.c:1335:
+ return (sizeof(struct fcgs_rcsid_req_s) + sizeof(struct ct_hdr_s));
ERROR: return is not a function, parentheses are not required
#1354: FILE: fcbuild.c:1354:
+ return (sizeof(struct fcgs_rptid_req_s) + sizeof(struct ct_hdr_s));
ERROR: return is not a function, parentheses are not required
#1371: FILE: fcbuild.c:1371:
+ return (sizeof(struct ct_hdr_s) + sizeof(struct fcgs_ganxt_req_s));
ERROR: return is not a function, parentheses are not required
#1388: FILE: fcbuild.c:1388:
+ return (sizeof(struct ct_hdr_s));
ERROR: return is not a function, parentheses are not required
#1428: FILE: fcbuild.c:1428:
+ return (sizeof(struct ct_hdr_s) + sizeof(fcgs_gmal_req_t));
ERROR: return is not a function, parentheses are not required
#1448: FILE: fcbuild.c:1448:
+ return (sizeof(struct ct_hdr_s) + sizeof(fcgs_gfn_req_t));
total: 93 errors, 0 warnings, 5 checks, 1449 lines checked
fcbuild.c has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Daniel Walker
>
> I dont know what's the current mainline inclusion quality threshold for
> non-staging Linux drivers - it might be ok. Also, the driver commit has
> been rebased a few days ago which makes it hard to see its stability
> track record.
>
> Ingo
>
> --------------->
> ERROR: return is not a function, parentheses are not required
> #191: FILE: fcbuild.c:191:
> + return (FC_PARSE_BUSY);
The author submitted a basic style clean up here,
http://patchwork.kernel.org/patch/50161/
It was a series that did the whole driver.. I'm not sure why those
patches aren't included, but at least there was an attempt at cleaning
the driver up..
Daniel
James Bottomley
>
> > On Thu, 8 Oct 2009, Theodore Tso wrote:
> > >
> > > So would it be acceptable to merge the 50 kloc of crap _during_ the
> > > merge window?
> >
> > Yes. I actually looked at the driver (since I had pulled it - I've
> > unpulled it but am still mulling it over), and while I think it looked
> > huge and overly complex, it by no means gave me the kinds of vibes I
> > get from some "obviously-ported-from-windows-with-no-clue" drivers.
> >
> > So at least from my quick look I didn't get the feeling that the
> > driver was "evil". For me, it's a timing issue. I hate getting big
> > pull requests after -rc1 is out, and I really don't like the feeling
> > that people are just ignoring the merge window.
> >
> > That said, if somebody wants to look more closely at the driver, and
> > then wants to convince people that it should have gone through
> > "staging", feel free. But that's not what I've personally been arguing
> > about.
>
> Greg, what's your take on the quality of this new driver? Do you have
> some time to do a review of this with drivers/staging/ versus drivers/
> glasses on? The Git URI is at:
To me, the matter of staging versus actual tree isn't a quality issue
(otherwise we'd be shifting ~75% of SCSI drivers to staging, depending
on whose view of "quality" was being used). It's an ABI issue. If we
would have to change the user visible ABI while the driver was being
cleaned up, I'd want it in staging to warn users to expect these
problems. Although we couldn't clean up everything, I did make sure
this driver plugs correctly into the standard linux FC ABI before
putting it in the SCSI tree, so there are no ABI changes anticipated
even though there will likely be a lot of code changes. Therefore, the
correct clean up path for this one is through the SCSI tree.
James
Greg KH
> On Fri, 2009-10-09 at 11:15 +0200, Ingo Molnar wrote:
> > * Linus Torvalds <torv...@linux-foundation.org> wrote:
> >
> > > On Thu, 8 Oct 2009, Theodore Tso wrote:
> > > >
> > > > So would it be acceptable to merge the 50 kloc of crap _during_ the
> > > > merge window?
> > >
> > > Yes. I actually looked at the driver (since I had pulled it - I've
> > > unpulled it but am still mulling it over), and while I think it looked
> > > huge and overly complex, it by no means gave me the kinds of vibes I
> > > get from some "obviously-ported-from-windows-with-no-clue" drivers.
> > >
> > > So at least from my quick look I didn't get the feeling that the
> > > driver was "evil". For me, it's a timing issue. I hate getting big
> > > pull requests after -rc1 is out, and I really don't like the feeling
> > > that people are just ignoring the merge window.
> > >
> > > That said, if somebody wants to look more closely at the driver, and
> > > then wants to convince people that it should have gone through
> > > "staging", feel free. But that's not what I've personally been arguing
> > > about.
> >
> > Greg, what's your take on the quality of this new driver? Do you have
> > some time to do a review of this with drivers/staging/ versus drivers/
> > glasses on? The Git URI is at:
>
> To me, the matter of staging versus actual tree isn't a quality issue
> (otherwise we'd be shifting ~75% of SCSI drivers to staging, depending
> on whose view of "quality" was being used).
Great, I'll be glad to take them :)
> It's an ABI issue. If we would have to change the user visible ABI
> while the driver was being cleaned up, I'd want it in staging to warn
> users to expect these problems. Although we couldn't clean up
> everything, I did make sure this driver plugs correctly into the
> standard linux FC ABI before putting it in the SCSI tree, so there are
> no ABI changes anticipated even though there will likely be a lot of
> code changes. Therefore, the correct clean up path for this one is
> through the SCSI tree.
Ok, that's your call as the scsi maintainer.
If you need anything to go into staging as an incentive to get the code
cleaned up, or if not, dropped, just send it to me.
thanks,
greg k-h
James Bottomley
>
> On Thu, 8 Oct 2009, James Bottomley wrote:
> >
> > OK, you're saying the merge window exemption should only apply to
> > drivers which meet our coding standards.
>
> Well, to me, it's not even "coding standards". It's more about "letting
> things slide so that users get their hands on things earlier, since it
> can't really regress". Coding standards are obviously a part of that, but
> I think the coding standard question should come into this mainly in the
> sense of "should it go through staging or not" kind of sense, not in the
> timing sense.
>
> The reason I object to this driver at this point is that I really think
> there's a _huge_ difference between some random average driver, and a 50
> kloc monster driver that basically seems to implement its own protocol.
The protocol it's implementing is a Fibre Channel state engine (it is a
FC driver, after all). We have an embryonic project in SCSI to do this:
libfc. libfc was separated from the fcoe project when we thought it
might prove useful. However, it isn't quite production ready yet, but I
do have agreement that the bca driver will move to it in the near
future.
Just for comparison with other enterprise FC drivers:
qla2xxx: 30k LOC
lpfc: 58k LOC
so it's not even our biggest FC driver.
The other reason for the big state engine is that this isn't a fat
firmware (everything done inside the firmware of the card) type driver.
We like thin firmware hardware because it's easier to fix when things go
wrong.
If you give the perception of penalising drivers for being "huge" you're
sending the message to hardware manufacturers that they'll have an
easier time of it if they simply shove all the nasty bits into firmware
and present the kernel community with a tiny driver for a bloated fat
firmware card.
> Most random new drivers tend to be a few hundred lines of code, in some
> cases a few thousand. They don't generally bring in their own subsystem
> code, they often just hook into existing things like the libata layer or
> the network driver infrastructure etc.
Right, and in this case, it will eventually plug into libfc ... we just
haven't quite got the infrastructure ready yet.
The functional infrastructure we need it to plug into for the userspace
ABI: scsi_transport_fc is all there and working.
> So most drivers are in a totally different class than the one I'm
> objecting to in the SCSI tree.
>
> And I also really do think there is a huge difference between some
> specialized high-end SCSI driver that is only relevant to enterprise
> people and some more average driver that is expected to perhaps exist in
> lots of consumer devices. How many people does it affect, and what's their
> ability to handle it?
Actually, larger than you would think. Our primary customer at
distributions is the enterprise. Now, you may say that enterprise
distro users aren't at the bleeding edge of kernel development.
However, we use the club of not being backported to a distro until
you're upstream to try and force driver writers/hardware vendors to
engage with the community. The carrot to them is that as soon as they
make it upstream, the distros will incorporate the driver. This makes
the merge window exemption an important part of that carrot, since
otherwise they might find themselves on the wrong side of a three month
cycle.
> Another way of putting that "consumer" vs "enterprise" thing: how big is
> the _upside_ of merging the driver outside fo the merge window? Again, I
> simply think pure number of potential users matters for the "should we let
> it slide" question.
The upside is that I can use this as an inducement to other driver
writers for good behaviour, plus the bfa driver can get picked up by the
enterprise distributions early. To people engaged in convincing
hardware vendors to work upstream and release early and often, that's a
very big upside.
James