[PATCH v2 net 0/4] net: dsa: mv88e6xxx: Amethyst (6393X) fixes

[PATCH v2 net 0/4] net: dsa: mv88e6xxx: Amethyst (6393X) fixes

[Tobias Waldekranz]

This series provides a set of bug fixes discovered while bringing up a
new board using mv88e6393x chips.

1/4 adds logging of low-level I/O errors that where previously only
logged at a much higher layer, e.g. "probe failed" or "failed to add
VLAN", at which time the origin of the error was long gone. Not
exactly a bugfix, though still suitable for -net IMHO; but I'm also
happy to send it via net-next instead if that makes more sense.

2/4 fixes an issue I've never seen on any other board. At first I
assumed that there was some board-specific issue, but we've not been
able to find one. If you give the chip enough time, it will eventually
signal "PPU Polling" and everything else will work as
expected. Therefore I assume that all is in order, and that we simply
need to increase the timeout.

3/4 just broadens Chris' original fix to apply to all chips. Though I
have obviously not tested this on every supported device, I can't see
how this could possibly be chip specific. Was there some specific
reason for originally limiting the set of chips that this applied to?

4/4 can only be supported on the Amethyst, which can control the
ieee-multicast policy per-port, rather than via a global setting as
it's done on the older families.

v1 -> v2:
 - Increase the global timeout in mv88e6xxx_wait_mask() to cover the
   slow PPU init, rather handling PPU init as a special case (Andrew)
 - (Because of the previous change, Paolo's suggestion on lowering the
   priority of the log message was rendered mute)

Tobias Waldekranz (4):
  net: dsa: mv88e6xxx: Improve I/O related error logging
  net: dsa: mv88e6xxx: Give chips more time to activate their PPUs
  net: dsa: mv88e6xxx: Never force link on in-band managed MACs
  net: dsa: mv88e6xxx: Limit rsvd2cpu policy to user ports on 6393X

 drivers/net/dsa/mv88e6xxx/chip.c | 88 +++++++++++++++++---------------
 drivers/net/dsa/mv88e6xxx/chip.h |  4 --
 drivers/net/dsa/mv88e6xxx/port.c | 48 ++++++++---------
 drivers/net/dsa/mv88e6xxx/port.h |  1 -
 4 files changed, 72 insertions(+), 69 deletions(-)

-- 
2.43.0

[PATCH v2 net 1/4] net: dsa: mv88e6xxx: Improve I/O related error logging

[Tobias Waldekranz]

In the rare event of an I/O error - e.g. a broken bus controller,
frozen chip, etc. - make sure to log all available information, so
that there is some hope of determining _where_ the error; not just
_that_ an error occurred.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 51 +++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 3a792f79270d..46926b769460 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -59,8 +59,11 @@ int mv88e6xxx_read(struct mv88e6xxx_chip *chip, int addr, int reg, u16 *val)
 	assert_reg_lock(chip);
 
 	err = mv88e6xxx_smi_read(chip, addr, reg, val);
-	if (err)
+	if (err) {
+		dev_err_ratelimited(chip->dev, "Failed to read from 0x%02x/0x%02x: %d",
+				    addr, reg, err);
 		return err;
+	}
 
 	dev_dbg(chip->dev, "<- addr: 0x%.2x reg: 0x%.2x val: 0x%.4x\n",
 		addr, reg, *val);
@@ -75,17 +78,19 @@ int mv88e6xxx_write(struct mv88e6xxx_chip *chip, int addr, int reg, u16 val)
 	assert_reg_lock(chip);
 
 	err = mv88e6xxx_smi_write(chip, addr, reg, val);
-	if (err)
+	if (err) {
+		dev_err_ratelimited(chip->dev, "Failed to write 0x%04x to 0x%02x/0x%02x: %d",
+				    val, addr, reg, err);
 		return err;
-
+	}
 	dev_dbg(chip->dev, "-> addr: 0x%.2x reg: 0x%.2x val: 0x%.4x\n",
 		addr, reg, val);
 
 	return 0;
 }
 
-int mv88e6xxx_wait_mask(struct mv88e6xxx_chip *chip, int addr, int reg,
-			u16 mask, u16 val)
+static int _mv88e6xxx_wait_mask(struct mv88e6xxx_chip *chip, int addr, int reg,
+				u16 mask, u16 val, u16 *last)
 {
 	const unsigned long timeout = jiffies + msecs_to_jiffies(50);
 	u16 data;
@@ -117,15 +122,45 @@ int mv88e6xxx_wait_mask(struct mv88e6xxx_chip *chip, int addr, int reg,
 	if ((data & mask) == val)
 		return 0;
 
-	dev_err(chip->dev, "Timeout while waiting for switch\n");
+	if (last)
+		*last = data;
+
 	return -ETIMEDOUT;
 }
 
+int mv88e6xxx_wait_mask(struct mv88e6xxx_chip *chip, int addr, int reg,
+			u16 mask, u16 val)
+{
+	u16 last;
+	int err;
+
+	err = _mv88e6xxx_wait_mask(chip, addr, reg, mask, val, &last);
+	if (!err)
+		return 0;
+
+	dev_err(chip->dev,
+		"%s waiting for 0x%02x/0x%02x to match 0x%04x (mask:0x%04x last:0x%04x)\n",
+		(err == -ETIMEDOUT) ? "Timed out" : "Failed",
+		addr, reg, val, mask, last);
+	return err;
+}
+
 int mv88e6xxx_wait_bit(struct mv88e6xxx_chip *chip, int addr, int reg,
 		       int bit, int val)
 {
-	return mv88e6xxx_wait_mask(chip, addr, reg, BIT(bit),
-				   val ? BIT(bit) : 0x0000);
+	u16 last;
+	int err;
+
+	err = _mv88e6xxx_wait_mask(chip, addr, reg, BIT(bit),
+				   val ? BIT(bit) : 0x0000, &last);
+	if (!err)
+		return 0;
+
+	dev_err(chip->dev,
+		"%s waiting for bit %d in 0x%02x/0x%02x to %s (last:0x%04x)\n",
+		(err == -ETIMEDOUT) ? "Timed out" : "Failed",
+		bit, addr, reg, val ? "set" : "clear", last);
+	return err;
 }
 
 struct mii_bus *mv88e6xxx_default_mdio_bus(struct mv88e6xxx_chip *chip)
-- 
2.43.0

Re: [PATCH v2 net 1/4] net: dsa: mv88e6xxx: Improve I/O related error logging

[Andrew Lunn]

On Thu, Dec 19, 2024 at 01:30:40PM +0100, Tobias Waldekranz wrote:
> In the rare event of an I/O error - e.g. a broken bus controller,
> frozen chip, etc. - make sure to log all available information, so
> that there is some hope of determining _where_ the error; not just
> _that_ an error occurred.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 net 1/4] net: dsa: mv88e6xxx: Improve I/O related error logging

[Vladimir Oltean]

On Thu, Dec 19, 2024 at 01:30:40PM +0100, Tobias Waldekranz wrote:
> +int mv88e6xxx_wait_mask(struct mv88e6xxx_chip *chip, int addr, int reg,
> +			u16 mask, u16 val)
> +{
> +	u16 last;
> +	int err;
> +
> +	err = _mv88e6xxx_wait_mask(chip, addr, reg, mask, val, &last);
> +	if (!err)
> +		return 0;
> +
> +	dev_err(chip->dev,
> +		"%s waiting for 0x%02x/0x%02x to match 0x%04x (mask:0x%04x last:0x%04x)\n",
> +		(err == -ETIMEDOUT) ? "Timed out" : "Failed",
> +		addr, reg, val, mask, last);

Would it help to use symbolic error names ("%pe", ERR_PTR(err))? You
wouldn't have to single out "Timed out", since it would print -ETIMEDOUT
and that should be pretty obvious already.

> +	return err;
> +}

[PATCH v2 net 2/4] net: dsa: mv88e6xxx: Give chips more time to activate their PPUs

[Tobias Waldekranz]

In a daisy-chain of three 6393X devices, delays of up to 750ms are
sometimes observed before completion of PPU initialization (Global 1,
register 0, bit 15) is signaled. Therefore, allow chips more time
before giving up.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 46926b769460..c7683ea334a7 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -92,7 +92,7 @@ int mv88e6xxx_write(struct mv88e6xxx_chip *chip, int addr, int reg, u16 val)
 static int _mv88e6xxx_wait_mask(struct mv88e6xxx_chip *chip, int addr, int reg,
 				u16 mask, u16 val, u16 *last)
 {
-	const unsigned long timeout = jiffies + msecs_to_jiffies(50);
+	const unsigned long timeout = jiffies + msecs_to_jiffies(2000);
 	u16 data;
 	int err;
 	int i;
-- 
2.43.0

Re: [PATCH v2 net 2/4] net: dsa: mv88e6xxx: Give chips more time to activate their PPUs

[Andrew Lunn]

On Thu, Dec 19, 2024 at 01:30:41PM +0100, Tobias Waldekranz wrote:
> In a daisy-chain of three 6393X devices, delays of up to 750ms are
> sometimes observed before completion of PPU initialization (Global 1,
> register 0, bit 15) is signaled. Therefore, allow chips more time
> before giving up.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v2 net 3/4] net: dsa: mv88e6xxx: Never force link on in-band managed MACs

[Tobias Waldekranz]

NOTE: This issue was addressed in the referenced commit, but a
conservative approach was chosen, where only 6095, 6097 and 6185 got
the fix.

