[illumos-Developer] Webrev: New ARECA arcmsr driver

[Garrett D'Amore]

On Thu, 2011-03-24 at 02:46 -0400, Richard Lowe wrote:
> arcmsr.c:150, arcmsr.c:161
> 
>  Do these differ by device revision?  Is increasing them correct on
> all models, or just certain newer models?

Correct on all models, apparently.

> 
> arcmsr.c:168,179 (old)
> 
>   Why are various of these flags going away?  (I have never seen them
> used before, are they perhaps pointless?)

So this driver never touches uio's, and so doesn't need/use D_64BIT to
be 64-bit clean, and D_HOTPLUG is intended for devices that support
surprise removal (e.g. cardbus/pcmcia/usb and is only used by the
cardbus stack).  (It should not have been used for this driver.)

> 
> arcmsr.c:248
> 
>   if (acb == NULL)

Fixed.

> 
> arcmsr.c:481,483
> 
>   "on a XX-bit kernel" not "in a..."
>   Also, I suspect this comment is wrong.

Badly, and totally wrong.  Comment removed.  I discussed this in
response to an earlier review.

> 
> arcmsr.c:1142
> 
>   Remove pointless blank line

Actually, in my editor, it is more readable with that blank line there.
The declarations are separated from the code.  So I'll leave it.  In the
future this entire stanza for target == 16 will be removed, since its
yet a 2nd way to get to the busted/non-functional bypass functionality
for the CLI and ArcHTTP bypass stuff.

> 
> arcmsr.c:1392
> 
>   Why?

I don't know actually, but I suspect its related to the fact that the
reset logic likes to wait for the adapter to complete an abort
operation, and may require interrupts functioning to work properly (and
interrupts would be disabled during ddi_in_panic() == B_TRUE. :-)

I didn't add that logic, and its present in the old code too.

> 
> arcmsr.c:1402
> 
>   Compare explicitly with something (or, following code suggest this
> ends up ultimately being boolean)

Agreed. This was weird, and I changed it to a boolean in my more massive
(and riskier) refactoring.  0xff as an error condition is weird.  For
now just doing != 0.

> 
> arcmsr.c:1387, arcmsr.c:1407, arcmsr.c:1411, arcmsr.c:1413,
> arcmsr.c:1416, arcmsr.c:1419, arcmsr.c:1422
> 
>   Doesn't SCSA provide some handy symbolic #defines for this to return?

Nope.  0, 1, and -1 per spec.

> 
> arcmsr.c:1440, arcmsr.c:1446, arcmsr.c:1466, arcmsr.c:1469
> 
>   NDI_ not DDI_? Huh?

The use of NDI is correct.  tran_bus_config() is an undocumented NDI
support function, and the routines use NDI properly.  I've verified all
NDI usage in this driver, and they didn't get it wrong.

> 
> arcmsr.c:1792,1803
> 
>   Why is this here?

The original author was keeping notes to himself here. Its present in
the old code too.  I'll remove it.

> 
> arcmsr.c:1822
> 
>   if (sgcount > 0)

I wrote it as != 0... its unsigned! :-)

> 
> arcmsr.c:1859
> 
>   Really "> 256" not ">= 256"?

That's a chip detail.. not sure.  Its this way in the old driver.  I'm
going to take it on faith for now.

> 
> arcmsr.c:1983
> 
>   if (pkg == NULL)
> 
>   Should this be an assert?  Why would we get here with a null pkt?

It probably should be.  When I refactor to use SCSAv3 I'll be able to
make this assertion much more conclusively, and I'll change it.

> 
> arcmsr.c:1995
> 
>   misuse of mutex_owned()  See mutex_owned(9F)
> 
>   "...      mutex_owned() should only be used in  ASSERT()  and  may
> be enforced by not being defined unless the preprocessor symbol DEBUG
> is defined."

Yes, I know.  Its hideous here.  I'll change it later, when I switch to
SCSAv3.  Its tricky to change it now because I need to fix the
completion logic and I don't want to do major refactoring of the code in
this pass.

> 
> 
> arcmsr.c:2017
> 
>   Either pass a boolean_t, or explicitly compare against 0

Ok.  Admittedly, I find this far less offensive than it seems every one
of my other reviewers does.

> 
> arcmsr.c:2254
> 
>   Don't comment out code, delete it.

I'm going to clean this up as part of the SCSAv3 fixes, because the
entire CCB state model will be revisited/simplified.  I didn't feel
comfortable removing it this time since I don't know what the upstream
intended here.  (They commented it, not me! :-)

> 
> arcmsr.c:2872
> 
>   Pointless comment.

Agreed.

> 
> arcmsr.c:2875
> 
>   Looks to be both pointless and incorrect comment?

Heh!  I left that comment in accidentally when I cleaned this code up.
(There was originally a big nasty #ifdef here that would do what that
comment said.)  Fixed.

> 
> arcmsr.c:3001
> 
>   Declare at top of scope.

Accept.

> 
> arcmsr.c:3079,3089
> 
>   Why do we need this?

I'm going to go back and clean this up to help with all the cmn_err and
crap.  It will make the code much cleaner when I do.  For now, its crap
left over from upstream.  So, fix later.  (And I'll actually be using
it, not just removing it.)

