if_msk.c link negotiation / packet drops

[Arnaud Lacombe]

Hi,

On Thu, Oct 13, 2011 at 5:54 PM, YongHyeon PYUN <pyunyh at gmail.com> wrote:
> On Thu, Oct 13, 2011 at 05:02:58PM -0400, Arnaud Lacombe wrote:
>> Hi,
>>
>> On Thu, Oct 13, 2011 at 4:47 PM, YongHyeon PYUN <pyunyh at gmail.com> wrote:
>> > On Thu, Oct 13, 2011 at 12:59:35AM -0700, perryh at pluto.rain.com wrote:
>> >> YongHyeon PYUN <pyunyh at gmail.com> wrote:
>> >> > On Wed, Oct 12, 2011 at 10:07:02AM -0400, Karim wrote:
>> >> > > ... why are we ORing the same call twice isn't the same thing
>> >> > > as calling it once:
>> >> > >
>> >> > > bmsr = PHY_READ(sc, E1000_SR) | PHY_READ(sc, E1000_SR);
>> >> >
>> >> > The E1000_SR_LINK_STATUS bit is latched low so it should be read
>> >> > twice.
>> >>
>> >> It might not be a bad idea to check the generated code to be sure
>> >> that the read _is_ being done twice. ?An optimizer might well come
>> >> to the same conclusion as Karim, and discard the "redundant" second
>> >> instance (unless there's a "volatile" declaration somewhere in the
>> >> expansion of PHY_READ, to explicitly indicate that it has side
>> >> effects).
>> >
>> > Last time I checked it, compiler generated correct code.
>> > Tried again on amd64 and I can still see the code is there.
>> >
>> What about other architecture (especially i386) ? which optimization
>
> Don't use i386 so I don't know.
>
>> level did you use ? which compiler version ?
>
> CURRENT, default optimization(O2).
>
>>
>> About the last question, I know for sure that there has been change in
>> FreeBSD's gcc between 7-STABLE, and FreeBSD -CURRENT.
>>
>> I agree with perryh@ than such hardware requirement _requires_ being
>> explicit in the code, ie proper `volatile' marking.
>>
>
> I'm not saying adding more safe belt is bad idea. If you have a
> patch please submit it. I don't like touching every PHY drivers.
>
Actually, it should not be needed, the generic implementation of
PHY_READREG() is doing an MIIBUS_READREG() which is forwarded to the
parent (miibusN here), then its parent (msk(4)).

my bad,
 - Arnaud


>>  - Arnaud
>>
>