Before the referenced commit, in the following setup, when the PHY
detected loss of link on the MDI, mv88e6xxx would force the MAC
down. If the MDI-side link was then re-established later on, there was
no longer any MII link over which the PHY could communicate that
information back to the MAC.

        .-SGMII/USXGMII
        |
.-----. v .-----.   .--------------.
| MAC +---+ PHY +---+ MDI (Cu/SFP) |
'-----'   '-----'   '--------------'

Since this a generic problem on all MACs connected to a SERDES - which
is the only time when in-band-status is used - move all chips to a
common mv88e6xxx_port_sync_link() implementation which avoids forcing
links on _all_ in-band managed ports.

Fixes: 4efe76629036 ("net: dsa: mv88e6xxx: Don't force link when using in-band-status")
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 35 +++-----------------------------
 drivers/net/dsa/mv88e6xxx/chip.h |  4 ----
 drivers/net/dsa/mv88e6xxx/port.c | 17 ----------------
 drivers/net/dsa/mv88e6xxx/port.h |  1 -
 4 files changed, 3 insertions(+), 54 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c7683ea334a7..707dfe5c1ed8 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1007,8 +1007,8 @@ static void mv88e6xxx_mac_link_down(struct phylink_config *config,
 	 * updated by the switch or if we are using fixed-link mode.
 	 */
 	if ((!mv88e6xxx_port_ppu_updates(chip, port) ||
-	     mode == MLO_AN_FIXED) && ops->port_sync_link)
-		err = ops->port_sync_link(chip, port, mode, false);
+	     mode == MLO_AN_FIXED))
+		err = mv88e6xxx_port_sync_link(chip, port, mode, false);
 
 	if (!err && ops->port_set_speed_duplex)
 		err = ops->port_set_speed_duplex(chip, port, SPEED_UNFORCED,
@@ -1048,8 +1048,7 @@ static void mv88e6xxx_mac_link_up(struct phylink_config *config,
 				goto error;
 		}
 
-		if (ops->port_sync_link)
-			err = ops->port_sync_link(chip, port, mode, true);
+		err = mv88e6xxx_port_sync_link(chip, port, mode, true);
 	}
 error:
 	mv88e6xxx_reg_unlock(chip);
