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