On 02/ 8/10 10:22 AM, Garrett D'Amore wrote:
> On 02/ 8/10 09:37 AM, Nicolas Droux wrote:
>>
>>> 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.
>> The new handle is declared completely independently of the underlying
>> structure, and consistently with existing opaque handles defined by
>> mac_provider.h. It's also consistent with the declaration of other DDI
>> opaque handles. I don't see how a handful of type casts in the
>> framework would be an issue.
>
> I know that some portions of the DDI use two separate structure
> definitions, which can be useful in certain circumstances -- usually
> when the "impl" version of the structure encloses more fields than the
> real structure.
>
> Generally however, its better not to separate the types completely,
> because you lose some protection that the compiler provides, and have to
> disable certain lint warnings. Casting is a compiler escape for these
> checks, and you really don't want to use it unless you have no other
> reasonable choice.
>
> The better choice (IMO) here is...
>
> In the exported header (mac_provider.h for example):
>
> typedef struct mac_handle *mac_handle_t;
>
> Then in the mac_impl.h (or whatever else you have) that is not included
> in user code:
>
> struct mac_handle {
> ... provide structure details here ...
> };
>
> Then mac clients can safely use mac_handle_t, but they will get a
> compiler error if they try to dereference it (assuming that they don't
> #include mac_impl.h).
>
> This provides checks that casting or other approaches (such as use of
> void *) won't. If they they pass a pointer to the wrong type, for
> example, to a function, then the compiler will *catch* this.We don't use a void *. The drivers will take advantage of type checking, and don't have to do anything special for lint. > You also get the same benefit in the core -- if you misuse types, you'll > get a compiler error or warning, that a cast would suppress. You *may* > also benefit from some other optimizations, where the compiler can > figure out that the types are guaranteed to be aligned properly for > example. (There is a lot of logic in the compilers to optimize certain > things that using casts disables because it defeats some of the > assumptions that the optimizer can otherwise make.) From the framework side there are only a handful of typecasts which are very well contained in the entry points dealing with this type. There are dozens of similar uses of opaque handles declared the same way throughout the driver and client interfaces, which have never caused problems. Also these entry points are not performance critical and the potential optimizations you are referring to wouldn't have any tangible impact. Nicolas. > > -- Garrett >> Nicolas. >> >>>> >>>>> 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. >>> _______________________________________________ >>> crossbow-discuss mailing list >>> crossbow-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss >
