I've updated the webrev ( http://cr.opensolaris.org/~gdamore/hme-mii )
and I've tested the private link properties. (At least I can change
their values with dladm and ndd. I don't have equipment capable of
verifying that the actual physical link timing parameters have been
changed. Hopefully nobody uses these inter-packet gap tunables anyway.)
- Garrett
Garrett D'Amore wrote:
> Girish Moodalbail wrote:
>> On 10/12/09 22:16, Garrett D'Amore wrote:
>>> All,
>>>
>>> I've converted hme to the common MII framework (and hence to
>>> Brussels). This fixes at least one bug (it didn't work at forced 10
>>> Mbps!), and cleans up a *a lot* of code. hme shrinks by about 2200
>>> lines.
>>>
>>> The webrev is here:
>>>
>>> http://cr.opensolaris.org/~gdamore/hme-mii
>>
>> Hello Garrett,
>>
>> I have looked at only Brussels interfaces and here are my comments:
>>
>> hme.c:
>>
>> L123-L128: Since all the 4 private properties are per-interface, the
>> comments above those lines are incorrect. They talk about making
>> these parameters per-interface (which is already per-interface) and
>> using ndd command.
>
> Good catch. I"m just remove the comment.
>
>>
>> L130-133: Do we really need these to be global variables? They are
>> used only for initiation and so can be represented as macros right.
>
> They could be macros, but making them globals also allows them to be
> adjusted via /etc/system. I'm not going to change them for now,
> because there simply isn't any compelling need to do so.
>
>>
>> L1917: rv should be reset to 0. otherwise when everything is fine we
>> will return (ENOTSUP) or else @L1950 return (ENOTSUP) and L1956
>> return (0)
>
> Accept. I guess I didn't try changing these parameters. Need to
> cover that with more testing.
>
>>
>> L1932: There is a missing 'else' clause.
>
> Accept, good catch.
>
>>
>> L72, L90, L127, L269: NIT: no need for ','
> Yeah, but C is ok with it either way. I like to leave the last , in
> place, so if another member gets added to the end, you don't have to
> remember to add one without creating a syntax error. As this is
> mostly a stylistic preference, I'm going to leave it alone for now.
>
> Thanks for reviewing this stuff.
>
> - Garrett
>>
>> ~Girish
>
> _______________________________________________
> networking-discuss mailing list
> networking-discuss at opensolaris.org
