Sebastien Roy wrote:
> Garrett D'Amore wrote:
>> I'm looking for folks to review my changes to hme. I *cannot* commit
>> these changes if I don't get reviewers!
>>
>> The webrev is at http://cr.opensolaris.org/~gdamore/nemo-hme
>
> More functionality with thousands of lines of code removed. Nice. :-)
Thanks for the review.
>
> usr/src/uts/sparc/hme/Makefile
>
> * Why does hme depend on ip? It doesn't seem like it should.
Because it uses some ndd related support functions from ip.
>
>
> usr/src/uts/sun/io/hme.c
>
> * 61-93: There's no point in assigning each member of the enumeration
> with the value it would have by default.
Reject. Probably true, but these are also used for indices into the
message array. It makes it easier to see how they line up (less
"counting".) This portion of the code is ugly, but it predates my
changes. (I just removed an entry.) :-) I am going to leave it alone
for now.
>
> * 2583-2589: I don't understand the need for the cascading chain of if
> statements here. This isn't much of an improvement over the previous
> code, and the indentation is still not cstyle compliant. It could be
> simplified to:
>
> if ((GET_ROM8(&(hmep->hme_romp[i])) & 0xff) == 'P' &&
> (GET_ROM8(&(hmep->hme_romp[i+1])) & 0xff) == 'C' &&
> (GET_ROM8(&(hmep->hme_romp[i+2])) & 0xff) == 'I' &&
> (GET_ROM8(&(hmep->hme_romp[i+3])) & 0xff) == 'R') {
> vpd_base =
> (int)((GET_ROM8(&(hmep->hme_romp[i+8])) & 0xff) |
> (GET_ROM8(&(hmep->hme_romp[i+9])) & 0xff) << 8);
> break; /* VPD pointer found */
> }
>
Accept. Apart from the Cstyle indentation (which admittedly is weird,
but it was weird before I touched it), there isn't really any runtime
difference. The code actually passes cstyle -cPp as it stands. (What I
did was make the minimal changes needed to pass Cstyle. I didn't want
to get into restructuring code too much... otherwise there are far far
worse places in this code.) I'm changing it anyway. (I think I did it
this way when I made the same change in eri.)
> * 2762: This line of code is quite twisted. You treat the a non
> boolean expression as a boolean expression in order to explicitly
> return a boolean value to a boolean variable. Why not just:
>
> doinit = ((hme->hme_flags & HMESTARTED) != 0);
>
> Which actually is a boolean expression.
Accept. I guess this is an area of preference. I find your suggestion
harder to read, though I suspect a naive compiler might generate crisper
code with your suggestion. (no if-else). In any case, I'll change it.
>
> * 3146: cstyle: no need for brackets.
Reject. I generally make a habit of using them even in these cases.
Two reasons:
* the brackets make it easier to read by virtue of creating extra
whitespace
* it helps avoid bugs created when someone adds extra code to the
result statement. granted, that's not too likely in this case...
NetBSD doesn't like the extraneous braces here, but the ON cstyle guide
explicitly permits them. Reference section 9.1 of the Cstyle guide at
http://opensolaris.org/os/community/documentation/getting_started_docs/cstyle.ms.pdf
>
> * 3177, 3179, 3221, 3244: cstyle: none of the expressions evaluate to
> a boolean. Change to: "if (<expr> != NULL)" ...
Accept. There were a few others with the same dvmaxh/dvmarh in the
source. I've fixed them as well. (They may not have shown up on the
review... likely they appeared in code I didn't change.
>
> * 3709: Is there a need for these macros? Why not have this code be
> unconditionally enabled at compile time?
Accept.
I had them in there for debugging. There could, in theory, be a need to
disable this functionality. (At one point I had bugs in the code, so it
let me separate the receive debug from the transmit.)
However, I suppose I can remove these macros, since I'm reasonably
confident that the code works on all hme variants now.
>
> * 3724-3726: The locking accomplishes nothing here.
Accept. Heh. Darren said the same thing. I was being paranoid
(assignment to a boolean is atomic anyway. :-)
>
> * 3802: cstyle: need blank line between locals and code.
Accept.
>
> * 4204: return a boolean_t
Accept. Nice catch! This was actually a bug.
>
> * 4227: please fix this comment with the new return semantics. It
> looks like this returns B_TRUE if the mblk was consumed (regardless of
> whether it was successfully transmitted), or B_FALSE if it wasn't.
Accept.
>
> * 4732: Who is this question for?
Accept. Oops. Stale left over. I was thinking about multiple unicast
filters. I removed the comment.
>
> * 5263,5264: It would be easier to read if these were down at 5434.
Reject. Line 5434 doesn't always get executed. (Only happens if a
receive interrupt occurs.) I need these variables to be set *all* the time.
>
> * 5871: To answer the XXX comment you added: No, you don't. The
> details behind that answer lie in i_dls_link_subchain(), which calls
> DLS_PREPARE_PKT() on every packet. This uses the DLS_STRIP_PADDING()
> macro to accomplish the same thing as this now redundant code in hme.
> The code here and at 6043 can thus be removed.
Accept. Hey, that's good news!
>
> * 6091-6100: cstyle: need brackets around the conditional expressions.
Accept. See, now you're just being inconsistent. :-)
>
> -Seb
I also tested these changes. Yay.
-- Garrett
_______________________________________________
driver-discuss mailing list
driver-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/driver-discuss
