[illumos-Developer] Webrev: New ARECA arcmsr driver

[Richard Lowe]

arcmsr.c:150, arcmsr.c:161

 Do these differ by device revision?  Is increasing them correct on
all models, or just certain newer models?

arcmsr.c:168,179 (old)

  Why are various of these flags going away?  (I have never seen them
used before, are they perhaps pointless?)

arcmsr.c:248

  if (acb == NULL)

arcmsr.c:481,483

  "on a XX-bit kernel" not "in a..."
  Also, I suspect this comment is wrong.

arcmsr.c:1142

  Remove pointless blank line

arcmsr.c:1392

  Why?

arcmsr.c:1402

  Compare explicitly with something (or, following code suggest this
ends up ultimately being boolean)

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?

arcmsr.c:1440, arcmsr.c:1446, arcmsr.c:1466, arcmsr.c:1469

  NDI_ not DDI_? Huh?

arcmsr.c:1792,1803

  Why is this here?

arcmsr.c:1822

  if (sgcount > 0)

arcmsr.c:1859

  Really "> 256" not ">= 256"?

arcmsr.c:1983

  if (pkg == NULL)

  Should this be an assert?  Why would we get here with a null pkt?

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


arcmsr.c:2017

  Either pass a boolean_t, or explicitly compare against 0

arcmsr.c:2254

  Don't comment out code, delete it.

arcmsr.c:2872

  Pointless comment.

arcmsr.c:2875

  Looks to be both pointless and incorrect comment?

arcmsr.c:3001

  Declare at top of scope.

arcmsr.c:3079,3089

  Why do we need this?

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.

arcmsr.c:3367, arcmsr.c:3388

  The alignment of these lines (the breaks) is wacky and misleading

arcmsr.c:3395

  Change this to:

    if (diff == 0)
            break;

  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?)

  (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

  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?

arcmsr.c:4044

  Don't comment out code, delete it.

arcmsr.c:4705, arcmsr.c:4782

  No reason to do this, just makes it less readable

arcmsr.c:4955, arcmsr.c:5039, arcmsr.c:5126, arcmsr.c:5504, arcmsr.c:5572

  Is this comment describing the obvious, or misleading?

arcmsr.c:5138

  Should be a boolean or explicit comparison

arcmsr.c:5115

  Should be boolean

arcmsr.c:5192,5194

  Is this comment useful?

  The whole function is kind of worrying.  Are the usecwaits arbitrary?

arcmsr.c:5614, arcmsr.c:5649

  Explicitly compare with 0

arcmsr.c:5850

  Looks like it should be boolean

arcmsr.h:785

  ptrdiff_t? (see arcmsr.c:2821)

-- Rich