> 
> arcmsr.c:3092,3140
> 
>   We have at least 2, and perhaps 3 list implementations in the
> kernel.  Pick one and use it, don't add another.

Agreed.  To be fixed later with the SCSAv3 rework (promise!).  This was
in the code inherited from Areca...

> 
> arcmsr.c:3367, arcmsr.c:3388
> 
>   The alignment of these lines (the breaks) is wacky and misleading

There is no way to cleanly & correctly indent these case clauses with
local variable declarations.  Refactoring this code into a more object
oriented approach (which again, I'm deferring to a later project), would
be a better solution.

What is there now is the only indent that seemed to meet cstyle and
wasn't *also* wrong.

> 
> arcmsr.c:3395
> 
>   Change this to:
> 
>     if (diff == 0)
>             break;


That's not correct, because there are two statements after the if
statement.  I did actually have a rewrite and reindent of this, but
again I'm doing the minimal here to get by.


> 
>   And the lack of indent means you don't have to smear the body down
> the right margin.
> 
> arcmsr.c:3469, arcmsr.c:3480, arcmsr.c:3494, arcmsr.c:3524,
> arcmsr.c:3538, arcmsr.c:3552,
> arcmsr.c:3580, arcmsr.c:3596, arcmsr.c:3611, arcmsr.c:3924,
> arcmsr.c:3958, arcmsr.c:3984,
> arcmsr.c:4074, arcmsr.c:4083, arcmsr.c:4120, arcmsr.c:4128
> 
>   Misleading break indent again.  Pull them into the braces or
> something.  (I'm not crazy, right?)

Oh, if you prefer to see the break's inside the braces, I *can* do that.
I chose the other way intentionally, as it makes things line up properly
as if you had other statements.

The style guide is unclear here... at the end of the day I hate
(stylistically) declaring variables in case consequents -- I don't write
code like this which is probably why I am foundering trying to find a
solution I like.

> 
>   (I'm going to stop listing these.  The same comment applies to every
> one in the file)
> 
> arcmsr.c:3727, arcmsr.c:3757
> 
>   If this is wanting to return boolean, it should return a boolean_t
> 
> arcmsr.c:3830, arcmsr.c:3842, arcmsr.c:3844

Agreed on both counts... fix later as part of the overall restructuring.
Our current code suffers this flaw too.

> 
>   Sneaking suspicion that this, too, is a boolean
> 
> arcmsr.c:3849, arcmsr.c:3861, arcmsr.c:3863, arcmsr.c:3882,
> arcmsr.c:3884, arcmsr.c:3992, arcmsr.c:4011, arcmsr.c:4016,
> arcmsr.c:4029
> 
>   And these?

Agreed again.  I'll fix later as part of restructuring.  Some of this
code is just awful.

> 
> arcmsr.c:4044
> 
>   Don't comment out code, delete it.

Same response as before.  I'll fix when I switch to SCSAv3.  For now it
is there as a notation left over from the upstream implementation.

> 
> arcmsr.c:4705, arcmsr.c:4782
> 
>   No reason to do this, just makes it less readable

Not sure what you're referring to.. the log message in general or the
specific format?  (I do want to clean this up, and having duplicate
statements here is nuts and would be part of what I'd clean up when I
restructure to be more "object oriented".)

> 
> arcmsr.c:4955, arcmsr.c:5039, arcmsr.c:5126, arcmsr.c:5504, arcmsr.c:5572
> 
>   Is this comment describing the obvious, or misleading?

Neither.

The comment should have "XXX:" or "TODO".  These use 0, 0, indicating
the whole handle, when a possibly (?) better implementation would use
the specific offset and size within the DMA region.  (Possibly better
because it isn't clear that its a performance win to subset the cache
flush that way.)  The comment was in our original driver.

> 
> arcmsr.c:5138
> 
>   Should be a boolean or explicit comparison

*Sigh*.  These stylistic nits are *everywhere*.  Accepted.

> 
> arcmsr.c:5115
> 
>   Should be boolean

More of same.  Accept.

> 
> arcmsr.c:5192,5194
> 
>   Is this comment useful?
> 
>   The whole function is kind of worrying.  Are the usecwaits arbitrary?

Yes, I think so. This whole thing needs closer inspection and clean up
when I refactor.  This is a hack to workaround chip errata.  These
functions are ugly in *every* driver that they occur in.

> 
> arcmsr.c:5614, arcmsr.c:5649
> 
>   Explicitly compare with 0
> 
> arcmsr.c:5850
> 
>   Looks like it should be boolean

Yeah, yeah.... 

> 
> arcmsr.h:785
> 
>   ptrdiff_t? (see arcmsr.c:2821)

Perhaps... but it probably doesn't matter that much.  TBH, this part of
this particular hardware chip is one thing I really, really don't like.
Giving me back a bus address on completions is just a bit weird.  I've
never seen any other device that does this.

	- Garrett


> 
> -- Rich
> 
> _______________________________________________
> Developer mailing list
> Developer at lists.illumos.org
> http://lists.illumos.org/m/listinfo/developer