@@ -4213,7 +4212,6 @@ static const struct mv88e6xxx_ops mv88e6085_ops = {
 	.phy_read = mv88e6185_phy_ppu_read,
 	.phy_write = mv88e6185_phy_ppu_write,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
 	.port_set_policy = mv88e6352_port_set_policy,
@@ -4257,7 +4255,6 @@ static const struct mv88e6xxx_ops mv88e6095_ops = {
 	.phy_read = mv88e6185_phy_ppu_read,
 	.phy_write = mv88e6185_phy_ppu_write,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6185_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_set_frame_mode = mv88e6085_port_set_frame_mode,
 	.port_set_ucast_flood = mv88e6185_port_set_forward_unknown,
@@ -4292,7 +4289,6 @@ static const struct mv88e6xxx_ops mv88e6097_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6185_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
 	.port_set_policy = mv88e6352_port_set_policy,
@@ -4339,7 +4335,6 @@ static const struct mv88e6xxx_ops mv88e6123_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_set_frame_mode = mv88e6085_port_set_frame_mode,
 	.port_set_ucast_flood = mv88e6352_port_set_ucast_flood,
@@ -4377,7 +4372,6 @@ static const struct mv88e6xxx_ops mv88e6131_ops = {
 	.phy_read = mv88e6185_phy_ppu_read,
 	.phy_write = mv88e6185_phy_ppu_write,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
 	.port_set_frame_mode = mv88e6351_port_set_frame_mode,
@@ -4422,7 +4416,6 @@ static const struct mv88e6xxx_ops mv88e6141_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6341_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6341_port_max_speed_mode,
@@ -4483,7 +4476,6 @@ static const struct mv88e6xxx_ops mv88e6161_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
 	.port_set_policy = mv88e6352_port_set_policy,
@@ -4529,7 +4521,6 @@ static const struct mv88e6xxx_ops mv88e6165_ops = {
 	.phy_read = mv88e6165_phy_read,
 	.phy_write = mv88e6165_phy_write,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_disable_learn_limit = mv88e6xxx_port_disable_learn_limit,
 	.port_disable_pri_override = mv88e6xxx_port_disable_pri_override,
@@ -4568,7 +4559,6 @@ static const struct mv88e6xxx_ops mv88e6171_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -4616,7 +4606,6 @@ static const struct mv88e6xxx_ops mv88e6172_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6352_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -4671,7 +4660,6 @@ static const struct mv88e6xxx_ops mv88e6175_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -4719,7 +4707,6 @@ static const struct mv88e6xxx_ops mv88e6176_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6352_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -4773,7 +4760,6 @@ static const struct mv88e6xxx_ops mv88e6185_ops = {
 	.phy_read = mv88e6185_phy_ppu_read,
 	.phy_write = mv88e6185_phy_ppu_write,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6185_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_set_frame_mode = mv88e6085_port_set_frame_mode,
 	.port_set_ucast_flood = mv88e6185_port_set_forward_unknown,
@@ -4815,7 +4801,6 @@ static const struct mv88e6xxx_ops mv88e6190_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6390_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6390_port_max_speed_mode,
@@ -4875,7 +4860,6 @@ static const struct mv88e6xxx_ops mv88e6190x_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6390x_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6390x_port_max_speed_mode,
@@ -4935,7 +4919,6 @@ static const struct mv88e6xxx_ops mv88e6191_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6390_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6390_port_max_speed_mode,
@@ -4995,7 +4978,6 @@ static const struct mv88e6xxx_ops mv88e6240_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6352_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -5056,7 +5038,6 @@ static const struct mv88e6xxx_ops mv88e6250_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6250_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -5100,7 +5081,6 @@ static const struct mv88e6xxx_ops mv88e6290_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6390_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6390_port_max_speed_mode,
@@ -5162,7 +5142,6 @@ static const struct mv88e6xxx_ops mv88e6320_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6320_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -5211,7 +5190,6 @@ static const struct mv88e6xxx_ops mv88e6321_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6320_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -5259,7 +5237,6 @@ static const struct mv88e6xxx_ops mv88e6341_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6341_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6341_port_max_speed_mode,
@@ -5322,7 +5299,6 @@ static const struct mv88e6xxx_ops mv88e6350_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -5368,7 +5344,6 @@ static const struct mv88e6xxx_ops mv88e6351_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -5418,7 +5393,6 @@ static const struct mv88e6xxx_ops mv88e6352_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6352_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -5481,7 +5455,6 @@ static const struct mv88e6xxx_ops mv88e6390_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6390_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6390_port_max_speed_mode,
@@ -5545,7 +5518,6 @@ static const struct mv88e6xxx_ops mv88e6390x_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6390x_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6390x_port_max_speed_mode,
@@ -5608,7 +5580,6 @@ static const struct mv88e6xxx_ops mv88e6393x_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6393x_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6393x_port_max_speed_mode,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 9fe8e8a7856b..27d19d55f9e5 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -525,10 +525,6 @@ struct mv88e6xxx_ops {
 	 */
 	int (*port_set_link)(struct mv88e6xxx_chip *chip, int port, int link);
 
-	/* Synchronise the port link state with that of the SERDES
-	 */
-	int (*port_sync_link)(struct mv88e6xxx_chip *chip, int port, unsigned int mode, bool isup);
-
 #define PAUSE_ON		1
 #define PAUSE_OFF		0
 
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index dc777ddce1f3..56ed2f57fef8 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -187,23 +187,6 @@ int mv88e6xxx_port_sync_link(struct mv88e6xxx_chip *chip, int port, unsigned int
 	int err = 0;
 	int link;
 
-	if (isup)
-		link = LINK_FORCED_UP;
-	else
-		link = LINK_FORCED_DOWN;
-
-	if (ops->port_set_link)
-		err = ops->port_set_link(chip, port, link);
-
-	return err;
-}
-
-int mv88e6185_port_sync_link(struct mv88e6xxx_chip *chip, int port, unsigned int mode, bool isup)
-{
-	const struct mv88e6xxx_ops *ops = chip->info->ops;
-	int err = 0;
-	int link;
-
 	if (mode == MLO_AN_INBAND)
 		link = LINK_UNFORCED;
 	else if (isup)
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index c1d2f99efb1c..26452e0a8448 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -484,7 +484,6 @@ int mv88e6390_port_set_rgmii_delay(struct mv88e6xxx_chip *chip, int port,
 int mv88e6xxx_port_set_link(struct mv88e6xxx_chip *chip, int port, int link);
 
 int mv88e6xxx_port_sync_link(struct mv88e6xxx_chip *chip, int port, unsigned int mode, bool isup);
-int mv88e6185_port_sync_link(struct mv88e6xxx_chip *chip, int port, unsigned int mode, bool isup);
 
 int mv88e6185_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port,
 				    int speed, int duplex);
-- 
2.43.0

Re: [PATCH v2 net 3/4] net: dsa: mv88e6xxx: Never force link on in-band managed MACs

[Andrew Lunn]

On Thu, Dec 19, 2024 at 01:30:42PM +0100, Tobias Waldekranz wrote:
> NOTE: This issue was addressed in the referenced commit, but a
> conservative approach was chosen, where only 6095, 6097 and 6185 got
> the fix.
> 
> Before the referenced commit, in the following setup, when the PHY
> detected loss of link on the MDI, mv88e6xxx would force the MAC
> down. If the MDI-side link was then re-established later on, there was
> no longer any MII link over which the PHY could communicate that
> information back to the MAC.
> 
>         .-SGMII/USXGMII
>         |
> .-----. v .-----.   .--------------.
> | MAC +---+ PHY +---+ MDI (Cu/SFP) |
> '-----'   '-----'   '--------------'
> 
> Since this a generic problem on all MACs connected to a SERDES - which
> is the only time when in-band-status is used - move all chips to a
> common mv88e6xxx_port_sync_link() implementation which avoids forcing
> links on _all_ in-band managed ports.
> 
> Fixes: 4efe76629036 ("net: dsa: mv88e6xxx: Don't force link when using in-band-status")
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 net 3/4] net: dsa: mv88e6xxx: Never force link on in-band managed MACs

[Russell King (Oracle)]

On Thu, Dec 19, 2024 at 01:30:42PM +0100, Tobias Waldekranz wrote:
> NOTE: This issue was addressed in the referenced commit, but a
> conservative approach was chosen, where only 6095, 6097 and 6185 got
> the fix.
> 
> Before the referenced commit, in the following setup, when the PHY
> detected loss of link on the MDI, mv88e6xxx would force the MAC
> down. If the MDI-side link was then re-established later on, there was
> no longer any MII link over which the PHY could communicate that
> information back to the MAC.
> 
>         .-SGMII/USXGMII
>         |
> .-----. v .-----.   .--------------.
> | MAC +---+ PHY +---+ MDI (Cu/SFP) |
> '-----'   '-----'   '--------------'
> 
> Since this a generic problem on all MACs connected to a SERDES - which
> is the only time when in-band-status is used - move all chips to a
> common mv88e6xxx_port_sync_link() implementation which avoids forcing
> links on _all_ in-band managed ports.
> 
> Fixes: 4efe76629036 ("net: dsa: mv88e6xxx: Don't force link when using in-band-status")
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

I'm feeling uneasy about this change.

The history of the patch you refer to is - original v1:

https://lore.kernel.org/r/20201013021858.20530-2-chris.packham@alliedtelesis.co.nz

When v3 was submitted, it was unchanged:

https://lore.kernel.org/r/20201020034558.19438-2-chris.packham@alliedtelesis.co.nz

Both of these applied the in-band-status thing to all Marvell DSA
switches, but as Marek states here:

https://lore.kernel.org/r/20201020165115.3ecfd601@nic.cz

doing so breaks last least one Marvell DSA switch (88E6390). Hence why
this approach is taken, rather than not forcing the link status on all
DSA switches.

Your patch appears to be reverting us back to what was effectively in
Chris' v1 patch from back then, so I don't think we can accept this
change. Sorry.

If you have a switch that needs your change, then I think you also need
to adopt the "conservative" approach and only fix the switch you're
working with - short of someone going through all the Marvell DSA
switch datasheets or testing on real hardware, I don't think it's a
good idea to change the behaviour of all Marvell switches like this.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 net 3/4] net: dsa: mv88e6xxx: Never force link on in-band managed MACs

[Tobias Waldekranz]

On tor, jan 02, 2025 at 10:31, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> On Thu, Dec 19, 2024 at 01:30:42PM +0100, Tobias Waldekranz wrote:
>> NOTE: This issue was addressed in the referenced commit, but a
>> conservative approach was chosen, where only 6095, 6097 and 6185 got
>> the fix.
>> 
>> Before the referenced commit, in the following setup, when the PHY
>> detected loss of link on the MDI, mv88e6xxx would force the MAC
>> down. If the MDI-side link was then re-established later on, there was
>> no longer any MII link over which the PHY could communicate that
>> information back to the MAC.
>> 
>>         .-SGMII/USXGMII
>>         |
>> .-----. v .-----.   .--------------.
>> | MAC +---+ PHY +---+ MDI (Cu/SFP) |
>> '-----'   '-----'   '--------------'
>> 
>> Since this a generic problem on all MACs connected to a SERDES - which
>> is the only time when in-band-status is used - move all chips to a
>> common mv88e6xxx_port_sync_link() implementation which avoids forcing
>> links on _all_ in-band managed ports.
>> 
>> Fixes: 4efe76629036 ("net: dsa: mv88e6xxx: Don't force link when using in-band-status")
>> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
>
> I'm feeling uneasy about this change.
>
> The history of the patch you refer to is - original v1:
>
> https://lore.kernel.org/r/20201013021858.20530-2-chris.packham@alliedtelesis.co.nz
>
> When v3 was submitted, it was unchanged:
>
> https://lore.kernel.org/r/20201020034558.19438-2-chris.packham@alliedtelesis.co.nz
>
> Both of these applied the in-band-status thing to all Marvell DSA
> switches, but as Marek states here:
>
> https://lore.kernel.org/r/20201020165115.3ecfd601@nic.cz

Thanks for that context!

> doing so breaks last least one Marvell DSA switch (88E6390). Hence why
> this approach is taken, rather than not forcing the link status on all
> DSA switches.
>
> Your patch appears to be reverting us back to what was effectively in
> Chris' v1 patch from back then, so I don't think we can accept this
> change. Sorry.

Before I abandon this broader fix, maybe you can help me understand
something:

If a user explicitly selects `managed = "in-band-status"`, why would we
ever interpret that as "let's force the MAC's settings according to what
the PHY says"? Is that not what `managed = "auto"` is for?

Could Marek's issue not stem from the fact that his DT stated that the
MAC would pick up the link information in-band, when in fact it was
unable to do so?

Looking at the functional spec for the 6390, it only seems to support
in-band-status when SGMII is used. So would it not be more accurate to
require `managed = "auto"` on boards with 6390 SERDES lanes connected to
an SFP cage, if hot-swap support between SGMII/1000BASE-X/2500BASE-X
transceivers is needed?

I.e. users requiring maximum flexibility can still have it, while
someone with a fixed connection to an SGMII PHY can still get the
benefit of using in-band signaling.

Re: [PATCH v2 net 3/4] net: dsa: mv88e6xxx: Never force link on in-band managed MACs

[Russell King (Oracle)]

On Thu, Jan 02, 2025 at 02:06:32PM +0100, Tobias Waldekranz wrote:
> On tor, jan 02, 2025 at 10:31, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > On Thu, Dec 19, 2024 at 01:30:42PM +0100, Tobias Waldekranz wrote:
> >> NOTE: This issue was addressed in the referenced commit, but a
> >> conservative approach was chosen, where only 6095, 6097 and 6185 got
> >> the fix.
> >> 
> >> Before the referenced commit, in the following setup, when the PHY
> >> detected loss of link on the MDI, mv88e6xxx would force the MAC
> >> down. If the MDI-side link was then re-established later on, there was
> >> no longer any MII link over which the PHY could communicate that
> >> information back to the MAC.
> >> 
> >>         .-SGMII/USXGMII
> >>         |
> >> .-----. v .-----.   .--------------.
> >> | MAC +---+ PHY +---+ MDI (Cu/SFP) |
> >> '-----'   '-----'   '--------------'
> >> 
> >> Since this a generic problem on all MACs connected to a SERDES - which
> >> is the only time when in-band-status is used - move all chips to a
> >> common mv88e6xxx_port_sync_link() implementation which avoids forcing
> >> links on _all_ in-band managed ports.
> >> 
> >> Fixes: 4efe76629036 ("net: dsa: mv88e6xxx: Don't force link when using in-band-status")
> >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> >
> > I'm feeling uneasy about this change.
> >
> > The history of the patch you refer to is - original v1:
> >
> > https://lore.kernel.org/r/20201013021858.20530-2-chris.packham@alliedtelesis.co.nz
> >
> > When v3 was submitted, it was unchanged:
> >
> > https://lore.kernel.org/r/20201020034558.19438-2-chris.packham@alliedtelesis.co.nz
> >
> > Both of these applied the in-band-status thing to all Marvell DSA
> > switches, but as Marek states here:
> >
> > https://lore.kernel.org/r/20201020165115.3ecfd601@nic.cz
> 
> Thanks for that context!
> 
> > doing so breaks last least one Marvell DSA switch (88E6390). Hence why
> > this approach is taken, rather than not forcing the link status on all
> > DSA switches.
> >
> > Your patch appears to be reverting us back to what was effectively in
> > Chris' v1 patch from back then, so I don't think we can accept this
> > change. Sorry.
> 
> Before I abandon this broader fix, maybe you can help me understand
> something:
> 
> If a user explicitly selects `managed = "in-band-status"`, why would we
> ever interpret that as "let's force the MAC's settings according to what
> the PHY says"? Is that not what `managed = "auto"` is for?

You seem confused with that point, somehow confusing the calls to
mac_link_up()/mac_link_down() when using in-band-status with something
that a PHY would indicate. No, that's just wrong.

If using in-band-status, these calls will be made in response to what
the PCS says the link state is, possibly in conjunction with a PHY if
there is a PHY present. Whether the PCS state gets forwarded to the MAC
is hardware specific, and we have at least one DSA switch where this
doesn't appear happen.

Please realise that there are _three_ distinct modules here:

- The MAC
- The PCS
- The PHY or media

and the managed property is about whether in-band signalling is used
from the PCS towards the media, not from the PCS towards the MAC.

So, if the MAC doesn't get updated with the PCS' link state, then
mac_link_up()/mac_link_down() need to do that manually, even if the
link from the PCS towards the media is using in-band signalling.

I think you're confusing in-band-status as meaning that the MAC
gets automatically updated with the PCS media-side link state -
the DT property has no bearing on that.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 net 3/4] net: dsa: mv88e6xxx: Never force link on in-band managed MACs

[Tobias Waldekranz]

On tor, jan 02, 2025 at 17:08, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> On Thu, Jan 02, 2025 at 02:06:32PM +0100, Tobias Waldekranz wrote:
>> On tor, jan 02, 2025 at 10:31, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
>> > On Thu, Dec 19, 2024 at 01:30:42PM +0100, Tobias Waldekranz wrote:
>> >> NOTE: This issue was addressed in the referenced commit, but a
>> >> conservative approach was chosen, where only 6095, 6097 and 6185 got
>> >> the fix.
>> >> 
>> >> Before the referenced commit, in the following setup, when the PHY
>> >> detected loss of link on the MDI, mv88e6xxx would force the MAC
>> >> down. If the MDI-side link was then re-established later on, there was
>> >> no longer any MII link over which the PHY could communicate that
>> >> information back to the MAC.
>> >> 
>> >>         .-SGMII/USXGMII
>> >>         |
>> >> .-----. v .-----.   .--------------.
>> >> | MAC +---+ PHY +---+ MDI (Cu/SFP) |
>> >> '-----'   '-----'   '--------------'
>> >> 
>> >> Since this a generic problem on all MACs connected to a SERDES - which
>> >> is the only time when in-band-status is used - move all chips to a
>> >> common mv88e6xxx_port_sync_link() implementation which avoids forcing
>> >> links on _all_ in-band managed ports.
>> >> 
>> >> Fixes: 4efe76629036 ("net: dsa: mv88e6xxx: Don't force link when using in-band-status")
>> >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
>> >
>> > I'm feeling uneasy about this change.
>> >
>> > The history of the patch you refer to is - original v1:
>> >
>> > https://lore.kernel.org/r/20201013021858.20530-2-chris.packham@alliedtelesis.co.nz
>> >
>> > When v3 was submitted, it was unchanged:
>> >
>> > https://lore.kernel.org/r/20201020034558.19438-2-chris.packham@alliedtelesis.co.nz
>> >
>> > Both of these applied the in-band-status thing to all Marvell DSA
>> > switches, but as Marek states here:
>> >
>> > https://lore.kernel.org/r/20201020165115.3ecfd601@nic.cz
>> 
>> Thanks for that context!
>> 
>> > doing so breaks last least one Marvell DSA switch (88E6390). Hence why
>> > this approach is taken, rather than not forcing the link status on all
>> > DSA switches.
>> >
>> > Your patch appears to be reverting us back to what was effectively in
>> > Chris' v1 patch from back then, so I don't think we can accept this
>> > change. Sorry.
>> 
>> Before I abandon this broader fix, maybe you can help me understand
>> something:
>> 
>> If a user explicitly selects `managed = "in-band-status"`, why would we
>> ever interpret that as "let's force the MAC's settings according to what
>> the PHY says"? Is that not what `managed = "auto"` is for?
>
> You seem confused with that point, somehow confusing the calls to
> mac_link_up()/mac_link_down() when using in-band-status with something
> that a PHY would indicate. No, that's just wrong.
>
> If using in-band-status, these calls will be made in response to what
> the PCS says the link state is, possibly in conjunction with a PHY if
> there is a PHY present. Whether the PCS state gets forwarded to the MAC
> is hardware specific, and we have at least one DSA switch where this
> doesn't appear happen.
>
> Please realise that there are _three_ distinct modules here:
>
> - The MAC
> - The PCS
> - The PHY or media

Right, I sloppily used "PHY" to refer to the link partner on the other
end of the SERDES.  I realize that the remote PCS does not have to
reside within a PHY.

> and the managed property is about whether in-band signalling is used
> from the PCS towards the media, not from the PCS towards the MAC.
>
> So, if the MAC doesn't get updated with the PCS' link state, then
> mac_link_up()/mac_link_down() need to do that manually, even if the
> link from the PCS towards the media is using in-band signalling.
>
> I think you're confusing in-band-status as meaning that the MAC
> gets automatically updated with the PCS media-side link state -
> the DT property has no bearing on that.

If `managed` does not declare a hardware capability of the controller,
then what information does it convey that is not already present in the
`phy-connection-type`?

E.g. what does it mean to have an SGMII link where in-band signaling is
not used?  Is that not part of what defines SGMII?

I.e. could you provide an example `$TYPE`, where both of the following
configs are valid, and what the difference between the two would be?

    &eth0 {
        phy-connection-type = "$TYPE";
        managed = "auto";
    };

    &eth0 {
        phy-connection-type = "$TYPE";
        managed = "in-band-status";
    };

> Thanks.

Thank you for taking the time to explain!

Re: [PATCH v2 net 3/4] net: dsa: mv88e6xxx: Never force link on in-band managed MACs

[Russell King (Oracle)]

On Sat, Jan 04, 2025 at 10:37:00PM +0100, Tobias Waldekranz wrote:
> On tor, jan 02, 2025 at 17:08, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > On Thu, Jan 02, 2025 at 02:06:32PM +0100, Tobias Waldekranz wrote:
> >> On tor, jan 02, 2025 at 10:31, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> >> > On Thu, Dec 19, 2024 at 01:30:42PM +0100, Tobias Waldekranz wrote:
> >> >> NOTE: This issue was addressed in the referenced commit, but a
> >> >> conservative approach was chosen, where only 6095, 6097 and 6185 got
> >> >> the fix.
> >> >> 
> >> >> Before the referenced commit, in the following setup, when the PHY
> >> >> detected loss of link on the MDI, mv88e6xxx would force the MAC
> >> >> down. If the MDI-side link was then re-established later on, there was
> >> >> no longer any MII link over which the PHY could communicate that
> >> >> information back to the MAC.
> >> >> 
> >> >>         .-SGMII/USXGMII
> >> >>         |
> >> >> .-----. v .-----.   .--------------.
> >> >> | MAC +---+ PHY +---+ MDI (Cu/SFP) |
> >> >> '-----'   '-----'   '--------------'
> >> >> 
> >> >> Since this a generic problem on all MACs connected to a SERDES - which
> >> >> is the only time when in-band-status is used - move all chips to a
> >> >> common mv88e6xxx_port_sync_link() implementation which avoids forcing
> >> >> links on _all_ in-band managed ports.
> >> >> 
> >> >> Fixes: 4efe76629036 ("net: dsa: mv88e6xxx: Don't force link when using in-band-status")
> >> >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> >> >
> >> > I'm feeling uneasy about this change.
> >> >
> >> > The history of the patch you refer to is - original v1:
> >> >
> >> > https://lore.kernel.org/r/20201013021858.20530-2-chris.packham@alliedtelesis.co.nz
> >> >
> >> > When v3 was submitted, it was unchanged:
> >> >
> >> > https://lore.kernel.org/r/20201020034558.19438-2-chris.packham@alliedtelesis.co.nz
> >> >
> >> > Both of these applied the in-band-status thing to all Marvell DSA
> >> > switches, but as Marek states here:
> >> >
> >> > https://lore.kernel.org/r/20201020165115.3ecfd601@nic.cz
> >> 
> >> Thanks for that context!
> >> 
> >> > doing so breaks last least one Marvell DSA switch (88E6390). Hence why
> >> > this approach is taken, rather than not forcing the link status on all
> >> > DSA switches.
> >> >
> >> > Your patch appears to be reverting us back to what was effectively in
> >> > Chris' v1 patch from back then, so I don't think we can accept this
> >> > change. Sorry.
> >> 
> >> Before I abandon this broader fix, maybe you can help me understand
> >> something:
> >> 
> >> If a user explicitly selects `managed = "in-band-status"`, why would we
> >> ever interpret that as "let's force the MAC's settings according to what
> >> the PHY says"? Is that not what `managed = "auto"` is for?
> >
> > You seem confused with that point, somehow confusing the calls to
> > mac_link_up()/mac_link_down() when using in-band-status with something
> > that a PHY would indicate. No, that's just wrong.
> >
> > If using in-band-status, these calls will be made in response to what
> > the PCS says the link state is, possibly in conjunction with a PHY if
> > there is a PHY present. Whether the PCS state gets forwarded to the MAC
> > is hardware specific, and we have at least one DSA switch where this
> > doesn't appear happen.
> >
> > Please realise that there are _three_ distinct modules here:
> >
> > - The MAC
> > - The PCS
> > - The PHY or media
> 
> Right, I sloppily used "PHY" to refer to the link partner on the other
> end of the SERDES.  I realize that the remote PCS does not have to
> reside within a PHY.

Sigh, it seems I'm not making myself clear.

Host system:

  ---------------------------+
    NIC (or DSA switch port) |
     +-------+    +-------+  |
     |       |    |       |  |
     |  MAC  <---->  PCS  <-----------------------> PHY, SFP or media
     |       |    |       |  |     ^
     +-------+    +-------+  |     |
                             |   phy interface type
  ---------------------------+   also in-band signalling
                                 which managed = "in-band-status"
				 applies to

> E.g. what does it mean to have an SGMII link where in-band signaling is
> not used?  Is that not part of what defines SGMII?

There _are_ PHYs out there that implement Cisco SGMII (which is IEEE
802.3 1000BASE-X modified to allow signalling at 10M and 100M speeds by
symbol replication, and changing the format of the 1000BASE-X to provide
the details of the SGMII link speed and duplex) but do _not_ support
that in-band signalling.

The point of SGMII without in-band signalling rather than just using
1000BASE-X without in-band signalling is that SGMII can operate at
10M and 100M, whereas 1000BASE-X can not.

The usual situation, however, is that most devices that support Cisco
SGMII also allow the in-band signalling to be configured to be used or
not used.


Going back to the diagram above, the link between the MAC and PCS is
_not_ described in DT currently, not by the managed property not by
the phy-modes etc properties.

Now, the port configuration register on the Marvell switches controls
the MAC settings. The PCS has a separate register set (normally
referred to as serdes in Marvell's Switch terminology) which is an
IEEE compliant clause 22 register layout.

The problem is, it seems *some* Marvell switches automatically forward
the PCS status to the MAC. Other switches do not. The DT "managed"
property does not describe this - because - as stated above - the
"managed" property applies to the link between the PCS and external
world (which may be a PHY, or may be media) and _not_ between the
MAC and its associated PCS.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 net 3/4] net: dsa: mv88e6xxx: Never force link on in-band managed MACs

[Tobias Waldekranz]

On lör, jan 04, 2025 at 22:09, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> On Sat, Jan 04, 2025 at 10:37:00PM +0100, Tobias Waldekranz wrote:
>> On tor, jan 02, 2025 at 17:08, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
>> > On Thu, Jan 02, 2025 at 02:06:32PM +0100, Tobias Waldekranz wrote:
>> >> On tor, jan 02, 2025 at 10:31, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
>> >> > On Thu, Dec 19, 2024 at 01:30:42PM +0100, Tobias Waldekranz wrote:
>> >> >> NOTE: This issue was addressed in the referenced commit, but a
>> >> >> conservative approach was chosen, where only 6095, 6097 and 6185 got
>> >> >> the fix.
>> >> >> 
>> >> >> Before the referenced commit, in the following setup, when the PHY
>> >> >> detected loss of link on the MDI, mv88e6xxx would force the MAC
>> >> >> down. If the MDI-side link was then re-established later on, there was
>> >> >> no longer any MII link over which the PHY could communicate that
>> >> >> information back to the MAC.
>> >> >> 
>> >> >>         .-SGMII/USXGMII
>> >> >>         |
>> >> >> .-----. v .-----.   .--------------.
>> >> >> | MAC +---+ PHY +---+ MDI (Cu/SFP) |
>> >> >> '-----'   '-----'   '--------------'
>> >> >> 
>> >> >> Since this a generic problem on all MACs connected to a SERDES - which
>> >> >> is the only time when in-band-status is used - move all chips to a
>> >> >> common mv88e6xxx_port_sync_link() implementation which avoids forcing
>> >> >> links on _all_ in-band managed ports.
>> >> >> 
>> >> >> Fixes: 4efe76629036 ("net: dsa: mv88e6xxx: Don't force link when using in-band-status")
>> >> >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
>> >> >
>> >> > I'm feeling uneasy about this change.
>> >> >
>> >> > The history of the patch you refer to is - original v1:
>> >> >
>> >> > https://lore.kernel.org/r/20201013021858.20530-2-chris.packham@alliedtelesis.co.nz
>> >> >
>> >> > When v3 was submitted, it was unchanged:
>> >> >
>> >> > https://lore.kernel.org/r/20201020034558.19438-2-chris.packham@alliedtelesis.co.nz
>> >> >
>> >> > Both of these applied the in-band-status thing to all Marvell DSA
>> >> > switches, but as Marek states here:
>> >> >
>> >> > https://lore.kernel.org/r/20201020165115.3ecfd601@nic.cz
>> >> 
>> >> Thanks for that context!
>> >> 
>> >> > doing so breaks last least one Marvell DSA switch (88E6390). Hence why
>> >> > this approach is taken, rather than not forcing the link status on all
>> >> > DSA switches.
>> >> >
>> >> > Your patch appears to be reverting us back to what was effectively in
>> >> > Chris' v1 patch from back then, so I don't think we can accept this
>> >> > change. Sorry.
>> >> 
>> >> Before I abandon this broader fix, maybe you can help me understand
>> >> something:
>> >> 
>> >> If a user explicitly selects `managed = "in-band-status"`, why would we
>> >> ever interpret that as "let's force the MAC's settings according to what
>> >> the PHY says"? Is that not what `managed = "auto"` is for?
>> >
>> > You seem confused with that point, somehow confusing the calls to
>> > mac_link_up()/mac_link_down() when using in-band-status with something
>> > that a PHY would indicate. No, that's just wrong.
>> >
>> > If using in-band-status, these calls will be made in response to what
>> > the PCS says the link state is, possibly in conjunction with a PHY if
>> > there is a PHY present. Whether the PCS state gets forwarded to the MAC
>> > is hardware specific, and we have at least one DSA switch where this
>> > doesn't appear happen.
>> >
>> > Please realise that there are _three_ distinct modules here:
>> >
>> > - The MAC
>> > - The PCS
>> > - The PHY or media
>> 
>> Right, I sloppily used "PHY" to refer to the link partner on the other
>> end of the SERDES.  I realize that the remote PCS does not have to
>> reside within a PHY.
>
> Sigh, it seems I'm not making myself clear.
>
> Host system:
>
>   ---------------------------+
>     NIC (or DSA switch port) |
>      +-------+    +-------+  |
>      |       |    |       |  |
>      |  MAC  <---->  PCS  <-----------------------> PHY, SFP or media
>      |       |    |       |  |     ^
>      +-------+    +-------+  |     |
>                              |   phy interface type
>   ---------------------------+   also in-band signalling
>                                  which managed = "in-band-status"
> 				 applies to

This part is 100% clear

>> E.g. what does it mean to have an SGMII link where in-band signaling is
>> not used?  Is that not part of what defines SGMII?
>
> There _are_ PHYs out there that implement Cisco SGMII (which is IEEE
> 802.3 1000BASE-X modified to allow signalling at 10M and 100M speeds by
> symbol replication, and changing the format of the 1000BASE-X to provide
> the details of the SGMII link speed and duplex) but do _not_ support
> that in-band signalling.
>
> The point of SGMII without in-band signalling rather than just using
> 1000BASE-X without in-band signalling is that SGMII can operate at
> 10M and 100M, whereas 1000BASE-X can not.
>
> The usual situation, however, is that most devices that support Cisco
> SGMII also allow the in-band signalling to be configured to be used or
> not used.

Yes, I know about the relationship between 1000BASE-X and SGMII, I just
did not know that there were devices that only implemented the symbol
replication part.

> Going back to the diagram above, the link between the MAC and PCS is
> _not_ described in DT currently, not by the managed property not by
> the phy-modes etc properties.

Clear.

> Now, the port configuration register on the Marvell switches controls
> the MAC settings. The PCS has a separate register set (normally
> referred to as serdes in Marvell's Switch terminology) which is an
> IEEE compliant clause 22 register layout.

...or Clause 45 on the Amethyst - sure.  I am quite familiar with these
devices.  That is not the source of my confusion.

> The problem is, it seems *some* Marvell switches automatically forward
> the PCS status to the MAC. Other switches do not. The DT "managed"

Yes, I understand this problem, and that that is the reason for
rejecting the patch.

> property does not describe this - because - as stated above - the
> "managed" property applies to the link between the PCS and external
> world (which may be a PHY, or may be media) and _not_ between the
> MAC and its associated PCS.

I understand _where_ it applies and _what_ it describes, but I do not
understand _how_ a driver should make use of it.

In other words, my question is:

For a NIC driver to properly support the `managed` property, how should
the setup and/or runtime behavior of the hardware and/or the driver
differ with the two following configs?

    &eth0 {
        phy-connection-type = "sgmii";
        managed = "auto";
    };

vs.

    &eth0 {
        phy-connection-type = "sgmii";
        managed = "in-band-status";
    };

Re: [PATCH v2 net 3/4] net: dsa: mv88e6xxx: Never force link on in-band managed MACs

[Russell King (Oracle)]

On Sun, Jan 05, 2025 at 12:16:07AM +0100, Tobias Waldekranz wrote:
> On lör, jan 04, 2025 at 22:09, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > Host system:
> >
> >   ---------------------------+
> >     NIC (or DSA switch port) |
> >      +-------+    +-------+  |
> >      |       |    |       |  |
> >      |  MAC  <---->  PCS  <-----------------------> PHY, SFP or media
> >      |       |    |       |  |     ^
> >      +-------+    +-------+  |     |
> >                              |   phy interface type
> >   ---------------------------+   also in-band signalling
> >                                  which managed = "in-band-status"
> > 				 applies to
> 
> This part is 100% clear

Apparently it isn't, because...

> In other words, my question is:
> 
> For a NIC driver to properly support the `managed` property, how should
> the setup and/or runtime behavior of the hardware and/or the driver
> differ with the two following configs?
> 
>     &eth0 {
>         phy-connection-type = "sgmii";
>         managed = "auto";
>     };
> 
> vs.
> 
>     &eth0 {
>         phy-connection-type = "sgmii";
>         managed = "in-band-status";
>     };

if it were, you wouldn't be asking this question.

Once again. The "managed" property defines whether in-band signalling
is used over the "phy-connection-type" link, which for SGMII will be
between the PCS and PHY, as shown in my diagram above that you claim
to understand 100%, but by the fact you are again asking this question,
you do not understand it AT ALL.

I don't know how to better explain it to you, because I think I've been
absolutely clear at every stage what the "managed" property describes.
I now have nothing further to add if you still can't understand it, so,
sorry, I'm giving up answering your emails on this topic, because it's
just too frustrating to me to continue if you still don't "get it".

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 net 3/4] net: dsa: mv88e6xxx: Never force link on in-band managed MACs

[Tobias Waldekranz]

On sön, jan 05, 2025 at 10:41, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> On Sun, Jan 05, 2025 at 12:16:07AM +0100, Tobias Waldekranz wrote:
>> On lör, jan 04, 2025 at 22:09, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
>> > Host system:
>> >
>> >   ---------------------------+
>> >     NIC (or DSA switch port) |
>> >      +-------+    +-------+  |
>> >      |       |    |       |  |
>> >      |  MAC  <---->  PCS  <-----------------------> PHY, SFP or media
>> >      |       |    |       |  |     ^
>> >      +-------+    +-------+  |     |
>> >                              |   phy interface type
>> >   ---------------------------+   also in-band signalling
>> >                                  which managed = "in-band-status"
>> > 				 applies to
>> 
>> This part is 100% clear
>
> Apparently it isn't, because...
>
>> In other words, my question is:
>> 
>> For a NIC driver to properly support the `managed` property, how should
>> the setup and/or runtime behavior of the hardware and/or the driver
>> differ with the two following configs?
>> 
>>     &eth0 {
>>         phy-connection-type = "sgmii";
>>         managed = "auto";
>>     };
>> 
>> vs.
>> 
>>     &eth0 {
>>         phy-connection-type = "sgmii";
>>         managed = "in-band-status";
>>     };
>
> if it were, you wouldn't be asking this question.
>
> Once again. The "managed" property defines whether in-band signalling
> is used over the "phy-connection-type" link, which for SGMII will be
> between the PCS and PHY, as shown in my diagram above that you claim
> to understand 100%, but by the fact you are again asking this question,
> you do not understand it AT ALL.
>
> I don't know how to better explain it to you, because I think I've been
> absolutely clear at every stage what the "managed" property describes.
> I now have nothing further to add if you still can't understand it, so,
> sorry, I'm giving up answering your emails on this topic, because it's
> just too frustrating to me to continue if you still don't "get it".

I agree that you have clearly explained what it describes, many times.

My remaining question - which you acknowledge that I asked twice, yet
chose not to answer - was how software is supposed to _act_ on that
description; presuming that the property is not in the DT merely for
documentation purposes.

I will study the kernel sources more closely and try to find
enlightenment that way instead.  Thank you for your time.

Re: [PATCH v2 net 3/4] net: dsa: mv88e6xxx: Never force link on in-band managed MACs

[Russell King (Oracle)]

On Mon, Jan 06, 2025 at 12:30:25AM +0100, Tobias Waldekranz wrote:
> On sön, jan 05, 2025 at 10:41, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > On Sun, Jan 05, 2025 at 12:16:07AM +0100, Tobias Waldekranz wrote:
> >> On lör, jan 04, 2025 at 22:09, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> >> > Host system:
> >> >
> >> >   ---------------------------+
> >> >     NIC (or DSA switch port) |
> >> >      +-------+    +-------+  |
> >> >      |       |    |       |  |
> >> >      |  MAC  <---->  PCS  <-----------------------> PHY, SFP or media
> >> >      |       |    |       |  |     ^
> >> >      +-------+    +-------+  |     |
> >> >                              |   phy interface type
> >> >   ---------------------------+   also in-band signalling
> >> >                                  which managed = "in-band-status"
> >> > 				 applies to
> >> 
> >> This part is 100% clear
> >
> > Apparently it isn't, because...
> >
> >> In other words, my question is:
> >> 
> >> For a NIC driver to properly support the `managed` property, how should
> >> the setup and/or runtime behavior of the hardware and/or the driver
> >> differ with the two following configs?
> >> 
> >>     &eth0 {
> >>         phy-connection-type = "sgmii";
> >>         managed = "auto";
> >>     };
> >> 
> >> vs.
> >> 
> >>     &eth0 {
> >>         phy-connection-type = "sgmii";
> >>         managed = "in-band-status";
> >>     };
> >
> > if it were, you wouldn't be asking this question.
> >
> > Once again. The "managed" property defines whether in-band signalling
> > is used over the "phy-connection-type" link, which for SGMII will be
> > between the PCS and PHY, as shown in my diagram above that you claim
> > to understand 100%, but by the fact you are again asking this question,
> > you do not understand it AT ALL.
> >
> > I don't know how to better explain it to you, because I think I've been
> > absolutely clear at every stage what the "managed" property describes.
> > I now have nothing further to add if you still can't understand it, so,
> > sorry, I'm giving up answering your emails on this topic, because it's
> > just too frustrating to me to continue if you still don't "get it".
> 
> I agree that you have clearly explained what it describes, many times.
> 
> My remaining question - which you acknowledge that I asked twice, yet
> chose not to answer - was how software is supposed to _act_ on that

I *have* answered it. Every time.

> description; presuming that the property is not in the DT merely for
> documentation purposes.

Utter claptap. Total rubbish. Completely wrong. It is acted on. It
causes phylink to enter in-band mode, and use the PCS to obtain the
in-band data instead of the PHY.

YOU are the one refusing to listen to what I'm saying, yet you claim
my explanations are clear and you understand them. You parently do
not.

End. Of. Discussion.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 net 3/4] net: dsa: mv88e6xxx: Never force link on in-band managed MACs

[Tobias Waldekranz]

On mån, jan 06, 2025 at 08:20, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> On Mon, Jan 06, 2025 at 12:30:25AM +0100, Tobias Waldekranz wrote:
>> On sön, jan 05, 2025 at 10:41, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
>> > On Sun, Jan 05, 2025 at 12:16:07AM +0100, Tobias Waldekranz wrote:
>> >> On lör, jan 04, 2025 at 22:09, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
>> >> > Host system:
>> >> >
>> >> >   ---------------------------+
>> >> >     NIC (or DSA switch port) |
>> >> >      +-------+    +-------+  |
>> >> >      |       |    |       |  |
>> >> >      |  MAC  <---->  PCS  <-----------------------> PHY, SFP or media
>> >> >      |       |    |       |  |     ^
>> >> >      +-------+    +-------+  |     |
>> >> >                              |   phy interface type
>> >> >   ---------------------------+   also in-band signalling
>> >> >                                  which managed = "in-band-status"
>> >> > 				 applies to
>> >> 
>> >> This part is 100% clear
>> >
>> > Apparently it isn't, because...
>> >
>> >> In other words, my question is:
>> >> 
>> >> For a NIC driver to properly support the `managed` property, how should
>> >> the setup and/or runtime behavior of the hardware and/or the driver
>> >> differ with the two following configs?
>> >> 
>> >>     &eth0 {
>> >>         phy-connection-type = "sgmii";
>> >>         managed = "auto";
>> >>     };
>> >> 
>> >> vs.
>> >> 
>> >>     &eth0 {
>> >>         phy-connection-type = "sgmii";
>> >>         managed = "in-band-status";
>> >>     };
>> >
>> > if it were, you wouldn't be asking this question.
>> >
>> > Once again. The "managed" property defines whether in-band signalling
>> > is used over the "phy-connection-type" link, which for SGMII will be
>> > between the PCS and PHY, as shown in my diagram above that you claim
>> > to understand 100%, but by the fact you are again asking this question,
>> > you do not understand it AT ALL.
>> >
>> > I don't know how to better explain it to you, because I think I've been
>> > absolutely clear at every stage what the "managed" property describes.
>> > I now have nothing further to add if you still can't understand it, so,
>> > sorry, I'm giving up answering your emails on this topic, because it's
>> > just too frustrating to me to continue if you still don't "get it".
>> 
>> I agree that you have clearly explained what it describes, many times.
>> 
>> My remaining question - which you acknowledge that I asked twice, yet
>> chose not to answer - was how software is supposed to _act_ on that
>
> I *have* answered it. Every time.
>
>> description; presuming that the property is not in the DT merely for
>> documentation purposes.
>
> Utter claptap. Total rubbish. Completely wrong. It is acted on. It

I have _never_ said that is was not.  In the very sentence you are
quoting I stated my presumption that it was _not_ merely there as
documentation.  I simply wanted to _how_ the kernel acts on it - the
thing you summerize with the following sentence...

> causes phylink to enter in-band mode, and use the PCS to obtain the
> in-band data instead of the PHY.

..._this_ is the process I wanted to learn more about.  How phylink
operates in the two different modes, what the responsibilites of the NIC
driver are, of the PHY driver (if applicable), etc.

> YOU are the one refusing to listen to what I'm saying, yet you claim
> my explanations are clear and you understand them. You parently do
> not.

I am sure that there are several points you have made that I ought to
have groked sooner, and that I could have stated my questions more
clearly.  I am only human.  Let me assure you that I have read each of
your replies many times over, each time wanting nothing more than to
have that light bulb moment.

Sincerely, I am thankful that you have taken the time to try to help me,
and I never meant to cause any distress.

[PATCH v2 net 4/4] net: dsa: mv88e6xxx: Limit rsvd2cpu policy to user ports on 6393X

[Tobias Waldekranz]

For packets with a DA in the IEEE reserved L2 group range, originating
from a CPU, forward it as normal, rather than classifying it as
management.

Example use-case:

     bridge (group_fwd_mask 0x4000)
     / |  \
 swp1 swp2 tap0
   \   /
(mv88e6xxx)

We've created a bridge with a non-zero group_fwd_mask (allowing LLDP
in this example) containing a set of ports managed by mv88e6xxx and
some foreign interface (e.g. an L2 VPN tunnel).

Since an LLDP packet coming in to the bridge from the other side of
tap0 is eligable for tx forward offloading, a FORWARD frame destined
for swp1 and swp2 would be send to the conduit interface.

Before this change, due to rsvd2cpu being enabled on the CPU port, the
switch would try to trap it back to the CPU. Given that the CPU is
trusted, instead assume that it indeed meant for the packet to be
forwarded like any other.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/port.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 56ed2f57fef8..bf6d558c112c 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -1416,6 +1416,23 @@ static int mv88e6393x_port_policy_write_all(struct mv88e6xxx_chip *chip,
 	return 0;
 }
 
+static int mv88e6393x_port_policy_write_user(struct mv88e6xxx_chip *chip,
+					     u16 pointer, u8 data)
+{
+	int err, port;
+
+	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
+		if (!dsa_is_user_port(chip->ds, port))
+			continue;
+
+		err = mv88e6393x_port_policy_write(chip, port, pointer, data);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 int mv88e6393x_set_egress_port(struct mv88e6xxx_chip *chip,
 			       enum mv88e6xxx_egress_direction direction,
 			       int port)
@@ -1457,26 +1474,28 @@ int mv88e6393x_port_mgmt_rsvd2cpu(struct mv88e6xxx_chip *chip)
 	int err;
 
 	/* Consider the frames with reserved multicast destination
-	 * addresses matching 01:80:c2:00:00:00 and
-	 * 01:80:c2:00:00:02 as MGMT.
+	 * addresses matching 01:80:c2:00:00:00 and 01:80:c2:00:00:02
+	 * as MGMT when received on user ports. Forward as normal on
+	 * CPU/DSA ports, to support bridges with non-zero
+	 * group_fwd_masks.
 	 */
 	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000000XLO;
-	err = mv88e6393x_port_policy_write_all(chip, ptr, 0xff);
+	err = mv88e6393x_port_policy_write_user(chip, ptr, 0xff);
 	if (err)
 		return err;
 
 	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000000XHI;
-	err = mv88e6393x_port_policy_write_all(chip, ptr, 0xff);
+	err = mv88e6393x_port_policy_write_user(chip, ptr, 0xff);
 	if (err)
 		return err;
 
 	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000002XLO;
-	err = mv88e6393x_port_policy_write_all(chip, ptr, 0xff);
+	err = mv88e6393x_port_policy_write_user(chip, ptr, 0xff);
 	if (err)
 		return err;
 
 	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000002XHI;
-	err = mv88e6393x_port_policy_write_all(chip, ptr, 0xff);
+	err = mv88e6393x_port_policy_write_user(chip, ptr, 0xff);
 	if (err)
 		return err;
 
-- 
2.43.0

Re: [PATCH v2 net 4/4] net: dsa: mv88e6xxx: Limit rsvd2cpu policy to user ports on 6393X

[Andrew Lunn]

On Thu, Dec 19, 2024 at 01:30:43PM +0100, Tobias Waldekranz wrote:
> For packets with a DA in the IEEE reserved L2 group range, originating
> from a CPU, forward it as normal, rather than classifying it as
> management.
> 
> Example use-case:
> 
>      bridge (group_fwd_mask 0x4000)
>      / |  \
>  swp1 swp2 tap0
>    \   /
> (mv88e6xxx)
> 
> We've created a bridge with a non-zero group_fwd_mask (allowing LLDP
> in this example) containing a set of ports managed by mv88e6xxx and
> some foreign interface (e.g. an L2 VPN tunnel).
> 
> Since an LLDP packet coming in to the bridge from the other side of
> tap0 is eligable for tx forward offloading, a FORWARD frame destined
> for swp1 and swp2 would be send to the conduit interface.
> 
> Before this change, due to rsvd2cpu being enabled on the CPU port, the
> switch would try to trap it back to the CPU. Given that the CPU is
> trusted, instead assume that it indeed meant for the packet to be
> forwarded like any other.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 net 4/4] net: dsa: mv88e6xxx: Limit rsvd2cpu policy to user ports on 6393X

[Vladimir Oltean]

On Thu, Dec 19, 2024 at 01:30:43PM +0100, Tobias Waldekranz wrote:
> For packets with a DA in the IEEE reserved L2 group range, originating
> from a CPU, forward it as normal, rather than classifying it as
> management.

Doesn't this break STP? Must be able to inject into ports with an STP
state other than FORWARDING. I expect that you need a DSA_CMD_FROM_CPU
tag for that, can't do it with DSA_CMD_FORWARD.

> Example use-case:
> 
>      bridge (group_fwd_mask 0x4000)
>      / |  \
>  swp1 swp2 tap0
>    \   /
> (mv88e6xxx)
> 
> We've created a bridge with a non-zero group_fwd_mask (allowing LLDP
> in this example) containing a set of ports managed by mv88e6xxx and
> some foreign interface (e.g. an L2 VPN tunnel).
> 
> Since an LLDP packet coming in to the bridge from the other side of
> tap0 is eligable for tx forward offloading, a FORWARD frame destined
> for swp1 and swp2 would be send to the conduit interface.
> 
> Before this change, due to rsvd2cpu being enabled on the CPU port, the
> switch would try to trap it back to the CPU. Given that the CPU is
> trusted, instead assume that it indeed meant for the packet to be
> forwarded like any other.

