[PATCH net] net: dsa: mv88e6xxx: fix in-band AN link establishment

[PATCH net] net: dsa: mv88e6xxx: fix in-band AN link establishment

[Russell King]

If in-band negotiation or fixed-link modes are specified for a DSA
port, the DSA code will force the link down during initialisation. For
fixed-link mode, this is fine, as phylink will manage the link state.
However, for in-band mode, phylink expects the PCS to detect link,
which will not happen if the link is forced down.

There is a related issue that in in-band mode, the link could come up
while we are making configuration changes, so we should force the link
down prior to reconfiguring the interface mode.

This patch addresses both issues.

Fixes: 3be98b2d5fbc ("net: dsa: Down cpu/dsa ports phylink will control")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 20 +++++++++++++++++---
 drivers/net/dsa/mv88e6xxx/chip.h |  1 +
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 0bce26f1df93..9c7b8cf0e39a 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -664,6 +664,7 @@ static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
 				 const struct phylink_link_state *state)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
+	struct mv88e6xxx_port *p = &chip->ports[port];
 	int err;
 
 	/* FIXME: is this the correct test? If we're in fixed mode on an
@@ -675,10 +676,14 @@ static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
 		return;
 
 	mv88e6xxx_reg_lock(chip);
-	/* FIXME: should we force the link down here - but if we do, how
-	 * do we restore the link force/unforce state? The driver layering
-	 * gets in the way.
+	/* In inband mode, the link may come up at any time while the link
+	 * is not forced down. Force the link down while we reconfigure the
+	 * interface mode.
 	 */
+	if (mode == MLO_AN_INBAND && p->interface != state->interface &&
+	    chip->info->ops->port_set_link)
+		chip->info->ops->port_set_link(chip, port, LINK_FORCED_DOWN);
+
 	err = mv88e6xxx_port_config_interface(chip, port, state->interface);
 	if (err && err != -EOPNOTSUPP)
 		goto err_unlock;
@@ -691,6 +696,15 @@ static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
 	if (err > 0)
 		err = 0;
 
+	/* Undo the forced down state above after completing configuration
+	 * irrespective of its state on entry, which allows the link to come up.
+	 */
+	if (mode == MLO_AN_INBAND && p->interface != state->interface &&
+	    chip->info->ops->port_set_link)
+		chip->info->ops->port_set_link(chip, port, LINK_UNFORCED);
+
+	p->interface = state->interface;
+
 err_unlock:
 	mv88e6xxx_reg_unlock(chip);
 
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index f78536bdfe39..a8ef7edbb80b 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -232,6 +232,7 @@ struct mv88e6xxx_port {
 	u64 atu_full_violation;
 	u64 vtu_member_violation;
 	u64 vtu_miss_violation;
+	phy_interface_t interface;
 	u8 cmode;
 	bool mirror_ingress;
 	bool mirror_egress;
-- 
2.20.1

Re: [PATCH net] net: dsa: mv88e6xxx: fix in-band AN link establishment

[Andrew Lunn]

On Sun, Jul 19, 2020 at 12:00:35PM +0100, Russell King wrote:
> If in-band negotiation or fixed-link modes are specified for a DSA
> port, the DSA code will force the link down during initialisation. For
> fixed-link mode, this is fine, as phylink will manage the link state.
> However, for in-band mode, phylink expects the PCS to detect link,
> which will not happen if the link is forced down.
> 
> There is a related issue that in in-band mode, the link could come up
> while we are making configuration changes, so we should force the link
> down prior to reconfiguring the interface mode.
> 
> This patch addresses both issues.
> 
> Fixes: 3be98b2d5fbc ("net: dsa: Down cpu/dsa ports phylink will control")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 20 +++++++++++++++++---
>  drivers/net/dsa/mv88e6xxx/chip.h |  1 +
>  2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 0bce26f1df93..9c7b8cf0e39a 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -664,6 +664,7 @@ static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
>  				 const struct phylink_link_state *state)
>  {
>  	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct mv88e6xxx_port *p = &chip->ports[port];
>  	int err;

David might not like the reverse christmas tree breakage, but:

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

    Andrew

Re: [PATCH net] net: dsa: mv88e6xxx: fix in-band AN link establishment

[David Miller]

From: Russell King <rmk+kernel@armlinux.org.uk>
Date: Sun, 19 Jul 2020 12:00:35 +0100

> If in-band negotiation or fixed-link modes are specified for a DSA
> port, the DSA code will force the link down during initialisation. For
> fixed-link mode, this is fine, as phylink will manage the link state.
> However, for in-band mode, phylink expects the PCS to detect link,
> which will not happen if the link is forced down.
> 
> There is a related issue that in in-band mode, the link could come up
> while we are making configuration changes, so we should force the link
> down prior to reconfiguring the interface mode.
> 
> This patch addresses both issues.
> 
> Fixes: 3be98b2d5fbc ("net: dsa: Down cpu/dsa ports phylink will control")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Applied and queued up for -stable,  but:

> @@ -664,6 +664,7 @@ static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
>  				 const struct phylink_link_state *state)
>  {
>  	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct mv88e6xxx_port *p = &chip->ports[port];
>  	int err;

I fixed the reverse christmas tree breakage here by moving the 'p'
assignment into the function body.