LinuxLists.cc - [GIT PULL] SCSI fixes for 2.6.32-rc3

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

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

2009-10-06 20:54:49

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

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.

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

2009-10-06 21:05:30

by Randy Dunlap

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

On Tue, 06 Oct 2009 20:54:02 +0000 James Bottomley wrote:

> 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

2009-10-08 14:34:03

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

On Tue, 2009-10-06 at 20:54 +0000, James Bottomley wrote:
> 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.
>
> 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?

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.

James

2009-10-08 14:40:18

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

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

2009-10-08 14:55:12

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

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.

Linus

2009-10-08 14:57:35

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

On Thu, 2009-10-08 at 07:39 -0700, Linus Torvalds wrote:
>
> 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

2009-10-08 19:49:06

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

On Thu, 2009-10-08 at 07:54 -0700, Linus Torvalds wrote:
>
> 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

2009-10-08 19:55:53

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

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

2009-10-08 20:01:43

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

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".

Linus

2009-10-08 20:04:59

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

On Thu, 2009-10-08 at 12:55 -0700, Linus Torvalds wrote:
> 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

2009-10-08 20:26:06

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

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

2009-10-08 21:08:24

by Theodore Ts'o

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

On Thu, Oct 08, 2009 at 01:00:58PM -0700, Linus Torvalds wrote:
>
> 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

2009-10-08 21:14:40

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

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

2009-10-09 09:16:31

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

* Linus Torvalds <[email protected]> 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.

2009-10-09 13:11:40

by Daniel Walker

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

On Fri, 2009-10-09 at 11:15 +0200, Ingo Molnar wrote:
>
> 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

2009-10-09 14:08:52

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

On Fri, 2009-10-09 at 11:15 +0200, Ingo Molnar wrote:
> * Linus Torvalds <[email protected]> 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). 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

2009-10-09 19:30:10

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

On Fri, Oct 09, 2009 at 09:08:07AM -0500, James Bottomley wrote:
> On Fri, 2009-10-09 at 11:15 +0200, Ingo Molnar wrote:
> > * Linus Torvalds <[email protected]> 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

2009-10-10 14:38:42

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

On Thu, 2009-10-08 at 13:25 -0700, Linus Torvalds wrote:
>
> 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

2009-10-12 13:07:40

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

* James Bottomley <[email protected]> wrote:

> On Fri, 2009-10-09 at 11:15 +0200, Ingo Molnar wrote:
> > * Linus Torvalds <[email protected]> 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). [...]

I think you need to update your notion of what goes into
drivers/staging/ - these days it's primarily about code/implementation
quality (Greg please correct me if i'm wrong about that).

Driver ABIs are distinctly down the priority list.

> [...] 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.

What kind of significant ABI does this driver expose? If this question
even comes up then it's using the wrong kind of ABI i think. Drivers
should almost never expose significant new ABIs.

If it's a storage management ABI then that should have been done at a
higher level - possibly as a system call, but at minimum as a general
facility.

(Anyway ... this cannot be argued without knowing what specific ABI you
mean here.)

Ingo

2009-10-12 14:20:00

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

On Mon, 2009-10-12 at 15:06 +0200, Ingo Molnar wrote:
> * James Bottomley <[email protected]> wrote:
>
> > On Fri, 2009-10-09 at 11:15 +0200, Ingo Molnar wrote:
> > > * Linus Torvalds <[email protected]> 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). [...]
>
> I think you need to update your notion of what goes into
> drivers/staging/ - these days it's primarily about code/implementation
> quality (Greg please correct me if i'm wrong about that).
>
> Driver ABIs are distinctly down the priority list.

Not for me they're not. We worked hard to unify exposed ABIs for
drivers, so this is the most important user visible feature. We can
clean up code in the drivers tree, that's easy. We can't break the user
visible ABI of a supported driver without causing a lot of pain to its
user community.

> > [...] 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.
>
> What kind of significant ABI does this driver expose? If this question
> even comes up then it's using the wrong kind of ABI i think. Drivers
> should almost never expose significant new ABIs.

The answer was in the email fragment you quoted: none on its own. It
plugs into the standard FC transport ABI now.

However, there are various storage bodies who see creating user ABIs as
part of their job. The way these tend to get implemented is in the
driver because that's the way it was done in UNIX (each driver rolls its
own). Part of my job is going through drivers and taking them out
again ... while ensuring we at least have a way to perform whatever task
it was in a uniform manner for all drivers.

> If it's a storage management ABI then that should have been done at a
> higher level - possibly as a system call, but at minimum as a general
> facility.

It is: it's implemented in the FC transport class, where it's uniform
for all FC drivers.

James

> (Anyway ... this cannot be argued without knowing what specific ABI you
> mean here.)
>
> Ingo

2009-10-12 14:29:19

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

On Mon, Oct 12, 2009 at 03:06:52PM +0200, Ingo Molnar wrote:
>
> * James Bottomley <[email protected]> wrote:
>
> > On Fri, 2009-10-09 at 11:15 +0200, Ingo Molnar wrote:
> > > * Linus Torvalds <[email protected]> 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). [...]
>
> I think you need to update your notion of what goes into
> drivers/staging/ - these days it's primarily about code/implementation
> quality (Greg please correct me if i'm wrong about that).

No, so far that is the majority of why stuff is in staging, although
userspace apis also do play a part here.

But I leave it up to the subsystem maintainer to do what they want to
do, if they want to take coding style mistakes, well, it's their
responsibility to maintain the crud :)

