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.
Thanks, and seasons greetings from the crossbow team.
mii.h: line 431: should just be removed, since you removed this parameter.
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)?
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.)
mac.h: line 114-115: why did you delete this PSARC reference?
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.)
arn_main.c: LGTM
atge.h: Why?
atge_main.c: LGTM
ath_main.c: LGTM
atu.c: LGTM
bfe.c: LGTM. I really wish this were converted to the common MII
framework, but not this case.
bge_impl.h: LGTM
bge_kstats.c: LGTM
bge_main2.c: Line 40. I still think the version number ought to be
removed from this line.
bge_main2.c: Line 152, 153: We really should nuke these private
properties. Not this case, but it probably would have simplified the
changes.
bge_main2.c: line 831: You removed this, but why? Does the framework do
it automatically now?
bge_main2.c: line 994: I''d prefer a _NOTE(ARGUNUSED()) rather than a
blanket /*ARGSUSED*/ Ditto 1073, and anyplace else /*ARGSUSED*/ appears.
bge_recv2.c: LGTM
bge_main.c: LGTM
bridge.c: LGTM
dmfe_main.c: LGTM
e1000g_mac.c: What changed? White space only?
e1000g_main.c: line 49. Another version that I wish we would remove.
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.
e1000g_main.c: line 2862: This change I''m not as comfortable with.
Please have the e1000g maintainer review this. (Ted You?)
e1000g_rx.c: LGTM
e1000g_tx.c: LGTM
e1000g_stat.c: LGTM
e1000g_sw.h: LGTM
hme.c: LGTM
hxge_impl.c: LGTM
hxge_kstats.c: LGTM
hxge_main.c: I need to review this more carefully, but ideally have the
hxge maintainer(s) review it (NSN?)
hxge_rxdma.c: LGTM
hxge_send.c: LGTM
ibd.c: LGTM
ibcm_arp_link.c: LGTM (although a bit surprising that its needed).
igb_gld.c: lines 853, 871: These changes need more careful review.
Please have someone else review them.
igb_rx.c: LGTM
igb_stat.c: LGTM
igb_sw.h: LGTM
igb_tx.c: LGTM
ipw2100.c: LGTM
ipw2200.c: LGTM
iwh.c: LGTM
iwk2.c: LGTM
iwp.c: LGTM
mii.c: lines 716-726: I''d recommend moving these to line 738 (i.e. just
before used, without the intervening case statements).
net80211_ioctl.c: LGTM
zyd.c: line 920, 921. This test is not needed.
mwl.c: LGTM
mxfe.c: LGTM. I wish this one were based on MII, but its harder to
do... some models use a non-MII NWAY scheme.
myri10ge.c: LGTM
myri10ge_lro.c: LGTM
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.
nge_main.c: LGTM
nge_rx.c: LGTM
nge_tx.c: LGTM
unm_nic_main.c: LGTM
pcan.c: LGTM. Wish this used the net80211 framework.
pcwl.c: LGTM
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).
sfe_util.c: LGTM
vr.c: LGTM. Please also have Joost Mulders review it. (I''d like to
see
this one converted to MII as well, btw.)
yge.c: line 3368. Hmm... perhaps this is better handled via a default:
case in the following switch table?
strsubr.c: LGTM
I''ll review the others as I have time.
-- Garrett
Eric Cheng wrote:> 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.
>
> Thanks, and seasons greetings from the crossbow team.
>
> _______________________________________________
> crossbow-discuss mailing list
> crossbow-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss
>
Just a reminder, folks. Please send us your comments by the 15th of Jan. If anyone is interested in reviewing, but needs more time, please let us know. thanks in advance. -venu On Fri, 25 Dec 2009, Eric Cheng wrote:> 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. > > Thanks, and seasons greetings from the crossbow team. > > _______________________________________________ > crossbow-discuss mailing list > crossbow-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss >
Hi, I''ve reviewed the change for e1000g, igb, ixgbe, bge, rge, nge, xge: e1000g_main.c: line 79: Too many blank / tab? rge_rxtx.c: line 292: Should it be HCK_IPV4_HDRCKSUM_OK? Please be notified that igb Brussels support has been integrated into snv today. ixgbe LRO support has be integrated into snv yesterday. So you''ll need to modify the related code for the new change. Others look good for me. Thanks, Samuel On 2009-12-25 16:54, Eric Cheng wrote:> 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. > > Thanks, and seasons greetings from the crossbow team. > > _______________________________________________ > crossbow-discuss mailing list > crossbow-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss
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.>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.>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.> >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.> >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.>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>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.>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.> >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.> >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.>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> >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.>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.> >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>> 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.
Thanks for the comments. Responses below. The updated webrev will be made available. -venu>I''ve reviewed the change for e1000g, igb, ixgbe, bge, rge, nge, xge: > >e1000g_main.c: >line 79: Too many blank / tab? >ACCEPT>rge_rxtx.c: >line 292: Should it be HCK_IPV4_HDRCKSUM_OK? >REJECT - It was not before. Is there an existing bug?>Please be notified that igb Brussels support has been integrated into >snv today. ixgbe LRO support has be integrated into snv yesterday. So >you''ll need to modify the related code for the new change. >Thanks for the heads-up.> On 2009-12-25 16:54, Eric Cheng wrote: >> 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. >> >> Thanks, and seasons greetings from the crossbow team. >> >> _______________________________________________ >> crossbow-discuss mailing list >> crossbow-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss > _______________________________________________ > crossbow-discuss mailing list > crossbow-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss >
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. >> > > ACCEPTThank 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 neededThanks!> >> >> 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: LGTMThanks! - 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.
On 02/06/10 07:37, venugopal iyer wrote:> > Thanks for the comments. Responses below. The updated webrev will be > made available. > > -venu > >> I''ve reviewed the change for e1000g, igb, ixgbe, bge, rge, nge, xge: >> >> e1000g_main.c: >> line 79: Too many blank / tab? >> > > ACCEPT > >> rge_rxtx.c: >> line 292: Should it be HCK_IPV4_HDRCKSUM_OK? >> > > REJECT - It was not before. Is there an existing bug?Hi Venu, I checked the code in snv gate. It''s set as HCK_IPV4_HDRCKSUM. To follow the crossbow-1.4 change. It need to be set as HCK_IPV4_HDRCKSUM_OK for received side, right? Thanks, Samuel> >> Please be notified that igb Brussels support has been integrated into >> snv today. ixgbe LRO support has be integrated into snv yesterday. So >> you''ll need to modify the related code for the new change. >> > > Thanks for the heads-up. > > >> On 2009-12-25 16:54, Eric Cheng wrote: >>> 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. >>> >>> Thanks, and seasons greetings from the crossbow team. >>> >>> _______________________________________________ >>> crossbow-discuss mailing list >>> crossbow-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss >> _______________________________________________ >> crossbow-discuss mailing list >> crossbow-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss >>
On Feb 5, 2010, at 5:16 PM, Garrett D''Amore wrote:> 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.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. 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-- Nicolas Droux - Solaris Kernel Networking - Sun Microsystems, Inc. nicolas.droux at sun.com - http://blogs.sun.com/droux
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 >> >
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 >
On 02/ 7/10 09:40 PM, Samuel Tu wrote:> > > On 02/06/10 07:37, venugopal iyer wrote: >> >> Thanks for the comments. Responses below. The updated webrev will be >> made available. >> >> -venu >> >>> I''ve reviewed the change for e1000g, igb, ixgbe, bge, rge, nge, xge: >>> >>> e1000g_main.c: >>> line 79: Too many blank / tab? >>> >> >> ACCEPT >> >>> rge_rxtx.c: >>> line 292: Should it be HCK_IPV4_HDRCKSUM_OK? >>> >> >> REJECT - It was not before. Is there an existing bug? > > Hi Venu, > > I checked the code in snv gate. It''s set as HCK_IPV4_HDRCKSUM. To > follow the crossbow-1.4 change. It need to be set as > HCK_IPV4_HDRCKSUM_OK for received side, right?ACCEPT - will do another sweep to make sure they were all updated in all drivers. Thanks, Nicolas.> > Thanks, > Samuel > >> >>> Please be notified that igb Brussels support has been integrated into >>> snv today. ixgbe LRO support has be integrated into snv yesterday. So >>> you''ll need to modify the related code for the new change. >>> >> >> Thanks for the heads-up. >> >> >>> On 2009-12-25 16:54, Eric Cheng wrote: >>>> 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. >>>> >>>> Thanks, and seasons greetings from the crossbow team. >>>> >>>> _______________________________________________ >>>> crossbow-discuss mailing list >>>> crossbow-discuss at opensolaris.org >>>> http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss >>> _______________________________________________ >>> crossbow-discuss mailing list >>> crossbow-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss >>> > _______________________________________________ > crossbow-discuss mailing list > crossbow-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss