RE: [PATCH net-next v4 2/6] r8169: add support for phylink

[Javen <javen_xu@realsil.com.cn>]

>
>> +     if (jumbo) {
>> +             if (!tp->jumbo_pause_saved) {
>> +                     struct ethtool_link_ksettings cmd = {};
>> +                     bool adv_pause, adv_asym;
>> +
>> +                     phylink_ethtool_get_pauseparam(tp->phylink, &tp-
>>saved_pause);
>> +                     if (tp->saved_pause.autoneg) {
>> +                             phylink_ethtool_ksettings_get(tp->phylink, &cmd);
>> +                             adv_pause = ethtool_link_ksettings_test_link_mode(&cmd,
>> +                                                                               advertising,
>> +                                                                               Pause);
>> +                             adv_asym  = ethtool_link_ksettings_test_link_mode(&cmd,
>> +                                                                               advertising,
>> +                                                                               Asym_Pause);
>> +                             if (adv_pause && !adv_asym) {
>> +                                     tp->saved_pause.rx_pause = 1;
>> +                                     tp->saved_pause.tx_pause = 1;
>
>This does not look correct. Pause is negotiated. In order to determine how to
>program the MAC you need to look at what the local side is advertising, and
>what the link peer is advertising. I don't see anything here about lp_.
>
>I forget what the issues is. Is it something like, if you are using a normal MTU,
>pause is supported? But with jumbo MTU it is not?

Yes, exactly. Pause is supported with normal MTU, but not with jumbo MTU.

The old non-phylink code handled this by clearing Pause and Asym_Pause
from phydev->advertising when jumbo MTU was enabled, and then restarting
autoneg:

        linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT,
                           tp->phydev->advertising);
        linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
                           tp->phydev->advertising);
        phy_start_aneg(tp->phydev);

With phylink, directly modifying phydev->advertising does not look like
the right thing to do, since phylink owns the advertised link modes.

What would be the correct phylink model for this?

Is there an existing phylink API which allows a MAC driver to update the
local advertisement and trigger autoneg after an MTU change? Or would this
require a new phylink helper/hook for dynamic MAC pause capabilities,
since the pause capability depends on the current MTU?

>
>For this to work correctly, i would expect a change of MTU to trigger a new
>autoneg, with the local advertisement changed. That might require changes in
>phylink, since it does not expect this sort of thing.
>
>> @@ -5288,6 +5326,8 @@ static void rtl_remove_one(struct pci_dev *pdev)
>>               r8169_remove_leds(tp->leds);
>>
>>       unregister_netdev(tp->dev);
>> +     if (tp->phylink)
>> +             phylink_destroy(tp->phylink);
>
>
>I've not looked in detail, but is tp->phylink optional? I would expect the probe
>to fail if it could not create it.

Yes. I will remove this check.

>
>> +static int rtl_mac_enable_tx_lpi(struct phylink_config *config, u32
>> +timer, bool tx_clk_stop) {
>> +     struct rtl8169_private *tp = container_of(config, struct
>> +rtl8169_private, phylink_config);
>> +
>> +     if (!rtl_supports_eee(tp))
>> +             return -EOPNOTSUPP;
>
>Can that happen? You should only be telling phylink EEE is supported if EEE is
>actually supported.

Agreed. rtl8169_get_lpi_caps() returns 0 when EEE is not supported, so
phylink should not call mac_enable_tx_lpi() for such devices.
I will remove the redundant check.

>
>> +static int rtl_init_phylink(struct rtl8169_private *tp) {
>> +     struct phylink *pl;
>> +     phy_interface_t phy_mode;
>> +
>> +     tp->phylink_config.dev = &tp->dev->dev;
>> +     tp->phylink_config.type = PHYLINK_NETDEV;
>> +     tp->phylink_config.mac_managed_pm = true;
>> +     tp->phylink_config.lpi_capabilities = rtl8169_get_lpi_caps(tp);
>> +     tp->phylink_config.mac_capabilities |= MAC_ASYM_PAUSE |
>> + MAC_SYM_PAUSE;
>> +
>> +     if (tp->sfp_mode) {
>> +             phy_mode = PHY_INTERFACE_MODE_INTERNAL;
>> +             tp->phylink_config.mac_capabilities |= MAC_10000FD;
>
>Only 10G? Is it not possible to slow down to 1G for a 1G SFP?

It can support 1G SFP as well.

I only added MAC_10000FD here because the original code path only added
the 10G capability, and I was trying to keep the behavior unchanged in
this conversion patch.

BRs,
Javen
>
>        Andrew