[illumos-Developer] Webrev: New ARECA arcmsr driver

[Garrett D'Amore]

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

Fixed.  Doh. :-)

> arcmsr.h:176-179 -- comment + size don't seem to agree -- should it be
> MSGDATABUFLEN + 1, or MSGDATABUFLEN?

I'm not sure which is correct.. this code came from Areca.  I would like
to remove it as it is only used in the currently busted utility bypass
stuff.

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

Accept.

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

This section of code is really ugly and busted.  I have a full rewrite
of it, but in retrospect I think I want to remove it.  In any event, I
want to avoid touching it because due to crashes in the Areca utility I
can't verify it.

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

No.  Ick.  More ugliness with the CLI pass through and more reason to
fix.  For now I'll limit it with min(xxx, MSGDATABUFLEN);

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

Fixed.

> 
> arcmsr.c:1822 perhaps != 0 instead?

Ok.

> 
> acrmsr.c:1983 != NULL instead?

Ok.  I should probably remove that, and will do so later, because this
should never be true once I convert to SCSAv3 DMA.  (Maybe replace with
an assert().)

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

Ick.  Probably.  This is a hideous hack IMO, and I'll be replacing this
logic as a future part of a rewrite to use SCSAv3 and a simpler resource
management design. For now I'll go ahead and make your suggested change.

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

strlcpy is better, and I'll convert to that.

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

Both accepted.

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

Accepted.

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

It should in fact, this whole thing needs to be cleaned up badly.  A lot
of things are not cleaned up on failure, and the current code we have
does an equally poor job.  I had cleaned this up in my other version
which had the major refactoring applied...

I guess I need to go back and fix it here... its ugly because the
mappings needed are different for different parts.

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

This is the same as above, basically.  Fixed.

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

Agreed.

> 
> acrmsr.c:3889 { should be on next line

Ooops!  This one escaped my massive cstyling fixes.  Thanks.

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