sowmini.varadhan at sun.com wrote:
> 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.
>
This is weird.
Out of curiosity, why is sad.h needed? I don't see anything in it that
I can think of dld.h as implicitly needing.
Its unfortunate that sad.h pollutes the namespace this way, but I think
it was designed with the idea that only the sad driver itself, along
with whatever userland component is associated with it (autopush?) would
include it.
I also note that those portions of the namespace that it pollutes with
do not appear to be documented anywhere. Maybe they could be changed?
(I guess this would impact userland components like autopush?)
>
>> 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?
>
I don't think QE has a problem with bge busted hardware, do they? I
know that *Sun* NICs have an implementation flaw, but I wasn't aware of
any relating to bge.
The EBUSY idea has merit, if you're going to leave this in.
> 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..
>
It seems like you could probably activate this in the descriptor rings,
so you don't have to check on it at run time. Of course, that means a
full device reset (well at least reprogramming the rx rings) is probably
required.
>
>> 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.
>
Thanks, that was exactly what I meant. :-)
>
>> 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
>
Ah, I didn't realize you were staging delivery this way. OK.
> 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!)
>
Good! (I don't know when I'll have time to review it though.)
Thanks!
-- Garrett
