[illumos-Developer] Webrev: New ARECA arcmsr driver

[Jason King]

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/
>
> (Some of the changes are gratuitous reordering... I've not tried to
> remove that reordering... the job was big enough as it was.)
>
> This code is lint and cstyle clean.
>
> There are a few more changes I'd like to consider making, but would like
> feedback on, and am thinking would better be done as part of a later
> change set:
>
> a) removal of the special firmware passthrough support.  It isn't safe,
> and from what I can tell doesn't work.  (Their utility crashes, and I
> cannot get source code for it, limiting my ability to do any debug.)  If
> anyone uses the Areca CLI or archttp utilities with arcmsr, I would
> *really* like to know.  (You can configure and manage the RAID mode
> cards using system BIOS, of course.)
>
> b) Possibly refactor the code into object oriented bits for each of the
> type A, B, C variants.  (I had done this, but the changes were too
> large, and risky, and so I want to proceed with these lower risk changes
> first.)
>
> c) Switch to SCSAv3.  This would give some performance boost and remove
> a lot of complexity in the driver.  I've already got a version with
> these changes, but they were again rather large and sweeping, and so
> I've shelved them for a future integration as part of my attempt to
> mitigate risk, and also make the review task a bit more manageable.
>
> d) remove reset(9e) and replace with quiesce(9e) -- testing needed for
> this.
>
> e) The code has support for handling 64-bit CDB addressing, but uses DMA
> attributes that prevent it (for good reason!).  We should simply that
> code by removal of the special cases for 64-bit CDB handling.  (Note
> that this does not relate to the *buffer* addresses, which are fully
> 64-bit capable.)
>
> Anyway, I would really, really like feedback on this driver.  Ideally I
> will be integrating this soon.

With the proviso that I'm not an expert on HBAs, so most of this is
going to be more generic comments (and mostly nits),

arcmsr.h:79-85, 537-543  -- These appear to be identical
arcmsr.h:176-179 -- comment + size don't seem to agree -- should it be
MSGDATABUFLEN + 1, or MSGDATABUFLEN?

arcmsr.c:308,348,445 Perhaps == NULL ?

arcmsr.c:502,566,4403 Is there a reason this is declared unsigned
long?  From the code it looks like it could just be declared uint8_t *
and save some unnecessary casting

arcmsr.c:580,4415 Is user_len guaranteed to be <= MSGDATABUFLEN?

arcmsr.c:855 Should we allow // style comments?

arcmsr.c:1822 perhaps != 0 instead?

acrmsr.c:1983 != NULL instead?

acrmsr.c:2003,4050 Shouldn't this be declared volatile in struct ACB?
(so that all references apply proper semantics in the face of a
change?)

acrmsr.c:2282 Consider strncpy(9f) instead

acrmsr.c:2303,2308 perhaps !=0 ?
acrmsr.c:2302,2307 perhaps != NULL ?

acrmsr.c:2590,2788,5908 What about using P2ROUNDUP instead?

acrmsr.c:2576 -- Throughout the function, after the register mapping
is done, any errors deallocate anything needed, but leave the mapping
in place -- should it not also unmap?

acrmsr.c:2704,2716 -- An error in either location appears to leak the
DMA mapped memory

acrmsr.c:3321,3322 - Casts appear to be superfluous

acrmsr.c:3889 { should be on next line