thanks,

greg k-h

2009-10-12 14:55:39

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

* James Bottomley <[email protected]> wrote:

> > > 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). [...]
> >
> > I think you need to update your notion of what goes into
> > drivers/staging/ - these days it's primarily about
> > code/implementation quality (Greg please correct me if i'm wrong
> > about that).
> >
> > Driver ABIs are distinctly down the priority list.
>
> Not for me they're not. We worked hard to unify exposed ABIs for
> drivers, so this is the most important user visible feature. We can
> clean up code in the drivers tree, that's easy. We can't break the
> user visible ABI of a supported driver without causing a lot of pain
> to its user community.

I think you are interpreting what should go into drivers/staging/ _very_
narrowly.

Basically if you skip the drivers/staging/ step for unclean drivers you
_remove_ an incentive of driver authors to fix up crap. If it's upstream
already without cleanups then why bother cleaning it up fully?

Staging should be for drivers that arent clean enough for mainline as-is
(having a messy ABI can be one of the reasons that makes a driver
unclean), but which are still important enough and have active
maintainers/developers who care about them.

It's basically an optimistic multi-step trust algorithm for drivers
whose lack would otherwise be a "cannot use upstream" showstopper for a
significant class of Linux users - but which are not yet clean enough
for upstream inclusion. The 4 steps of:

- out of tree
- in Greg's staging tree
- upstream in drivers/staging/
- upstream in drivers/

Offer various degrees of incentives and walking that path expresses it
both to driver authors and to kernel maintainers that all parties
involved can be trusted to produce clean code.

Everyone wins from that:

- users get faster hw-enablement

- driver authors get reinforcement that their stuff is moving forward
(which they can communicate back to their employers)

- maintainers get patches that they can build trust upon and the danger
of release-and-forget drivers is lessened.

- even if authors orphan a driver, actual real users have the ability
to move things themselves.

- [ if none of that happens then sure the driver wasnt all that
important to begin with and can be dropped - nobody loses. ]

I think your interpretation is arbitrary - where did you get that ABI
rule from? I'm sure it cannot be from any of the drivers/staging/
discussions on lkml, i've followed them quite closely. If 'has a messy
ABI' was the only requirement for drivers/staging/ then we could move
90% of drivers/staging/ into drivers/ straight away - and that would be
counter-productive IMHO.

Sidenote, in fact i think we should expand on that: drivers/staging/
should be used in the _other_ direction as well - to un-upstream stale
drivers that are abandoned and unused, in a gradual fashion. 'git mv' is
cheap.

Basically, drivers/staging/ gives us an excellent opportunity to
_increase_ the quality of drivers by applying stronger upstream
inclusion filters, without having to hurt users/developers in the
process. We just have to start using it that way as well.

Ingo

2009-10-12 15:14:33

[permalink] [raw]

Subject: Moving drivers into staging (was Re: [GIT PULL] SCSI fixes for 2.6.32-rc3)

adding David Miller and the wireless developers who had this idea as
well...

On Mon, Oct 12, 2009 at 04:54:53PM +0200, Ingo Molnar wrote:
> I think your interpretation is arbitrary - where did you get that ABI
> rule from? I'm sure it cannot be from any of the drivers/staging/
> discussions on lkml, i've followed them quite closely. If 'has a messy
> ABI' was the only requirement for drivers/staging/ then we could move
> 90% of drivers/staging/ into drivers/ straight away - and that would be
> counter-productive IMHO.

I agree with this, and the other points you raised that I snipped out.

> Sidenote, in fact i think we should expand on that: drivers/staging/
> should be used in the _other_ direction as well - to un-upstream stale
> drivers that are abandoned and unused, in a gradual fashion. 'git mv' is
> cheap.

Ok, this is about the 3rd or 4th time I've heard this, from totally
different people lately. It seems that I'm the only one that has the
ability to drop drivers out of the kernel tree, which is a funny
situation :)

In thinking about this a lot more, I don't really mind it. If people
want to push stuff out of "real" places in the kernel, into
drivers/staging/ and give the original authors and maintainers notice
about what is going on, _and_ provide a TODO file for what needs to
happen to get the code back into the main portion of the kernel tree,
then I'll be happy to help out with this and manage it.

I think a 6-9 month window (basically 3 kernel releases) should be
sufficient time to have a driver that has been in drivers/staging/ be
cleaned up enough to move back into the main kernel tree. If not, it
could be easily dropped.

Any objections to this?

> Basically, drivers/staging/ gives us an excellent opportunity to
> _increase_ the quality of drivers by applying stronger upstream
> inclusion filters, without having to hurt users/developers in the
> process. We just have to start using it that way as well.

I totally agree. And so far, it does seem to be working well for this.
A number of companies have used it to successfully get their code into
the main kernel, as well as driving them to clean up their code better
to keep it from having to go into the staging tree.

thanks,

greg k-h

2009-10-12 15:26:34

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

