archive mirror
 help / color / mirror / Atom feed
From: Helmut Grohne <>
To: Andrew Lunn <>
Cc: Florian Fainelli <>,
	Heiner Kallweit <>,
	Russell King <>,
	"David S. Miller" <>,
	Jakub Kicinski <>, <>,
	Woojung Huh <>,
	"Microchip Linux Driver Support" <>,
	Vivien Didelot <>,
	Tristram Ha <>
Subject: [PATCH v3] net: dsa: microchip: call phy_remove_link_mode during probe
Date: Mon, 20 Jul 2020 11:04:18 +0200	[thread overview]
Message-ID: <20200720090416.GA7307@laureti-dev> (raw)
In-Reply-To: <>

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. Doing so reverts any previous change to advertised link modes
e.g. using a udevd .link file.

phy_remove_link_mode is not meant to be used while opening a link and
should be called during phy probe when the link is not yet available to

Therefore move the phy_remove_link_mode calls into
ksz9477_switch_register. It indirectly calls dsa_register_switch, which
creates the relevant struct phy_devices and we update the link modes
right after that. At that time dev->features is already initialized by

Remove phy_setup from ksz_dev_ops as no users remain.

Fixes: 42fc6a4c613019 ("net: dsa: microchip: prepare PHY for proper advertisement")
Signed-off-by: Helmut Grohne <>
 drivers/net/dsa/microchip/ksz9477.c    | 39 +++++++++++++-------------
 drivers/net/dsa/microchip/ksz_common.c |  2 --
 drivers/net/dsa/microchip/ksz_common.h |  2 --
 3 files changed, 20 insertions(+), 23 deletions(-)

On Fri, Jul 17, 2020 at 03:18:14PM +0200, Andrew Lunn wrote:
> I'm not questioning the ordering. I'm questioning which phydev
> structure is being manipulated.
> Is slave_dev->phydev == &dev->ports[i].phydev ?

You are spot on. I mistakenly assumed this to be the case, but it really
is not. Thank you. Your detailed explanations are much appreciated. This
slipped my testing, because I only checked whether the 1Gbit HDX mode
was correctly removed. It seems like something else does that already,
so I didn't notice that I was operating on the wrong phy_device.

The dev->ports[i].phydev is not actually exposed beyond the driver. The
driver sets the phydev.speed in a few places and even reads it back in
one place. It also sets phydev.duplex, but never reads it back. It
queries, which is statically 0 due to using devm_kzalloc.

I think the use of this ksz_port.phydev is very misleading, but I'm
unsure how to fix this. It is not clear to me whether all those updates
should be performed on the connected phydev instead or whether this is
just internal state tracking.

That leaves the question of when to remove the link modes. ksz9477_setup
is called before dsa_register_switch. At the time it runs, the port's
slave device is a NULL pointer. The actual phydev is not linked up and
we cannot access it. The phydev only gets initialized during
dsa_register_switch when dsa_slave_phy_connect is called. Since there is
no suitable hook there, we cannot change the link modes before
phylink_connect_phy is called.

As far as I understand your previous mails, it is not necessary to
remove the link modes before phylink_connect_phy. So the next place
after dsa_register_switch seems to be inside ksz9477_switch_register (as
dsa_register_switch is the final call in ksz_switch_register). I'm
unsure whether this poses a race condition with user space, but on the
system under test, the race is reliably won if there is any.

Beyond resolving the phydev mess, I wish that we could agree on the
order of verbs and nouns in symbols in some way. dsa_register_switch vs.
ksz_switch_register became confusing at a time. I see where either is
coming from, but I think that consistency would be better here.


changes since v2:
 * Operate on the correct phydev. Thanks to Andrew Lunn.
changes since v1:
 * Don't change phy_remove_link_mode. Instead, call it at the right
   time. Thanks to Andrew Lunn for the detailed explanation.

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 8d15c3016024..368964b09aae 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -974,23 +974,6 @@ static void ksz9477_port_mirror_del(struct dsa_switch *ds, int port,
 			     PORT_MIRROR_SNIFFER, false);
-static void ksz9477_phy_setup(struct ksz_device *dev, int port,
-			      struct phy_device *phy)
-	/* Only apply to port with PHY. */
-	if (port >= dev->phy_port_cnt)
-		return;
-	/* The MAC actually cannot run in 1000 half-duplex mode. */
-	phy_remove_link_mode(phy,
-			     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
-	/* PHY does not support gigabit. */
-	if (!(dev->features & GBIT_SUPPORT))
-		phy_remove_link_mode(phy,
-				     ETHTOOL_LINK_MODE_1000baseT_Full_BIT);
 static bool ksz9477_get_gbit(struct ksz_device *dev, u8 data)
 	bool gbit;
@@ -1603,7 +1586,6 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 	.get_port_addr = ksz9477_get_port_addr,
 	.cfg_port_member = ksz9477_cfg_port_member,
 	.flush_dyn_mac_table = ksz9477_flush_dyn_mac_table,
-	.phy_setup = ksz9477_phy_setup,
 	.port_setup = ksz9477_port_setup,
 	.r_mib_cnt = ksz9477_r_mib_cnt,
 	.r_mib_pkt = ksz9477_r_mib_pkt,
@@ -1617,7 +1599,26 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 int ksz9477_switch_register(struct ksz_device *dev)
-	return ksz_switch_register(dev, &ksz9477_dev_ops);
+	int ret, i;
+	struct phy_device *phydev;
+	ret = ksz_switch_register(dev, &ksz9477_dev_ops);
+	if (ret)
+		return ret;
+	for (i = 0; i < dev->phy_port_cnt; ++i) {
+		phydev = dsa_to_port(dev->ds, i)->slave->phydev;
+		/* The MAC actually cannot run in 1000 half-duplex mode. */
+		phy_remove_link_mode(phydev,
+				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
+		/* PHY does not support gigabit. */
+		if (!(dev->features & GBIT_SUPPORT))
+			phy_remove_link_mode(phydev,
+					     ETHTOOL_LINK_MODE_1000baseT_Full_BIT);
+	}
+	return ret;
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index fd1d6676ae4f..7b6c0dce7536 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -358,8 +358,6 @@ int ksz_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
 	/* setup slave port */
 	dev->dev_ops->port_setup(dev, port, false);
-	if (dev->dev_ops->phy_setup)
-		dev->dev_ops->phy_setup(dev, port, phy);
 	/* port_stp_state_set() will be called after to enable the port so
 	 * there is no need to do anything.
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index f2c9bb68fd33..7d11dd32ec0d 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -119,8 +119,6 @@ struct ksz_dev_ops {
 	u32 (*get_port_addr)(int port, int offset);
 	void (*cfg_port_member)(struct ksz_device *dev, int port, u8 member);
 	void (*flush_dyn_mac_table)(struct ksz_device *dev, int port);
-	void (*phy_setup)(struct ksz_device *dev, int port,
-			  struct phy_device *phy);
 	void (*port_cleanup)(struct ksz_device *dev, int port);
 	void (*port_setup)(struct ksz_device *dev, int port, bool cpu_port);
 	void (*r_phy)(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);

  reply	other threads:[~2020-07-20  9:04 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-14  8:25 [PATCH] net: phy: phy_remove_link_mode should not advertise new modes 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
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           ` Helmut Grohne [this message]
2020-07-20 20:43             ` [PATCH v3] " 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:

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

  git send-email \
    --in-reply-to=20200720090416.GA7307@laureti-dev \ \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).