It looks like an oversight in the switchdev tx_fwd_offload scheme. Can't
we teach nbp_switchdev_frame_mark_tx_fwd_offload() to make an exception
for is_link_local_ether_addr() packets, and not set skb->offload_fwd_mark?

Re: [PATCH v2 net 4/4] net: dsa: mv88e6xxx: Limit rsvd2cpu policy to user ports on 6393X

[Vladimir Oltean]

On Thu, Dec 19, 2024 at 04:05:41PM +0200, Vladimir Oltean wrote:
> On Thu, Dec 19, 2024 at 01:30:43PM +0100, Tobias Waldekranz wrote:
> > For packets with a DA in the IEEE reserved L2 group range, originating
> > from a CPU, forward it as normal, rather than classifying it as
> > management.
> 
> Doesn't this break STP? Must be able to inject into ports with an STP
> state other than FORWARDING. I expect that you need a DSA_CMD_FROM_CPU
> tag for that, can't do it with DSA_CMD_FORWARD.

ah, I made a stupid mistake, I though locally generated STP goes through
__br_forward().

I put a WARN_ON_ONCE(skb->protocol == htons(ETH_P_802_2)); in DSA xmit
and convinced myself that this is not the case.

[   67.115425]  br_send_bpdu+0x130/0x2a0
[   67.119187]  br_send_config_bpdu+0x12c/0x170
[   67.123559]  br_transmit_config+0x114/0x180
[   67.127842]  br_config_bpdu_generation+0x6c/0x88
[   67.132562]  br_hello_timer_expired+0x44/0x98
[   67.137022]  call_timer_fn+0xc8/0x300

Please ignore this comment.

Re: [PATCH v2 net 4/4] net: dsa: mv88e6xxx: Limit rsvd2cpu policy to user ports on 6393X

[Tobias Waldekranz]

On tor, dec 19, 2024 at 16:05, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Thu, Dec 19, 2024 at 01:30:43PM +0100, Tobias Waldekranz wrote:
>> For packets with a DA in the IEEE reserved L2 group range, originating
>> from a CPU, forward it as normal, rather than classifying it as
>> management.
>
> Doesn't this break STP? Must be able to inject into ports with an STP
> state other than FORWARDING. I expect that you need a DSA_CMD_FROM_CPU
> tag for that, can't do it with DSA_CMD_FORWARD.

You need a FROM_CPU to force a packet out through a blocking port,
yes. But I don't see how this could apply to STP.

If STP is enabled on the bridge, we will never allow BPDUs to be
forwarded. Locally originating STP BPDUs are always injected directly on
the lower interface, so OFM is never set on those.

If STP is disabled on the bridge, then we will forward incoming BPDUs
(br_handle_frame()). In that case OFM could be set if the BPDU came in
on a foreign port. But since STP is disabled, no port will be blocked in
this case, so it would not matter.

>> Example use-case:
>> 
>>      bridge (group_fwd_mask 0x4000)
>>      / |  \
>>  swp1 swp2 tap0
>>    \   /
>> (mv88e6xxx)
>> 
>> We've created a bridge with a non-zero group_fwd_mask (allowing LLDP
>> in this example) containing a set of ports managed by mv88e6xxx and
>> some foreign interface (e.g. an L2 VPN tunnel).
>> 
>> Since an LLDP packet coming in to the bridge from the other side of
>> tap0 is eligable for tx forward offloading, a FORWARD frame destined
>> for swp1 and swp2 would be send to the conduit interface.
>> 
>> Before this change, due to rsvd2cpu being enabled on the CPU port, the
>> switch would try to trap it back to the CPU. Given that the CPU is
>> trusted, instead assume that it indeed meant for the packet to be
>> forwarded like any other.
>
> It looks like an oversight in the switchdev tx_fwd_offload scheme. Can't
> we teach nbp_switchdev_frame_mark_tx_fwd_offload() to make an exception
> for is_link_local_ether_addr() packets, and not set skb->offload_fwd_mark?

That sounds like a better option if it is acceptible to the broader
community. I thought that this might be a quirk of mv88e6xxx's rsvd2cpu
bits. But if more devices behave in the same way, then it would be
better to just exempt this whole class from offloading.

Do you know how any other ASICs behave from this perspective?

Re: [PATCH v2 net 4/4] net: dsa: mv88e6xxx: Limit rsvd2cpu policy to user ports on 6393X

[Vladimir Oltean]

On Thu, Dec 19, 2024 at 03:34:43PM +0100, Tobias Waldekranz wrote:
> On tor, dec 19, 2024 at 16:05, Vladimir Oltean <olteanv@gmail.com> wrote:
> > It looks like an oversight in the switchdev tx_fwd_offload scheme. Can't
> > we teach nbp_switchdev_frame_mark_tx_fwd_offload() to make an exception
> > for is_link_local_ether_addr() packets, and not set skb->offload_fwd_mark?
> 
> That sounds like a better option if it is acceptible to the broader
> community. I thought that this might be a quirk of mv88e6xxx's rsvd2cpu
> bits. But if more devices behave in the same way, then it would be
> better to just exempt this whole class from offloading.
> 
> Do you know how any other ASICs behave from this perspective?

The other driver with tx_fwd_offload, sja1105, is going to drop any
packet coming from the host_port which isn't sent through a management
route (set up by sja1105_defer_xmit()). So it's more than likely bugged.