On Mon, 2009-10-12 at 16:54 +0200, Ingo Molnar wrote:
> * James Bottomley <[email protected]> wrote:
>
> > > > 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). [...]
> > >
> > > I think you need to update your notion of what goes into
> > > drivers/staging/ - these days it's primarily about
> > > code/implementation quality (Greg please correct me if i'm wrong
> > > about that).
> > >
> > > Driver ABIs are distinctly down the priority list.
> >
> > Not for me they're not. We worked hard to unify exposed ABIs for
> > drivers, so this is the most important user visible feature. We can
> > clean up code in the drivers tree, that's easy. We can't break the
> > user visible ABI of a supported driver without causing a lot of pain
> > to its user community.
>
> I think you are interpreting what should go into drivers/staging/ _very_
> narrowly.

As it is my right to do.

> Basically if you skip the drivers/staging/ step for unclean drivers you
> _remove_ an incentive of driver authors to fix up crap. If it's upstream
> already without cleanups then why bother cleaning it up fully?

There's cleanup and there's cleanup. Most of the remaining problems
with the bfa driver won't be found by checkpatch and they're not obvious
to a non-storage expert ... but also these aren't major; I just want it
where I can see it to make sure forward progress is being made.

The major area of change for this driver is interfacing it to libfc ...
that's not a staging tree function. Assuming we can make it work, this
driver will become the first non fcoe consumer of libfc. Likely both
libfc and this driver will have to change as we try this. Again, it's
not something just anyone can do (in fact, I intend to leave it to the
experts and just referee from the sidelines).

> Staging should be for drivers that arent clean enough for mainline as-is
> (having a messy ABI can be one of the reasons that makes a driver
> unclean), but which are still important enough and have active
> maintainers/developers who care about them.
>
> It's basically an optimistic multi-step trust algorithm for drivers
> whose lack would otherwise be a "cannot use upstream" showstopper for a
> significant class of Linux users - but which are not yet clean enough
> for upstream inclusion. The 4 steps of:
>
> - out of tree
> - in Greg's staging tree
> - upstream in drivers/staging/
> - upstream in drivers/
>
> Offer various degrees of incentives and walking that path expresses it
> both to driver authors and to kernel maintainers that all parties
> involved can be trusted to produce clean code.
>
> Everyone wins from that:
>
> - users get faster hw-enablement
>
> - driver authors get reinforcement that their stuff is moving forward
> (which they can communicate back to their employers)
>
> - maintainers get patches that they can build trust upon and the danger
> of release-and-forget drivers is lessened.
>
> - even if authors orphan a driver, actual real users have the ability
> to move things themselves.
>
> - [ if none of that happens then sure the driver wasnt all that
> important to begin with and can be dropped - nobody loses. ]

Well, the choir appreciates the sermon.

> I think your interpretation is arbitrary - where did you get that ABI
> rule from? I'm sure it cannot be from any of the drivers/staging/
> discussions on lkml, i've followed them quite closely. If 'has a messy
> ABI' was the only requirement for drivers/staging/ then we could move
> 90% of drivers/staging/ into drivers/ straight away - and that would be
> counter-productive IMHO.

It's called making an external process serve my purposes. I'm not
asking anyone else to do this. However, having a halfway house for
incorrect ABI drivers is a useful additional function which we were
lacking before.

Putting a driver that needs expert cleanup into someone else's tree
seems counter productive to me because it's separating the community of
experts from the driver updates. I mean, fine for whitespace stuff, and
also fine if we actually have documentation; less so if only the authors
understand the hardware and their patches need monitoring.

Having the SCSI tree update drivers/staging is an OK solution. However,
at this stage, I don't think the bfa driver is bad enough to be tagged
with the staging taint flag.

James

> Sidenote, in fact i think we should expand on that: drivers/staging/
> should be used in the _other_ direction as well - to un-upstream stale
> drivers that are abandoned and unused, in a gradual fashion. 'git mv' is
> cheap.
>
> Basically, drivers/staging/ gives us an excellent opportunity to
> _increase_ the quality of drivers by applying stronger upstream
> inclusion filters, without having to hurt users/developers in the
> process. We just have to start using it that way as well.
>
> Ingo

2009-10-12 15:43:38

[permalink] [raw]

Subject: Re: Moving drivers into staging (was Re: [GIT PULL] SCSI fixes for 2.6.32-rc3)

* Greg KH <[email protected]> wrote:

> > Sidenote, in fact i think we should expand on that: drivers/staging/
> > should be used in the _other_ direction as well - to un-upstream
> > stale drivers that are abandoned and unused, in a gradual fashion.
> > 'git mv' is cheap.
>
> Ok, this is about the 3rd or 4th time I've heard this, from totally
> different people lately. [...]

Heh. I guess i had a good crystal ball on that then - i raised that
issue in my very first comments on drivers/staging/, 2.5 years ago - see
the "Linux Kernel developer statement about closed source code" thread,
where i wrote:

| - We must accept open-source drivers (which are license-compatible
| with the kernel) for new hardware the moment they are offered. No
| ifs and when. No whining about quality, style, security, re-use,
| non-reuse, obsolete APIs, overlapping functionality, the already
| busy merge schedule of a maintainer or whatever other thing can come
| up on lkml.
|
| We can create arbitrary quarantine mechanisms we wish to use:
| CONFIG_REALLY_BROKEN. We could even create drivers/staging/ as a
| temporary staging area for drivers.
|
| What we cannot do is to _deny_ the distribution channel and exclude
| users. The moment we do that (and we still do that in way too many
| areas of the kernel) we have lost the availability race.

