Steven, Thank you very much for your review. I answer in lines.
----- Original Message ----- >Date: Wed, 11 Jun 2008 21:01:57 -0500 >Alan DuBoff wrote: >> On Wed, 11 Jun 2008, Steven Stallion wrote: >> >>> Not sure if the process is the same for Sun internal, but: >>> >>> If you look on the update profile page on os.o there is a place you can >>> define up to 3 public keys (it is located near the bottom). These are >>> used >>> when you scp/sftp to cr.os.o, just make sure your username is the same as >>> your profile. >> >> I guess for some reason I always try to get there from SWAN and it >> doesn't work, I can access by replacing the key and going from external... >> >>> I would be more than happy to review - I've looked at sfe a bit in the >>> past so I am somewhat familiar with it. Once the webrev is up, I'll go >>> through the changes in detail. >>> >>> HTH! >> >> That would actually help out a lot! Both me and Masa would appreciate it. >> >> http://cr.opensolaris.org/~aland/webrev-6655415/ >> >> I just updated the nits that Jim Carlson pointed out, and refreshed it >> and it's up now. >> >> -- >> >> Alan DuBoff - Solaris x86 IHV/OEM Group > >I'll start just with sfe.c since there are a number of questions/issues >I see (mostly cosmetic/clarity): > >sfe.c >----- >902 stray newline; remove I agree. >(the changes on lines 1140 - 1180 really bug me): > >1146 tmp0 is not meaningful - why was the assignment broken out? IMHO > having this broken apart makes the code more difficult to read. >1147 tmp1 is not meaningful - see comments above (line 1146) >1177 this line should not have changed; using a tmp variable here has > no benefit >1178 I would have handled this differently. On line 1170, I would > have: 'mark |= dcp->dmac_size;', and then > 'tdp->d_cmdsts = LE_32(mark);' on line 1180. The reason of changing line 1140 - 1180 is for performance. As LE_32() is a macro, LE_32(p->xxx) will cause several memory references to the same location by a pointer, that are difficult for compilers to optimize. For line 1178, I think you are right. I think I can change the line if there are not other comments. >1188 Remove; use 'dp->gc.gc_tx_ring_size' directly on line 1194 The reason that I didn't use dp->gc.gc_tx_ring_size directly, is the variable in the function is used twice or more in the original code I distribute from my web site. Currently I continue to maintain both simplified sfe.c in opensolaris and the original code in my web site. I'd like to make the line same with the original code to maintain them easily. >1216 & 1217 See comments for lines 1146 & 1147 It is for speed. >1243 See comments on line 1188 > >1256 & 1257 Don't break the statements apart; clutter The reason is for supporting big endian CPUs, i.e. sparc. LE_32(tdp->d_cmdsts) is not atomic in sparc. LE_32() macro generates several memory references to read the same word in the same descriptor. It will corrupt the result of LE_32() if the nic will update the descriptor at the same time. >1437 See comment for line 1188 The same reason with line 1188 >1498 See comment for line 1188 The same reason with line 1188 >1654 Either use a macro for the mask, or document the need; > Also, either document the magic numbers or provide macros > (see lines 1640 - 1642) I'll try to use macros, which are defined according to the datasheet. >1675 - 1708 Vertical whitespace please! The statements run together > and its difficult to discern exactly what is going on > here. I agree. I'll insert vertical whitespaces after line 1686 and line 1700. > Document the wait and the delay - are we waiting > for the interface to settle ? Yes, I'll document that. >1737 Comment is useless; either explain why we are doing this or not > at all; its pretty obvious it is doing something 32 times. I'd like to describe the number of the bits is 32. In some linux nic drivers, they send 33 bits. But according to the datasheets, 32 seems right. I'll improve the comment to be valuable. >1712 (old) Why was this comment removed ? Sorry, I could not remember the reason. I'll make it alive again. >2060 Not sure why this was marked as a change; check to make sure you >do not have any extraneous whitespace (cstyle < sfe.c) The reason was a comma(,) at the end of the line. It must be a semicolon(;). -masa >Steve > >-- >Yet magic and hierarchy >arise from the same source, >and this source has a null pointer. > >Reference the NULL within NULL, >it is the gateway to all wizardry. >_______________________________________________ >driver-discuss mailing list >driver-discuss@opensolaris.org >http://mail.opensolaris.org/mailman/listinfo/driver-discuss _______________________________________________ driver-discuss mailing list driver-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/driver-discuss
