[illumos-Developer] Webrev: New ARECA arcmsr driver

[Dan McDonald]

On Fri, Mar 18, 2011 at 7:15 PM, Garrett D'Amore <garrett at nexenta.com> wrote:
> Recently, ARECA supplied me with a code drop of their latest Solaris
> code, which supports the Areca 1880 cards, as well as adding support for
> the newer interrupt model and fixing some bugs.
>
> I want to give a great big *THANK YOU* to Areca for supporting illumos
> in this way!
>
> That said the code had a number of issues, which I've addressed (if
> anyone wants the original code drop, or the list of issues that I've
> fixed, let me know.)
>
> The updated driver is posted here in webrev form:
>
> http://mexico.purplecow.org/gdamore/webrev/arcmsr/

A LOT of what I have to say will probably be applicable to your second whack
on this code.  Hopefully the contributors will take my advice to heart as
well.

usr/src/uts/intel/io/scsi/adapters/arcmsr/arcmsr.h

* PtrToNum() & friends:

	- Use "void *" instead of caddr_t * unless there's a really
	  good reason.

	- I believe "uintptr_t" is the type you use for pointers-as-numbers.
	  This rids you of your _LP64 ifdef crap as it does the right thing
	  regardless.

* Lines 537-543:  redundant?

* struct ACB:  It'd be nice to know what the new mutexes are protecting.


usr/src/uts/intel/io/scsi/adapters/arcmsr/arcmsr.c

* Line 150... should that have an "ll" or "ull" at the end of the constant?

* ObPetPeeve:  I hate "if (!ptr)", and prefer "if (ptr == NULL)".  The
  compiler spits out the same code for either.

* Line 480 - Dumb question, and maybe I'll answer this myself, but how can we
  make the datamodel-matching guarantee here?

* Lines 517,541, etc. - WTF call memcpy() for one byte?!  (If the theoretical
  "functional strength reduction" I wrote about for advanced graduate
  compilers is available, sure this'll fall out, but ewwww....)

* Line 731 - It *appears* that in this case, you leak "pktioctlfld".

* Line 1137 --> See Albert's previous comment.  ;)

* General --> I see mixed uses of memcpy() and bcopy().  IIRC, kernel bcopy()
  is much faster than kernel memcpy().  This doesn't count the length == 1
  instances, which are just dumb.

* Lines 1407, 1412-1413 --> redundant, nuke 'em.

* Line 1425... do you want to clear ACB_F_BUS_RESET even if you return
  failure?

* Lines 1995-2001:  This sort of behavior always makes me nervous.

* Line 2254:  C++ comment allowed?

* Line 2282:  (void) strlcpy(devbuf, devnm, sizeof (devbuf));
  (Unless you have IRON CLAD guarantees about the input.)

* Line 2283:  WTF?  Assigning a character-constant to a *pointer*?!
  I'd look at this function a bit harder.

* Line 3756, others:  ewww, hardcoded 20 seconds.  #define please?

* Line 3764, same here.

* Rest seems okay, but I'm not a low-level driver wizard.


Hope this helps,
Dan