[...]

| If it's not maintained actively(sc) (i.e. it's a fire-and-forget
| driver) then it can get marked BROKEN with time, and can get removed
| as well.

Hm, i think i even gave drivers/staging/ its name?

> [...] It seems that I'm the only one that has the ability to drop
> drivers out of the kernel tree, which is a funny situation :)

You are the only one who has the ability to send a warning shot towards
drivers _without hurting users_, and by moving it into the focus of a
team of cleanup oriented developers.

I think that's an important distinction ;-)

> In thinking about this a lot more, I don't really mind it. If people
> want to push stuff out of "real" places in the kernel, into
> drivers/staging/ and give the original authors and maintainers notice
> about what is going on, _and_ provide a TODO file for what needs to
> happen to get the code back into the main portion of the kernel tree,
> then I'll be happy to help out with this and manage it.
>
> I think a 6-9 month window (basically 3 kernel releases) should be
> sufficient time to have a driver that has been in drivers/staging/ be
> cleaned up enough to move back into the main kernel tree. If not, it
> could be easily dropped.
>
> Any objections to this?

Sounds excellent to me!

> > Basically, drivers/staging/ gives us an excellent opportunity to
> > _increase_ the quality of drivers by applying stronger upstream
> > inclusion filters, without having to hurt users/developers in the
> > process. We just have to start using it that way as well.
>
> I totally agree. And so far, it does seem to be working well for
> this. A number of companies have used it to successfully get their
> code into the main kernel, as well as driving them to clean up their
> code better to keep it from having to go into the staging tree.

Yeah. I think we were hurting from an under-estimated trust problem:
driver authors couldnt trust _us maintainers_ to follow through with
upstreaming. drivers/staging/ improved that IMHO.

I.e. the old process of upstreaming drivers was too arbitrary, incurred
big latencies and was dependent on the whims of maintainers. I.e. new
developers got exposed to some of the worst social aspects of a
distributed development process.

Now there's basically a single (and friendly! ;-) upstreaming channel
that people (yet-)unfamilar with Linux practices can standardize on.
This reduces the pressure on maintainers and also creates a reference
point for upstreaming honesty - which is almost unconditionally good i
think.

Ingo

2009-10-12 15:43:57

[permalink] [raw]

Subject: Re: Moving drivers into staging (was Re: [GIT PULL] SCSI fixes for 2.6.32-rc3)

On Mon, 2009-10-12 at 08:09 -0700, Greg KH wrote:
> adding David Miller and the wireless developers who had this idea as
> well...
>
> On Mon, Oct 12, 2009 at 04:54:53PM +0200, Ingo Molnar wrote:
> > I think your interpretation is arbitrary - where did you get that ABI
> > rule from? I'm sure it cannot be from any of the drivers/staging/
> > discussions on lkml, i've followed them quite closely. If 'has a messy
> > ABI' was the only requirement for drivers/staging/ then we could move
> > 90% of drivers/staging/ into drivers/ straight away - and that would be
> > counter-productive IMHO.
>
> I agree with this, and the other points you raised that I snipped out.
>
> > Sidenote, in fact i think we should expand on that: drivers/staging/
> > should be used in the _other_ direction as well - to un-upstream stale
> > drivers that are abandoned and unused, in a gradual fashion. 'git mv' is
> > cheap.
>
> Ok, this is about the 3rd or 4th time I've heard this, from totally
> different people lately. It seems that I'm the only one that has the
> ability to drop drivers out of the kernel tree, which is a funny
> situation :)
>
> In thinking about this a lot more, I don't really mind it. If people
> want to push stuff out of "real" places in the kernel, into
> drivers/staging/ and give the original authors and maintainers notice
> about what is going on, _and_ provide a TODO file for what needs to
> happen to get the code back into the main portion of the kernel tree,
> then I'll be happy to help out with this and manage it.
>
> I think a 6-9 month window (basically 3 kernel releases) should be
> sufficient time to have a driver that has been in drivers/staging/ be
> cleaned up enough to move back into the main kernel tree. If not, it
> could be easily dropped.
>
> Any objections to this?

Not as an optional tool to use when necessary.

If you want to make this a mandatory path for old drivers, then, I think
it's far too rigid, yes. There's a huge amount of danger to changing
working drivers simply on grounds of code cleanup and that danger
increases exponentially as they get older and the hardware gets rarer.
Look at what happened to the initio driver in 2008 for instance. That
was cleaned up by Alan Cox, no mean expert in the field, with the
assistance of a tester with the actual card, so basically a textbook
operation. However, a bug crept in during this process that wasn't
spotted by the tester. When it was spotted (bug report ~6 months later)
the original tester wasn't available and code inspection across the
cleanup was very hard. Fortunately, the reporter was motivated to track
down and patch the driver, so it worked out all right in the end, but a
lot of bug reporters aren't so capable (or so motivated). Plus most
clean up patches for old hardware tend only to be compile tested, so the
potential for bugs is far greater.

