netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrew Lunn <andrew@lunn.ch>
To: Helmut Grohne <helmut.grohne@intenta.de>
Cc: Florian Fainelli <f.fainelli@gmail.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	netdev@vger.kernel.org, Woojung Huh <woojung.huh@microchip.com>,
	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
	Vivien Didelot <vivien.didelot@gmail.com>
Subject: Re: [PATCH] net: phy: phy_remove_link_mode should not advertise new modes
Date: Wed, 15 Jul 2020 21:27:22 +0200	[thread overview]
Message-ID: <20200715192722.GD1256692@lunn.ch> (raw)
In-Reply-To: <20200714082540.GA31028@laureti-dev>

On Tue, Jul 14, 2020 at 10:25:42AM +0200, Helmut Grohne wrote:
> When doing "ip link set dev ... up" for a ksz9477 backed link,
> ksz9477_phy_setup is called and it calls phy_remove_link_mode to remove
> 1000baseT HDX. During phy_remove_link_mode, phy_advertise_supported is
> called.
> 
> If one wants to advertise fewer modes than the supported ones, one
> usually reduces the advertised link modes before upping the link (e.g.
> by passing an appropriate .link file to udev).  However upping
> overrwrites the advertised link modes due to the call to
> phy_advertise_supported reverting to the supported link modes.
> 
> It seems unintentional to have phy_remove_link_mode enable advertising
> bits and it does not match its description in any way. Instead of
> calling phy_advertise_supported, we should simply clear the link mode to
> be removed from both supported and advertising.

We have two different reasons for removing link modes.

1) The PHY cannot support a link mode. E.g.

static int bcm84881_get_features(struct phy_device *phydev)
{
        int ret;

        ret = genphy_c45_pma_read_abilities(phydev);
        if (ret)
                return ret;

        /* Although the PHY sets bit 1.11.8, it does not support 10M modes */
        linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
                           phydev->supported);
        linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
                           phydev->supported);

        return 0;
}

This is done very early on, as part of probing the PHY. This is done
before supported is copied into advertised towards the end of the PHYs
probe.

2) The MAC does not support a link mode. It uses
phy_remove_link_mode() to remove a link mode. There are two different
times this can be done:

a) As part of open(), the PHY is connected to the MAC. Since the PHY
is not connected to the MAC until you open it, you cannot use ethtool
to change the advertised modes until you have opened it. Hence user
space cannot of removed anything and you don't need to worry about
this copy.

b) As part of the MAC drivers probe, the PHY is connected to the MAC.
In this case, ethtool can be used by userspace to remove link
modes. But the MAC driver should of already removed the modes it does
not support, directly after connecting the PHY to the MAC in its probe
function. So advertising and supported at the same already.

The key point here is ksz9477_phy_setup(), and how it breaks this
model. It is called from ksz_enable_port(). That is called via
dsa_port_enable() in dsa_slave_open(). But the PHY was connected to
the MAC during probe of the MAC. So we have a bad mix of a) and b),
which is leading to your problem. You need to fix the switch driver so
it cleanly does b), removes the link mode early on before the user has
chance to use ethtool.

       Andrew


  parent reply	other threads:[~2020-07-15 19:27 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-14  8:25 Helmut Grohne
2020-07-14 21:07 ` David Miller
2020-07-15  7:03   ` Helmut Grohne
2020-07-15 18:20     ` Jakub Kicinski
2020-07-15 19:01       ` Andrew Lunn
2020-07-15 18:51     ` Andrew Lunn
2020-07-15 19:27 ` Andrew Lunn [this message]
2020-07-16 12:57   ` [PATCH v2] net: dsa: microchip: call phy_remove_link_mode during probe Helmut Grohne
2020-07-16 14:10     ` Andrew Lunn
2020-07-17  8:18       ` Helmut Grohne
2020-07-17 13:18         ` Andrew Lunn
2020-07-20  9:04           ` [PATCH v3] " Helmut Grohne
2020-07-20 20:43             ` Andrew Lunn
2020-07-21 11:07               ` [PATCH v4] " Helmut Grohne
2020-07-21 15:20                 ` Andrew Lunn
2020-07-21 22:50                 ` David Miller
2020-07-20 21:04             ` [PATCH v3] " Andrew Lunn
2020-07-21  7:38               ` Helmut Grohne

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200715192722.GD1256692@lunn.ch \
    --to=andrew@lunn.ch \
    --cc=UNGLinuxDriver@microchip.com \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=helmut.grohne@intenta.de \
    --cc=hkallweit1@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=netdev@vger.kernel.org \
    --cc=vivien.didelot@gmail.com \
    --cc=woojung.huh@microchip.com \
    --subject='Re: [PATCH] net: phy: phy_remove_link_mode should not advertise new modes' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).