netdev - Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1ad9f9a5-8f39-40bd-94bb-6b700f30c4ba@pobox.com>
Date:   Tue, 25 Jun 2019 13:37:30 -0500
From:   Daniel Santos <daniel.santos@...ox.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>,
        René van Dorst <opensource@...rst.com>
Cc:     sean.wang@...iatek.com, f.fainelli@...il.com, davem@...emloft.net,
        matthias.bgg@...il.com, andrew@...n.ch, vivien.didelot@...il.com,
        frank-w@...lic-files.de, netdev@...r.kernel.org,
        linux-mediatek@...ts.infradead.org, linux-mips@...r.kernel.org
Subject: Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

Hello,

Although I'm new to the entire Ethernet / *MII subsystem and I haven't
touched DSA yet, I've recently had to add some of this functionality to
the older OpenWRT drivers for swconfig control over the ports.  René, do
you have an actual datasheet or programming guide for the mt7530?  I
only have one for the mt7620.


On 6/25/19 7:10 AM, Russell King - ARM Linux admin wrote:
> mac_link_*().
>
>>>> +            if (state->pause || phylink_test(state->advertising, Pause))
>>>> +                    mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
>>>> +            if (state->pause & MLO_PAUSE_TX)
>>>> +                    mcr |= PMCR_TX_FC_EN;
>>>> +            if (state->pause & MLO_PAUSE_RX)
>>>> +                    mcr |= PMCR_RX_FC_EN;
>>> This is clearly wrong - if any bit in state->pause is set, then we
>>> end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set.  If we have Pause
>>> Pause set in the advertising mask, then both are set.  This doesn't
>>> seem right - are these bits setting the advertisement, or are they
>>> telling the MAC to use flow control?
>> Last one, tell the MAC to use flow control.
> So the first if() statement is incorrect, and should be removed
> entirely.  You only want to enable the MAC to use flow control as a
> result of the negotiation results.

René,
iiuc, this is what's documented in table 28B-3 of the 802.3 spec on page
598.  pdf of section 2 here:
http://www.ismlab.usf.edu/dcom/Ch3_802.3-2005_section2.pdf

>> On the current driver both bits are set in a forced-link situation.
>>
>> If we always forces the MAC mode I think I always set these bits and don't
>> anything with the Pause modes? Is that the right way to do it?
> So what happens if your link partner (e.g. switch) does not support
> flow control?  What if your link partner floods such frames to all
> ports?  You end up transmitting flow control frames, which could be
> sent to all stations on the network... seems not a good idea.
>
> Implementing stuff properly and not taking short-cuts is always a
> good idea for inter-operability.

But will there still be a mechanism to ignore link partner's advertising
and force these parameters?  I've run into what appears to be quirks on
two separate NICs or their drivers, a tp-link tg-3468 (r8169) and an
Aquantia AQC107 802.3bz (atlantic) where these link partners aren't
auto-negotiating correctly after I switch the mt7530 out of
auto-negotiation mode.  Of course, it could be a mistake I've made (and
should thus be discussed elsewhere), but iirc, I had to force enable
flow control and then also disable auto-negotiation on the link partner
and force the mode I wanted.

Cheers,
Daniel