We can't fix this from sja1105_xmit() by reordering sja1105_imprecise_xmit()
and sja1105_defer_xmit(). It's not just the order of operations in the
tagger. It's the fact that the bridge thinks it doesn't need to clone
the skb, and it does.

So yes, it's probably best to exclude link-local from skb->offload_fwd_mark.

Re: [PATCH v2 net 4/4] net: dsa: mv88e6xxx: Limit rsvd2cpu policy to user ports on 6393X

[Vladimir Oltean]

On Thu, Dec 19, 2024 at 04:42:08PM +0200, Vladimir Oltean wrote:
> The other driver with tx_fwd_offload, sja1105,

Correction: I forgot there is one more driver with tx_fwd_offload:
vsc73xx, but that doesn't properly support link-local traffic yet at all,
according to the vsc73xx_port_stp_state_set() comment. So we can ignore it.

> is going to drop any
> packet coming from the host_port which isn't sent through a management
> route (set up by sja1105_defer_xmit()). So it's more than likely bugged.
> 
> We can't fix this from sja1105_xmit() by reordering sja1105_imprecise_xmit()
> and sja1105_defer_xmit(). It's not just the order of operations in the
> tagger. It's the fact that the bridge thinks it doesn't need to clone
> the skb, and it does.

Another correction: we could probably make a best-effort attempt to
honor skb->offload_fwd_mark in sja1105_mgmt_xmit() by setting mgmt_route.destports
to the bit mask of all other ports that are in dsa_port_bridge_dev_get(dp).
But it gets unpleasantly difficult to manage, plus I think we still don't
get MAC SA learning from these packets.

> So yes, it's probably best to exclude link-local from skb->offload_fwd_mark.

So I'm still of this opinion :) I think the effort to handle the corner
cases isn't worth it relative to the benefit of offloading the forwarding
of slow protocols.

Re: [PATCH v2 net 4/4] net: dsa: mv88e6xxx: Limit rsvd2cpu policy to user ports on 6393X

[Tobias Waldekranz]

On tor, dec 19, 2024 at 16:52, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Thu, Dec 19, 2024 at 04:42:08PM +0200, Vladimir Oltean wrote:
>> The other driver with tx_fwd_offload, sja1105,
>
> Correction: I forgot there is one more driver with tx_fwd_offload:
> vsc73xx, but that doesn't properly support link-local traffic yet at all,
> according to the vsc73xx_port_stp_state_set() comment. So we can ignore it.
>
>> is going to drop any
>> packet coming from the host_port which isn't sent through a management
>> route (set up by sja1105_defer_xmit()). So it's more than likely bugged.
>> 
>> We can't fix this from sja1105_xmit() by reordering sja1105_imprecise_xmit()
>> and sja1105_defer_xmit(). It's not just the order of operations in the
>> tagger. It's the fact that the bridge thinks it doesn't need to clone
>> the skb, and it does.
>
> Another correction: we could probably make a best-effort attempt to
> honor skb->offload_fwd_mark in sja1105_mgmt_xmit() by setting mgmt_route.destports
> to the bit mask of all other ports that are in dsa_port_bridge_dev_get(dp).
> But it gets unpleasantly difficult to manage, plus I think we still don't
> get MAC SA learning from these packets.
>
>> So yes, it's probably best to exclude link-local from skb->offload_fwd_mark.
>
> So I'm still of this opinion :) I think the effort to handle the corner
> cases isn't worth it relative to the benefit of offloading the forwarding
> of slow protocols.

Agreed. I will remove this patch in v3 and replace it with a general
patch for the bridge instead. Thanks!

Re: [PATCH v2 net 4/4] net: dsa: mv88e6xxx: Limit rsvd2cpu policy to user ports on 6393X

[Vladimir Oltean]

On Thu, Dec 19, 2024 at 01:30:43PM +0100, Tobias Waldekranz wrote:
> For packets with a DA in the IEEE reserved L2 group range, originating
> from a CPU, forward it as normal, rather than classifying it as
> management.
> 
> Example use-case:
> 
>      bridge (group_fwd_mask 0x4000)
>      / |  \
>  swp1 swp2 tap0
>    \   /
> (mv88e6xxx)
> 
> We've created a bridge with a non-zero group_fwd_mask (allowing LLDP
> in this example) containing a set of ports managed by mv88e6xxx and
> some foreign interface (e.g. an L2 VPN tunnel).
> 
> Since an LLDP packet coming in to the bridge from the other side of
> tap0 is eligable for tx forward offloading, a FORWARD frame destined
> for swp1 and swp2 would be send to the conduit interface.
> 
> Before this change, due to rsvd2cpu being enabled on the CPU port, the
> switch would try to trap it back to the CPU. Given that the CPU is
> trusted, instead assume that it indeed meant for the packet to be
> forwarded like any other.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Is it fair to say that commit d82f8ab0d874 ("net: dsa: tag_dsa: offload
the bridge forwarding process") broke this and that we need a Fixes: tag
for that and not earlier? Prior to that, I believe that rsvd2cpu would
not affect these forwarded reserved L2 multicast groups, because they
were sent with FROM_CPU.

> ---
>  drivers/net/dsa/mv88e6xxx/port.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> index 56ed2f57fef8..bf6d558c112c 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.c
> +++ b/drivers/net/dsa/mv88e6xxx/port.c
> @@ -1416,6 +1416,23 @@ static int mv88e6393x_port_policy_write_all(struct mv88e6xxx_chip *chip,
>  	return 0;
>  }
>  
> +static int mv88e6393x_port_policy_write_user(struct mv88e6xxx_chip *chip,
> +					     u16 pointer, u8 data)
> +{
> +	int err, port;
> +
> +	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
> +		if (!dsa_is_user_port(chip->ds, port))
> +			continue;

Can you remember to convert this "dsa_to_port() called in a loop"
antipattern to dsa_switch_for_each_user_port() in net-next? I wanted to
ask you to do it now, but the blamed commit is in kernel 5.15, and
82b318983c51 ("net: dsa: introduce helpers for iterating through ports
using dp") made its appearance in 5.16.

> +
> +		err = mv88e6393x_port_policy_write(chip, port, pointer, data);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
>  int mv88e6393x_set_egress_port(struct mv88e6xxx_chip *chip,
>  			       enum mv88e6xxx_egress_direction direction,
>  			       int port)
> @@ -1457,26 +1474,28 @@ int mv88e6393x_port_mgmt_rsvd2cpu(struct mv88e6xxx_chip *chip)
>  	int err;
>  
>  	/* Consider the frames with reserved multicast destination
> -	 * addresses matching 01:80:c2:00:00:00 and
> -	 * 01:80:c2:00:00:02 as MGMT.
> +	 * addresses matching 01:80:c2:00:00:00 and 01:80:c2:00:00:02

Is the comment correct? LLDP is group 01:80:c2:00:00:0e. It doesn't make
it sound like what is done here should affect it.

> +	 * as MGMT when received on user ports. Forward as normal on
> +	 * CPU/DSA ports, to support bridges with non-zero
> +	 * group_fwd_masks.
>  	 */
>  	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000000XLO;
> -	err = mv88e6393x_port_policy_write_all(chip, ptr, 0xff);
> +	err = mv88e6393x_port_policy_write_user(chip, ptr, 0xff);
>  	if (err)
>  		return err;
>  
>  	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000000XHI;
> -	err = mv88e6393x_port_policy_write_all(chip, ptr, 0xff);
> +	err = mv88e6393x_port_policy_write_user(chip, ptr, 0xff);
>  	if (err)
>  		return err;
>  
>  	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000002XLO;
> -	err = mv88e6393x_port_policy_write_all(chip, ptr, 0xff);
> +	err = mv88e6393x_port_policy_write_user(chip, ptr, 0xff);
>  	if (err)
>  		return err;
>  
>  	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000002XHI;
> -	err = mv88e6393x_port_policy_write_all(chip, ptr, 0xff);
> +	err = mv88e6393x_port_policy_write_user(chip, ptr, 0xff);
>  	if (err)
>  		return err;
>  
> -- 
> 2.43.0
>