[illumos-Developer] Webrev: New ARECA arcmsr driver

[Garrett D'Amore]

On Thu, 2011-03-24 at 01:20 -0400, Dan McDonald wrote:
> 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.

Agreed.  Fixed.

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

Fixed.

> 
> * Lines 537-543:  redundant?

Fixed.

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

Agreed.  Fixing this will be a part of a future effort though, because
the current mutex usage is actually ugly and inconsistent.  I am not
100% that the code is bug free here (in fact I'm certain it *isn't*),
but I'm trying to avoid the larger refactoring required to fix it.

The isr_mutex is used for jobs sent to the chip, and is what is acquired
in the interrupt service routine.   The ccb_complete_list_mutex is used
*solely* for the ccb_complete_list.

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

Added ull.  Strangely the compiler doesn't complain here, perhaps
because the context is in initializing a 64-bit unsigned.

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

I've cleaned up a bunch (all?  maybe?) of those in response to other
feedback.

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

The comment here is *dead wrong*, and shows misunderstanding on the part
of the author.  There is no datamodel sensitive member of the structure,
and *that* is why its safe.  The supposed guarantee is false.  (This
came from Areca.)

I'm just going to remove the comment.

> 
> * 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....)

Yes, this whole section of code is complete and utter crap.  I have a
rewrite of it queued somewhere else, but it turns out that the code
suffers in other more critical design ways, and I want to spend no more
effort to fix this crap because I'm almost certain to just remove it in
a subsequent pass.  (For example, what happens if two programs try to
talk simultaneously to this interface?  I don't know.  And there are no
safeguards here!)

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

Good catch.  I'll fix.  (In the future that structure should go away.
If I were going to retain this functionality, I'd nuke the kmem_alloc
and use either an alloca() or just stick the structure on the stack
directly.  Going to kmem for this is stupid.   Again, I didn't write
this. :-)

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

Got it.

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

Yep.  All the memcpy's are on the ioctl pass through code that is slated
for future removal. :-)

Don't even look at the differences between cmn_err, and arcmsr_log as
another case if inconsistent coding.  TBD later.

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

Accept.

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

Probably not.  This should be restructured so that we only use this flag
with RESET_ALL, and likewise we only need the mutex for that path (well,
ignoring the check for ARECA_RAID_GONE, but I think that can be done
without a lock as well for a variety of reasons.)

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

Me too!  This is related to the above reset logic, and the failure
handling associated with it.  I will be cleaning this up as part of the
SCSAv3 conversion, because I'm going to change to use a separate lock
for the completion list and not try to do them inline here.

> 
> * Line 2254:  C++ comment allowed?

Fixed.

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

Fixed.

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

That code is right.  It is assigning a pointer to a string ("") not a
character const ('\0').  The idea is to make sure that addr always
points to a non-NULL ASCII string.

> 
> * Line 3756, others:  ewww, hardcoded 20 seconds.  #define please?
> 
> * Line 3764, same here.

I'd like to clean this crap up too, as part of a separate refactoring
effort.  For now I'm going to leave them alone unless someone objects
more strenuously.

	- Garrett
> 
> * Rest seems okay, but I'm not a low-level driver wizard.
> 
> 
> Hope this helps,
> Dan
> 
> _______________________________________________
> Developer mailing list
> Developer at lists.illumos.org
> http://lists.illumos.org/m/listinfo/developer