On 02/ 5/10 03:20 PM, venugopal iyer wrote:
>
>
> Thanks for the comments, please find responses below. We will send
> out the updated webrev.
>
> thanks,
>
> -venu
>
>>
>> -----------------------------------------------------------------------------
>>
>>
>>
>> GD1: mii.h: line 431: should just be removed, since you removed this
>> parameter.
>>
>
> ACCEPT
>
>> GD2: mac_provider.h: line 171: Why using __ prefix? Is there some
>> reason the structure name needs to be hidden from drivers (it seems
>> to me that this might require additional casting in the framework)?
>
> We want to keep the corresponding structure opaque to drivers, and
> still ta ke advantage of type checking. Sure there are some casts
> needed as
> a side effect , but only a very few in the framework, and none in the
> drivers.
You can have struct foobar as a forward declaration and not provide the
actual details of struct foobar... You don't need to introduce a middle
structure for this purpose.
>
>
>> GD3: mac_provider.h: lines 356, 357. You should indicate that these
>> values are just private, without advertising the guilty party
>> (nxge). Is there a PSARC contract covering these interfaces, because
>> they seem like they are contract private instead of consolidation
>> private? (It seems really unfortunate that we need a special set of
>> defines here for a single driver. Why are these necessary? I'd
>> consider this a bug.)
>
> ACCEPT - I will update the comment. For some background,
> MAC_RING_RX_ENQUEUE was needed to workaround a nxge bug (6816730 nxge
> does
> not build RX packet chains when operating in interrupt mode) that has
> been
> fixed since then. I filed 6923555 to track this. There are other
> issues in
> the nxge TX path that required us to do serialization on TX, and these
> will
> be addressed in the future.
>
>>
>> GD4: mac.h: line 114-115: why did you delete this PSARC reference?
>>
>
> The bulk of that PSARC case is now obsoleted by the new changes. It's
> not very useful to have only some references here and there that are not
> updated as the code changes and new cases are defined. It could actually
> confuse more than help the reader.
Ok.
>
>> GD5: afe.c: LGTM. Although, it might be nice to have a short-cut for
>> the property capabilities. Say, "MC_PROPERTIES", which includes
>> MC_SETPROP, MC_GETPROP, and MC_PROPINFO. (This comment applies to
>> all the mac providers.)
>
> ACCEPT - I will add a short-cut, but I'm not committing to modify
> every driver to use that shortcut.
Fair enough! :-)
>
>>
>> GD6:arn_main.c: LGTM
>>
>> GD7:atge.h: Why?
>
> I needed to remove mac_flow.h inclusion from mac_provider.h, atge.h was
> referring to some definitions present in mac_flow.h.
Ok.
>
>>
>> GD8:atge_main.c: LGTM
>>
>> GD9:ath_main.c: LGTM
>>
>> GD10:atu.c: LGTM
>>
>> GD11:bfe.c: LGTM. I really wish this were converted to the common
>> MII framework, but not this case.
>>
>> GD12:bge_impl.h: LGTM
>>
>> GD13:bge_kstats.c: LGTM
>>
>> GD14:bge_main2.c: Line 40. I still think the version number ought
>> to be removed from this line.
>
> ACCEPT - OK, I'll remove it while I'm there.
>
>> GD15: bge_main2.c: Line 152, 153: We really should nuke these private
>> properties. Not this case, but it probably would have simplified the
>> changes.
>>
>
> I'd rather avoid more changes unless necessary at this point. I would
> prefer that filed a CR to track it instead.
Ok, can you please file one then?
>
>> GD16:bge_main2.c: line 831: You removed this, but why? Does the
>> framework do it automatically now?
>
> Yes.
>
>> GD16: bge_main2.c: line 994: I'd prefer a _NOTE(ARGUNUSED()) rather
>> than a blanket /*ARGSUSED*/ Ditto 1073, and anyplace else
>> /*ARGSUSED*/ appears.
>
> ACCEPT - I thought I had used the _NOTE() everywhere, I guess this one
> fell
> through the cracks.
>
>>
>> GD17:bge_recv2.c: LGTM
>>
>> GD18:bge_main.c: LGTM
>>
>> GD19bridge.c: LGTM
>>
>> GD20:dmfe_main.c: LGTM
>>
>> GD21:e1000g_mac.c: What changed? White space only?
>>
>
> the cast was added
>
>> GD22:e1000g_main.c: line 49. Another version that I wish we would
>> remove.
>>
>
> ACCEPT
Thank you.
>
>> GD23:e1000g_main.c: line 610. If we're removing the count of private
>> properties, then how will we iterate for the '?' in ndd (and I have
>> long wished for a way to discover a driver's private properties as
>> well, via dladm)? I need to go back and check that code.
>>
>
> The m_priv_props list is now NULL-terminated.
Ah, ok. Thanks.
>
>> GD24:e1000g_main.c: line 2862: This change I'm not as comfortable
>> with. Please have the e1000g maintainer review this. (Ted You?)
>
> Samuel Tu reviewed these changes as well.
Good!
>
>>
>> GD25:e1000g_rx.c: LGTM
>>
>> GD26:e1000g_tx.c: LGTM
>>
>> GD27:e1000g_stat.c: LGTM
>>
>> GD28:e1000g_sw.h: LGTM
>>
>> GD29:hme.c: LGTM
>>
>> GD30:hxge_impl.c: LGTM
>>
>> GD31:hxge_kstats.c: LGTM
>>
>> GD32:hxge_main.c: I need to review this more carefully, but ideally
>> have the hxge maintainer(s) review it (NSN?)
>
> Michael Speer who was with NSN until he joined the Crossbow team was
> one of
> the main authors and reviewed the changes. We've sent the code review
> request t o NSN as well.
Sounds good!
>
>>
>> GD33:hxge_rxdma.c: LGTM
>>
>> GD34:hxge_send.c: LGTM
>>
>> GD35:ibd.c: LGTM
>>
>> GD36:ibcm_arp_link.c: LGTM (although a bit surprising that its needed).
>>
>> GD37:igb_gld.c: lines 853, 871: These changes need more careful
>> review. Please have someone else review them.
>>
>
> Samuel also reviewed these changes.
Good!
>
>> GD38:igb_rx.c: LGTM
>>
>> GD39:igb_stat.c: LGTM
>>
>> GD40:igb_sw.h: LGTM
>>
>> GD41:igb_tx.c: LGTM
>>
>> GD42:ipw2100.c: LGTM
>>
>> GD43:ipw2200.c: LGTM
>>
>> GD44:iwh.c: LGTM
>>
>> GD45:iwk2.c: LGTM
>>
>> GD46:iwp.c: LGTM
>>
>> GD47:mii.c: lines 716-726: I'd recommend moving these to line 738
>> (i.e. just before used, without the intervening case statements).
>
> ACCEPT
>
>>
>> GD48:net80211_ioctl.c: LGTM
>>
>> GD49:zyd.c: line 920, 921. This test is not needed.
>
> ACCEPT
>
>>
>> GD50:mwl.c: LGTM
>>
>> GD51:mxfe.c: LGTM. I wish this one were based on MII, but its harder
>> to do... some models use a non-MII NWAY scheme.
>>
>> GD52:myri10ge.c: LGTM
>>
>> GD53:myri10ge_lro.c: LGTM
>>
>> GD54:myri10ge_var.h: lines 60-62 -- seems like this driver shouldn't
>> need netinet/in.h. I'd have tried to remove it. Possibly also the
>> other netinet/ headers.
>
> ACCEPT - Will try to remove, we will see if they are really needed
Thanks!
>
>>
>> GD55:nge_main.c: LGTM
>>
>> GD56:nge_rx.c: LGTM
>>
>> GD57:nge_tx.c: LGTM
>>
>> GD58:unm_nic_main.c: LGTM
>>
>> GD59:pcan.c: LGTM. Wish this used the net80211 framework.
>>
>> GD60:pcwl.c: LGTM
>>
>> GD61:rtls.c: LGTM. Warning, I have been planning on a conversion of
>> this driver to the MII framework. This work *could* conceivably
>> integrate before b135, and hence before your integration. If so, the
>> merge changes will be modest (ala afe's change in scope).
>>
>
> Thanks for the heads-up. We will merge as needed.
And it has so occurred. :-)
>
>> GD62:sfe_util.c: LGTM
>>
>> GD63:vr.c: LGTM. Please also have Joost Mulders review it. (I'd
>> like to see this one converted to MII as well, btw.)
>
> He no longer seems to be with Sun. We have sent the code review
> annoucement
> on the relevant opensolaris.org aliases, so if he is still following
> these
> aliases he had the chance to do the review.
Oh, that's news to me. Ok, well then I'll do the conversion of it to
MII after b134.
>
>>
>> GD64:yge.c: line 3368. Hmm... perhaps this is better handled via a
>> default: case in the following switch table?
>
> ACCEPT
>
>>
>> GD65:strsubr.c: LGTM
Thanks!
- Garrett
>
>>> Hi, folks
>>>
>>> We'd like to make the code available for review for the following
>>> additions as part of follow-up to the crossbow project:
>>>
>>> PSARC/2009/638 Public GLDv3 Interfaces
>>> PSARC/2009/501 Dynamic Ring Grouping on NICs
>>> PSARC/2009/448 pool dladm link property
>>> PSARC/2009/436 Anti-spoofing Link Protection
>>> PSARC/2009/364 dlstat and flowstat
>>>
>>> Webrev: http://cr.opensolaris.org/~tlc/crossbow_onnv130
>>>
>>> The changes also include some bug fixes. The changes/webrev is based on
>>> build 130. We will update the webrev once in a couple of weeks time to
>>> include some minor bug fixes, however we'd like to get started on the
>>> code review now.
>>>
>>> Thanks in advance, to Sebastien, Cathy, Girish and Garrett who have
>>> agreed to review the changes.
>>>
>>> Please send comments etc. to crossbow-discuss at opensolaris.org by
>>> the 15th of Jan. If anyone needs more time, please let us know.
>>>
>>> For internal folks, the webrev is @
>>> /net/nvtbld-x.sfbay/exportz/crossbow/crossbow_onnv130/webrev
>>>
>>> /net/nvtbld-x.sfbay/exportz/crossbow/crossbow_onnv130/webrev-closed
>>> The above also has cscope built.
