[brussels-dev] [networking-discuss] Code review request for Brussels Framework.

[sowmini.varadhan at sun.com]

On (11/08/07 13:27), Garrett D'Amore wrote:
>
> 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?)
>

don't know- Brussels inherited this part from clearview-uv- maybe
someone there can clarify?

<..discussion about private property for checksum offload snipped..>

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

Ok. As I said, let me confer with the Beijing team and get back on
this one. 

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

Consider it accepted! (will spin out a new webrev after I coalesce
the review comments)

Thanks very much for the review!

--Sowmini