Thanks very much for the review! > Sorry I didn't get to this sooner: > > hme.c: Why this change? Did you define a common GETSTRUCT() somewhere > that collides? (If so, that would be a poor choice of name.) (You had me > going there... I thought maybe you had converted hme to Brussels. :-)
sad.h has a GETSTRUCT definition which got pulled in via dld.h, hence the change. > > nge_kstats.c: It looks like you nuked the kstats, but it doesn't look like > you did any other conversion for nge? Is that correct? That is correct. This function got left behind when PSARC 2007/396 was putback. > bge_kstats.c: line 672: you should be able to remove this. don't bother > leaving it in the final code. Ok. Thanks for the confirmation. I believe Zeeshanul is also looking at this bug; one of us (whoever goes in first) will take care of this. > bge_main2.c: line 857-869: shouldn't there by symbolic constants (#defines) > for these values? Pretty please? It looks like maybe you could borrow > the LINK_FLOW_CTRL constants you used at lines 1011-1042. Accept. > line 1188, 1189: I'm trying to understand the purpose of the prototype. > And making it inline is probably not ideal. (In fact, that function should > rarely be called. Inlining serves no perf. benefit, and probably hurts > debugging. And maybe memory size as well.) Accept. > line 1257-1282: I'm not thrilled that this is here. Did bge have it vi > ndd before? And in particular, it may require renegotiation with IP (the > tx checksum facility in particular.) I don't see where that is done, so I > don't think the value is binding unless IP is replumbed. It looks like you > did this for lines 1519-1522 as well. I'd just rip them out. (And again > at 3033.) > > bge_recv2.c: line 238/239. See above comments. I'd not bother to make > this conditional. And in fact, these checks you've done here are bad > because they add two conditionals on a very hot code path. Please remove. No, this was not available via ndd before, but as I mentioned in PSARC, we decided to do it as a private property to provide a back-door to QE. As you correctly point out, it's something that will require replumbing, and, more importantly (as we discussed in the commitment review), the real solution is to fix the bugs instead of providing this back-door. It's because this is intended as a temporary back-door that we decided not to go to any great pains to avoid the replumbing requirement. At the same time, given that QE does suffer from our broken checksum implementation and need a back-door, I think there is some value in keeping this private property. Perhaps we should examine the bge_mac_state and return EBUSY if the mac has been started, to make it clear that a replumb is needed? As for the perf requirement, let me check with Ted You to see if there are ways to mitigate this, and get back to you.. > bge_ndd.c: line 33. Surely this warning should come from the MAC layer > rather than the driver? Nemo could intercept the ndd ioctl, and issue the > warning. (It can also look to see if the driver already supports brussels > at that point, before issuing the warning.) That's an interesting suggestion. You are proposing that mac_ioctl examine the mc_callbacks for (MC_SETPROP|MC_GETPROP), and issue the warning if the ((struct iocblk *)mp->b_rptr)->ioc_cmd is ND_SET or ND_GET, right? yes, that's a good idea- I'll implement it. > > I'm also surprised by bge_ndd.c... it looks like full NDD support is still > present here. Why? I was hoping to see a lot of code just ripped out, and > handled by Brussels. (It appears that you don't have code to intercept NDD > ioctls for Brussels drivers? I'm a little disappointed. Maybe this was a > Phase II deliverable.) no, that's part of the ndd compatiblity component Recall that the Brussels umbrella case defined 3 major design components: - framework - persistence - ndd-compat don't lose heart- the ndd compat is in the pipeline, prototyped and almost ready to go! (and you can get a head-start on that review at /net/npt.sfbay/export/brussels/brussels-gate on SWAN!) > strplumb.c: Why this change? > > ipw2100.c and ipw2100.h: I'm not sure these changes are terribly relevant > to Brussels. both of the above changes are related to each other. In onnv today, dld.h does not have to include mac.h (there's nothing in dld.h that needs this). So we went and cleaned up some of the header files, and the changes you see there are part of that cleanup. As Artem pointed out: Artem> I did I bit of a header cleanup. There was a circular header Artem> dep: Brussels needed dld.h definitions in mac.c, but dld.h Artem> included mac.h though it didn't have to. So dld.h no longer Artem> includes mac.h and dls.h. Hences the changes. Nothing worth Artem> discussing. --Sowmini