James

> > Basically, drivers/staging/ gives us an excellent opportunity to
> > _increase_ the quality of drivers by applying stronger upstream
> > inclusion filters, without having to hurt users/developers in the
> > process. We just have to start using it that way as well.
>
> I totally agree. And so far, it does seem to be working well for this.
> A number of companies have used it to successfully get their code into
> the main kernel, as well as driving them to clean up their code better
> to keep it from having to go into the staging tree.
>
> thanks,
>
> greg k-h

2009-10-12 17:25:48

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

On Mon, 12 Oct 2009, James Bottomley wrote:
> >
> > I think you are interpreting what should go into drivers/staging/ _very_
> > narrowly.
>
> As it is my right to do.

Umm, James, it cuts both ways.

Others can assert their interpretation, and quite frankly, yours is the
odd one out. Everybody else agrees oevrwhelmingly that "staging" is about
ugly and not-up-to-snuff drivers.

So you can talk about your 'right' to interpret things all you want, but
what's the point? If others haev the same right (which presumably even you
agree they do), then if people think a driver is ugly and needs to be in
staging, what makes _your_ right so special?

As mentioned earlier, I don't personally care about this driver, but I do
care about your behavior. You can't just ignore other people if they say
that a driver is too damn ugly.

Linus

2009-10-12 23:39:19

[permalink] [raw]

Subject: Re: Moving drivers into staging (was Re: [GIT PULL] SCSI fixes for 2.6.32-rc3)

On Mon, Oct 12, 2009 at 05:42:44PM +0200, Ingo Molnar wrote:
> Hm, i think i even gave drivers/staging/ its name?

Yes you did, and I appreciate it :)

> > [...] It seems that I'm the only one that has the ability to drop
> > drivers out of the kernel tree, which is a funny situation :)
>
> You are the only one who has the ability to send a warning shot towards
> drivers _without hurting users_, and by moving it into the focus of a
> team of cleanup oriented developers.
>
> I think that's an important distinction ;-)

Good point.

> > In thinking about this a lot more, I don't really mind it. If people
> > want to push stuff out of "real" places in the kernel, into
> > drivers/staging/ and give the original authors and maintainers notice
> > about what is going on, _and_ provide a TODO file for what needs to
> > happen to get the code back into the main portion of the kernel tree,
> > then I'll be happy to help out with this and manage it.
> >
> > I think a 6-9 month window (basically 3 kernel releases) should be
> > sufficient time to have a driver that has been in drivers/staging/ be
> > cleaned up enough to move back into the main kernel tree. If not, it
> > could be easily dropped.
> >
> > Any objections to this?
>
> Sounds excellent to me!

Great, I'll await the patches to move stuff to drivers/staging/ now.

Wireless developers, warm up your editors :)

thanks,

greg k-h

2009-10-12 23:39:24

[permalink] [raw]

Subject: Re: Moving drivers into staging (was Re: [GIT PULL] SCSI fixes for 2.6.32-rc3)

On Mon, Oct 12, 2009 at 10:43:06AM -0500, James Bottomley wrote:
> If you want to make this a mandatory path for old drivers, then, I think
> it's far too rigid, yes. There's a huge amount of danger to changing
> working drivers simply on grounds of code cleanup and that danger
> increases exponentially as they get older and the hardware gets rarer.
> Look at what happened to the initio driver in 2008 for instance. That
> was cleaned up by Alan Cox, no mean expert in the field, with the
> assistance of a tester with the actual card, so basically a textbook
> operation. However, a bug crept in during this process that wasn't
> spotted by the tester. When it was spotted (bug report ~6 months later)
> the original tester wasn't available and code inspection across the
> cleanup was very hard. Fortunately, the reporter was motivated to track
> down and patch the driver, so it worked out all right in the end, but a
> lot of bug reporters aren't so capable (or so motivated). Plus most
> clean up patches for old hardware tend only to be compile tested, so the
> potential for bugs is far greater.

I understand the potential for bugs, and am not saying to do this for
all drivers, so it is not mandatory at all.

I have just received a bunch of people asking me if we can use
drivers/staging/ to get stuff that is known broken, or has other
problems (style issues[1]), out into an area where people know it needs
to be fixed up otherwise it will be dropped.

thanks,

greg k-h

[1] No, floppy.c doesn't count, no matter how much people might want it
to :)

2009-10-13 14:30:23

[permalink] [raw]

Subject: Re: [GIT PULL] SCSI fixes for 2.6.32-rc3

On Mon, 2009-10-12 at 10:24 -0700, Linus Torvalds wrote:
>
> On Mon, 12 Oct 2009, James Bottomley wrote:
> > >
> > > I think you are interpreting what should go into drivers/staging/ _very_
> > > narrowly.
> >
> > As it is my right to do.
>
> Umm, James, it cuts both ways.
>
> Others can assert their interpretation, and quite frankly, yours is the
> odd one out. Everybody else agrees oevrwhelmingly that "staging" is about
> ugly and not-up-to-snuff drivers.

I haven't actually ever said otherwise. I have asserted that the bfa
driver, while large, isn't out of the ball park for a FC driver, that it
does provide all the correct user visible FC ABI pieces but that it does
have minor anomalies, primarily in bfa/include, that need cleaning up,
plus it needs to interface to libfc at some future point. I have also
asserted that drivers/scsi is the best place to address the remaining
issues.

> So you can talk about your 'right' to interpret things all you want, but
> what's the point? If others haev the same right (which presumably even you
> agree they do), then if people think a driver is ugly and needs to be in
> staging, what makes _your_ right so special?

It's a judgement call whether a driver goes through staging or not. I'm
happy to hear other opinions about this driver, but I'd like them to be
from reading the code, not generalities. I've stated my specific
reasons why this driver isn't a good candidate for staging.

> As mentioned earlier, I don't personally care about this driver, but I do
> care about your behavior. You can't just ignore other people if they say
> that a driver is too damn ugly.

But no-one's actually said that. The whole discussion was theoretical
rather than based on this driver. My position on the generalities is
that ABI issues head a driver automatically for staging. For non user
visible code based issues, it's a maintainer judgement call where the
cleanup is best done, which in turn depends on what the actual issues
are. For this specific driver, I've given reasons several times why the
changes are best made in drivers/scsi.

James

2009-10-13 18:09:15

by Luis R. Rodriguez

[permalink] [raw]

Subject: Re: Moving drivers into staging (was Re: [GIT PULL] SCSI fixes for 2.6.32-rc3)

On Mon, Oct 12, 2009 at 4:24 PM, Greg KH <[email protected]> wrote:
> On Mon, Oct 12, 2009 at 05:42:44PM +0200, Ingo Molnar wrote:
>> Hm, i think i even gave drivers/staging/ its name?
>
> Yes you did, and I appreciate it :)
>
>> > [...] It seems that I'm the only one that has the ability to drop
>> > drivers out of the kernel tree, which is a funny situation :)
>>
>> You are the only one who has the ability to send a warning shot towards
>> drivers _without hurting users_, and by moving it into the focus of a
>> team of cleanup oriented developers.
>>
>> I think that's an important distinction ;-)
>
> Good point.
>
>> > In thinking about this a lot more, I don't really mind it. If people
>> > want to push stuff out of "real" places in the kernel, into
>> > drivers/staging/ and give the original authors and maintainers notice
>> > about what is going on, _and_ provide a TODO file for what needs to
>> > happen to get the code back into the main portion of the kernel tree,
>> > then I'll be happy to help out with this and manage it.
>> >
>> > I think a 6-9 month window (basically 3 kernel releases) should be
>> > sufficient time to have a driver that has been in drivers/staging/ be
>> > cleaned up enough to move back into the main kernel tree. If not, it
>> > could be easily dropped.
>> >
>> > Any objections to this?
>>
>> Sounds excellent to me!
>
> Great, I'll await the patches to move stuff to drivers/staging/ now.
>
> Wireless developers, warm up your editors :)

OK -- prism54 seems like a good candidate, instead of removing it
completely as I originally outlined on the feature removal schedule.
Do we have a file to give notices to move drivers to staging because
they are old as with the feature removal schedule? The more visible
these things become the better it is for users.

Luis

2009-10-14 04:46:06

[permalink] [raw]

Subject: Re: Moving drivers into staging (was Re: [GIT PULL] SCSI fixes for 2.6.32-rc3)

On Tue, Oct 13, 2009 at 11:08:15AM -0700, Luis R. Rodriguez wrote:
> On Mon, Oct 12, 2009 at 4:24 PM, Greg KH <[email protected]> wrote:
> > On Mon, Oct 12, 2009 at 05:42:44PM +0200, Ingo Molnar wrote:
> >> Hm, i think i even gave drivers/staging/ its name?
> >
> > Yes you did, and I appreciate it :)
> >
> >> > [...] It seems that I'm the only one that has the ability to drop
> >> > drivers out of the kernel tree, which is a funny situation :)
> >>
> >> You are the only one who has the ability to send a warning shot towards
> >> drivers _without hurting users_, and by moving it into the focus of a
> >> team of cleanup oriented developers.
> >>
> >> I think that's an important distinction ;-)
> >
> > Good point.
> >
> >> > In thinking about this a lot more, I don't really mind it. ??If people
> >> > want to push stuff out of "real" places in the kernel, into
> >> > drivers/staging/ and give the original authors and maintainers notice
> >> > about what is going on, _and_ provide a TODO file for what needs to
> >> > happen to get the code back into the main portion of the kernel tree,
> >> > then I'll be happy to help out with this and manage it.
> >> >
> >> > I think a 6-9 month window (basically 3 kernel releases) should be
> >> > sufficient time to have a driver that has been in drivers/staging/ be
> >> > cleaned up enough to move back into the main kernel tree. ??If not, it
> >> > could be easily dropped.
> >> >
> >> > Any objections to this?
> >>
> >> Sounds excellent to me!
> >
> > Great, I'll await the patches to move stuff to drivers/staging/ now.
> >
> > Wireless developers, warm up your editors :)
>
> OK -- prism54 seems like a good candidate, instead of removing it
> completely as I originally outlined on the feature removal schedule.
> Do we have a file to give notices to move drivers to staging because
> they are old as with the feature removal schedule? The more visible
> these things become the better it is for users.

