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

[Andrew Lunn <andrew@lunn.ch>]

> +	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?

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.

> +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.

> +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?

	Andrew