netdev - Re: phylink: flow control on fixed-link not working.

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]
Date:   Wed, 17 Jul 2019 22:58:16 +0000
From:   René van Dorst <opensource@...rst.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     netdev@...r.kernel.org
Subject: Re: phylink: flow control on fixed-link not working.

Quoting Russell King - ARM Linux admin <linux@...linux.org.uk>:

> On Wed, Jul 17, 2019 at 09:31:11PM +0000, René van Dorst wrote:
>> Hi,
>>
>> I am trying to enable flow control/pause on PHYLINK and fixed-link.
>>
>> My setup SOC mac (mt7621) <-> RGMII <-> SWITCH mac (mt7530).
>>
>> It seems that in fixed-link mode all the flow control/pause bits are cleared
>> in
>> phylink_parse_fixedlink(). If I read phylink_parse_fixedlink() [0]  
>> correctly,
>> I see that pl->link_config.advertising is AND with pl->supprted  
>> which has only
>> the PHY_SETTING() modes bits set. pl->link_config.advertising is  
>> losing Pause
>> bits. pl->link_config.advertising is used in phylink_resolve_flow()  
>> to set the
>> MLO_PAUSE_RX/TX BITS.
>>
>> I think this is an error.
>> Because in phylink_start() see this part [1].
>>
>>  /* Apply the link configuration to the MAC when starting. This allows
>>   * a fixed-link to start with the correct parameters, and also
>>   * ensures that we set the appropriate advertisement for Serdes links.
>>   */
>>  phylink_resolve_flow(pl, &pl->link_config);
>>  phylink_mac_config(pl, &pl->link_config);
>>
>>
>> If I add a this hacky patch below, flow control is enabled on the  
>> fixed-link.
>>         if (s) {
>>                 __set_bit(s->bit, pl->supported);
>> +               if (phylink_test(pl->link_config.advertising, Pause))
>> +                       phylink_set(pl->supported, Pause);
>>         } else {
>>
>> So is phylink_parse_fixedlink() broken or should it handled in a other way?
>
> Quite simply, if the MAC says it doesn't support pause modes (i.o.w.
> the validate callback clears them) then pause modes aren't supported.

Hi Russel,

Thanks for your response.

I believe that I am setting pause bits right on both ends see SOC [0] and
SWITCH [1] and also in the DTS [2].

Correct me if it is not the right way.


Maybe I am looking in the wrong part of the code.
But I added many debug lines in phylink_parse_fixedlink() [3] to see what
happens with the Pause bit in the pl->link_config.advertising and  
pl->supported.


This is the dmesg output.
[    1.991245] libphy: Fixed MDIO Bus: probed
[    2.031260] phylink_create: config0: Pause
[    2.039410] phylink_create: supported: Pause
[    2.047904] mtk_validate: mask: Pause
[    2.055186] mtk_validate: supported: Pause
[    2.063332] mtk_validate: advertising: Pause
[    2.071825] phylink_create: config1: Pause
[    2.079966] phylink_create: config2: Pause
[    2.088132] phylink_parse_fixedlink: config: Pause
[    2.097660] phylink_parse_fixedlink: support: Pause
[    2.107366] mtk_validate: mask: Pause
[    2.114647] mtk_validate: supported: Pause
[    2.122792] mtk_validate: advertising: Pause
[    2.131283] phylink_parse_fixedlink: config2: Pause
[    2.140971] phylink_parse_fixedlink: support2: Pause
[    2.150845] phylink_parse_fixedlink: config3: Pause
[    2.160546] phylink_parse_fixedlink: support3: Pause
[    2.170420] phylink_parse_fixedlink: config4: Pause
[    2.180120] phylink_parse_fixedlink: config5: Pause

[    5.854674] mt7530 mdio-bus:1f: configuring for fixed/trgmii link mode
[    5.867665] phylink_resolve_flow: PAUSE_AN: pause: 0, 12, 8dfba630
[    5.867670] phylink_resolve_flow: new_pause: 0
[    5.879980] mt7530 mdio-bus:1f: phylink_mac_config:  
mode=fixed/trgmii/1Gbps/Full adv=00,00000000,00000220 pause=12 link=1  
an=1
[    6.651239] DSA: tree 0 setup
[    6.658192] input: gpio-keys as /devices/platform/gpio-keys/input/input0
[    6.672108] mt7530 mdio-bus:1f: phylink_mac_config:  
mode=fixed/trgmii/1Gbps/Full adv=00,00000000,00000220 pause=12 link=1  
an=1
[   28.937543] mtk_soc_eth 1e100000.ethernet eth0: configuring for  
fixed/trgmii link mode
[   28.965884] mtk_soc_eth 1e100000.ethernet eth0: phylink_mac_config:  
mode=fixed/trgmii/1Gbps/Full adv=00,00000000,00000220 pause=12 link=1  
an=1
[   29.000740] mtk_soc_eth 1e100000.ethernet eth0: phylink_mac_config:  
mode=fixed/trgmii/1Gbps/Full adv=00,00000000,00000220 pause=12 link=1  
an=1
[   29.026392] mtk_soc_eth 1e100000.ethernet eth0: Link is Up -  
1Gbps/Full - flow control off
[   29.373577] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready


I don't see the "config6:" [4] debug.
I think the pause bits are always cleared in  
pl->link_config.advertising by phylink_parse_fixedlink()

Again I may understand the code wrong or I am looking at the wrong place.
So I hope you can point me in the right direction.

Greats,

René


[0]:  
https://github.com/vDorst/linux-1/blob/8538cdefd425592d249a71445c466159b0f27475/drivers/net/ethernet/mediatek/mtk_eth_soc.c#L502
[1]:  
https://github.com/vDorst/linux-1/blob/8538cdefd425592d249a71445c466159b0f27475/drivers/net/dsa/mt7530.c#L1468
[2]:  
https://github.com/vDorst/linux-1/blob/8538cdefd425592d249a71445c466159b0f27475/drivers/staging/mt7621-dts/UBNT-ER-e50.dtsi#L122
[3]:  
https://github.com/vDorst/linux-1/blob/8538cdefd425592d249a71445c466159b0f27475/drivers/net/phy/phylink.c#L214
[4]:  
https://github.com/vDorst/linux-1/blob/8538cdefd425592d249a71445c466159b0f27475/drivers/net/phy/phylink.c#L263

>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up