We've found that the feature removal file is also ignored :)

How about when it was scheduled to be removed, we put it in staging and
I'll add it to my announcements about the staging tree every release?

Unless you can think of a better way?

thanks,

greg k-h

2009-10-14 05:20:15

by Joe Perches

[permalink] [raw]

Subject: Re: Moving drivers into staging (was Re: [GIT PULL] SCSI fixes for 2.6.32-rc3)

On Tue, 2009-10-13 at 21:45 -0700, Greg KH wrote:
> How about when it was scheduled to be removed, we put it in staging and
> I'll add it to my announcements about the staging tree every release?
> Unless you can think of a better way?

staging/to_be_removed_unless_fixed_by/v.x.y ?

2009-10-14 06:34:01

[permalink] [raw]

Subject: Re: Moving drivers into staging (was Re: [GIT PULL] SCSI fixes for 2.6.32-rc3)

* Joe Perches <[email protected]> wrote:

> On Tue, 2009-10-13 at 21:45 -0700, Greg KH wrote:
> > How about when it was scheduled to be removed, we put it in staging and
> > I'll add it to my announcements about the staging tree every release?
> > Unless you can think of a better way?
>
> staging/to_be_removed_unless_fixed_by/v.x.y ?

Yes, that's a real worry. Some time ago i suggested:

drivers/staging/good/
drivers/staging/bad/
drivers/staging/ugly/

good: drivers that are to go upstream in the next cycle
bad: outgoing drivers being obsoleted or abandoned
ugly: incoming messy drivers with active developers

The messaging of this looks nice and the names are short and obvious.

An added benefit is that this kind of separation makes it easy for
people interested in drivers/staging to follow the 'status' of drivers.
Once stuff goes into 'good' a different kind of review is needed than if
a driver goes into 'ugly'.

The main disadvantage would be the PR angle: putting new drivers into a
path named 'ugly'. Not something you want to put into a quarterly status
report, right? If we put drivers/staging/ugly/ drivers into
drivers/staging/ itself, we'd solve that problem. I.e. we'd keep the
current scheme, but we'd also add drivers/staging/good/ and
drivers/staging/bad/ as two extra stages for incoming and outgoing
drivers.

A third version would be a more neutral name:

drivers/staging/incoming/
drivers/staging/outgoing/

I think it has many advantages, but (of course!) it all depends on
whether Greg wants to have any separation like this.

Ingo

2009-10-14 14:15:31

by James Smart

[permalink] [raw]

Subject: Re: Moving drivers into staging (was Re: [GIT PULL] SCSI fixes for 2.6.32-rc3)

Ingo Molnar wrote:
> Yes, that's a real worry. Some time ago i suggested:
>
> drivers/staging/good/
> drivers/staging/bad/
> drivers/staging/ugly/
>
> good: drivers that are to go upstream in the next cycle
> bad: outgoing drivers being obsoleted or abandoned
> ugly: incoming messy drivers with active developers
>
> The messaging of this looks nice and the names are short and obvious.
>
> An added benefit is that this kind of separation makes it easy for
> people interested in drivers/staging to follow the 'status' of drivers.
> Once stuff goes into 'good' a different kind of review is needed than if
> a driver goes into 'ugly'.
>
> The main disadvantage would be the PR angle: putting new drivers into a
> path named 'ugly'. Not something you want to put into a quarterly status
> report, right? If we put drivers/staging/ugly/ drivers into
> drivers/staging/ itself, we'd solve that problem. I.e. we'd keep the
> current scheme, but we'd also add drivers/staging/good/ and
> drivers/staging/bad/ as two extra stages for incoming and outgoing
> drivers.

Change "ugly" to "wip" (work in progress). Should remove the negative
connotation and keeps things short. Does miss the spaghetti western theme
though :)

-- james s

2009-10-14 17:55:05

by Stefan Richter

[permalink] [raw]

Subject: Re: Moving drivers into staging (was Re: [GIT PULL] SCSI fixes for 2.6.32-rc3)

Ingo Molnar wrote:
> * Joe Perches <[email protected]> wrote:
>
>> On Tue, 2009-10-13 at 21:45 -0700, Greg KH wrote:
>> > How about when it was scheduled to be removed, we put it in staging and
>> > I'll add it to my announcements about the staging tree every release?
>> > Unless you can think of a better way?
>>
>> staging/to_be_removed_unless_fixed_by/v.x.y ?
>
> Yes, that's a real worry. Some time ago i suggested:
>
> drivers/staging/good/
> drivers/staging/bad/
> drivers/staging/ugly/

How well do "git am", "quilt import" and friends cope with ever changing
directories?

How about using drivers/staging/this_driver/TODO and (or) its Kconfig
help text to leave a note about the plans for this driver?

The worry that these will be ignored like
Documentation/feature-removal-schedule.txt is being ignored may apply to
the path name based solution too, I'm afraid.

(Besides, Greg's release announcements are a good channel for this.
These announcements are actually picked up by the specialized press.)
--
Stefan Richter
-=====-==--= =-=- -===-
http://arcgraph.de/sr/

2009-10-14 18:37:58

[permalink] [raw]

Subject: Re: Moving drivers into staging (was Re: [GIT PULL] SCSI fixes for 2.6.32-rc3)

* Stefan Richter <[email protected]> wrote:

> Ingo Molnar wrote:
> > * Joe Perches <[email protected]> wrote:
> >
> >> On Tue, 2009-10-13 at 21:45 -0700, Greg KH wrote:
> >> > How about when it was scheduled to be removed, we put it in staging and
> >> > I'll add it to my announcements about the staging tree every release?
> >> > Unless you can think of a better way?
> >>
> >> staging/to_be_removed_unless_fixed_by/v.x.y ?
> >
> > Yes, that's a real worry. Some time ago i suggested:
> >
> > drivers/staging/good/
> > drivers/staging/bad/
> > drivers/staging/ugly/
>
> How well do "git am", "quilt import" and friends cope with ever
> changing directories?

Once a driver is in a tree it's in Git and git mv is easy. People
working with Linux better familiarize themselves with Git workflow - the
sooner the better.

If it's not in tree then it will adopt to whatever layout there is once
it gets into Greg's tree. I dont see the problem.

> How about using drivers/staging/this_driver/TODO and (or) its Kconfig
> help text to leave a note about the plans for this driver?

Well, the answer is obvious i think. Tell me, at a glance, if you see a
patch on lkml, which one is for a staging driver to be obsoleted, and
which one is the one going upstream real soon? The patches say:

+++ a/drivers/staging/foo/x.c

+++ a/drivers/staging/bar/y.c

Then tell me the same at a glance if you see patches for:

+++ a/drivers/staging/wip/x.c

+++ a/drivers/staging/bad/y.c

> The worry that these will be ignored like
> Documentation/feature-removal-schedule.txt is being ignored may apply
> to the path name based solution too, I'm afraid.

It wont be 'ignored', as it's in every patch, it's in every commit, it's
in every substantial communication about that driver.

The problem with feature-removal-schedule.txt is that it's too much out
of sight and not part of the regular patch workflow. Same goes for any
TODO file. Experience has shown that the actual _path_ were drivers end
up does matter quite a bit, to general visibility and to mindset.

That's one of the reasons why we have _half a thousand_ directories in
drivers/ to begin with. The directory namespace is very powerful, and we
use it to convey all sorts of information about the logical category a
driver is in.

Using it in drivers/staging/ instead of the current flat hierarchy would
thus be pretty natural.

Ingo

2009-10-14 19:02:25

by Stefan Richter

[permalink] [raw]

Subject: Re: Moving drivers into staging (was Re: [GIT PULL] SCSI fixes for 2.6.32-rc3)

On 14 Oct, Ingo Molnar wrote:
> * Stefan Richter <[email protected]> wrote:
>> How well do "git am", "quilt import" and friends cope with ever
>> changing directories?

[Perhaps the -p option does the trick. And git-am could be done on a
temporary branch from before the move of the files.]

> Once a driver is in a tree it's in Git and git mv is easy. People
> working with Linux better familiarize themselves with Git workflow - the
> sooner the better.

Even if author and committer both work with git, they often use e-mail
to transfer patches.

> If it's not in tree then it will adopt to whatever layout there is once
> it gets into Greg's tree.

Many "good" drivers will start as "ugly" or "bad" ones.

>> How about using drivers/staging/this_driver/TODO and (or) its Kconfig
>> help text to leave a note about the plans for this driver?
>
> Well, the answer is obvious i think. Tell me, at a glance, if you see a
> patch on lkml, which one is for a staging driver to be obsoleted, and
> which one is the one going upstream real soon? The patches say:
>
> +++ a/drivers/staging/foo/x.c
>
> +++ a/drivers/staging/bar/y.c
>
> Then tell me the same at a glance if you see patches for:
>
> +++ a/drivers/staging/wip/x.c
>
> +++ a/drivers/staging/bad/y.c

Does this information matter much?

What's more interesting is whether development activity will _lead_ to a
driver being moved from bad or ugly to good.
--
Stefan Richter
-=====-==--= =-=- -===-
http://arcgraph.de/sr/

2009-10-14 19:16:34

[permalink] [raw]

Subject: Re: Moving drivers into staging (was Re: [GIT PULL] SCSI fixes for 2.6.32-rc3)

On Wed, Oct 14, 2009 at 08:33:08AM +0200, Ingo Molnar wrote:
>
> * Joe Perches <[email protected]> wrote:
>
> > On Tue, 2009-10-13 at 21:45 -0700, Greg KH wrote:
> > > How about when it was scheduled to be removed, we put it in staging and
> > > I'll add it to my announcements about the staging tree every release?
> > > Unless you can think of a better way?
> >
> > staging/to_be_removed_unless_fixed_by/v.x.y ?
>
> Yes, that's a real worry. Some time ago i suggested:
>
> drivers/staging/good/
> drivers/staging/bad/
> drivers/staging/ugly/
>
> good: drivers that are to go upstream in the next cycle
> bad: outgoing drivers being obsoleted or abandoned
> ugly: incoming messy drivers with active developers
>
> The messaging of this looks nice and the names are short and obvious.

Yeah, but they make my life much harder than it needs to be.

I'd prefer to stick with the directory naming scheme we have today, it
seems to work well so far.

thanks,

greg k-h

2009-10-15 06:04:04