[PATCH net] net: dsa: Don't instantiate phylink for CPU/DSA ports unless needed

[PATCH net] net: dsa: Don't instantiate phylink for CPU/DSA ports unless needed

[Andrew Lunn]

By default, DSA drivers should configure CPU and DSA ports to their
maximum speed. In many configurations this is sufficient to make the
link work.

In some cases it is necessary to configure the link to run slower,
e.g. because of limitations of the SoC it is connected to. Or back to
back PHYs are used and the PHY needs to be driven in order to
establish link. In this case, phylink is used.

Only instantiate phylink if it is required. If there is no PHY, or no
fixed link properties, phylink can upset a link which works in the
default configuration.

Fixes: 0e27921816ad ("net: dsa: Use PHYLINK for the CPU/DSA ports")
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/port.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index ed7dabb57985..ec13dc666788 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -648,9 +648,14 @@ static int dsa_port_phylink_register(struct dsa_port *dp)
 int dsa_port_link_register_of(struct dsa_port *dp)
 {
 	struct dsa_switch *ds = dp->ds;
+	struct device_node *phy_np;
 
-	if (!ds->ops->adjust_link)
-		return dsa_port_phylink_register(dp);
+	if (!ds->ops->adjust_link) {
+		phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
+		if (of_phy_is_fixed_link(dp->dn) || phy_np)
+			return dsa_port_phylink_register(dp);
+		return 0;
+	}
 
 	dev_warn(ds->dev,
 		 "Using legacy PHYLIB callbacks. Please migrate to PHYLINK!\n");
@@ -665,11 +670,12 @@ void dsa_port_link_unregister_of(struct dsa_port *dp)
 {
 	struct dsa_switch *ds = dp->ds;
 
-	if (!ds->ops->adjust_link) {
+	if (!ds->ops->adjust_link && dp->pl) {
 		rtnl_lock();
 		phylink_disconnect_phy(dp->pl);
 		rtnl_unlock();
 		phylink_destroy(dp->pl);
+		dp->pl = NULL;
 		return;
 	}
 
-- 
2.25.1

Re: [PATCH net] net: dsa: Don't instantiate phylink for CPU/DSA ports unless needed

[Vladimir Oltean]

On Wed, 11 Mar 2020 at 17:24, Andrew Lunn <andrew@lunn.ch> wrote:
>
> By default, DSA drivers should configure CPU and DSA ports to their
> maximum speed. In many configurations this is sufficient to make the
> link work.
>
> In some cases it is necessary to configure the link to run slower,
> e.g. because of limitations of the SoC it is connected to. Or back to
> back PHYs are used and the PHY needs to be driven in order to
> establish link. In this case, phylink is used.
>
> Only instantiate phylink if it is required. If there is no PHY, or no
> fixed link properties, phylink can upset a link which works in the
> default configuration.
>
> Fixes: 0e27921816ad ("net: dsa: Use PHYLINK for the CPU/DSA ports")
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---


No regressions on ocelot/felix and sja1105, which have no .adjust_link
and do have fixed-link for the CPU port.

>  net/dsa/port.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index ed7dabb57985..ec13dc666788 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -648,9 +648,14 @@ static int dsa_port_phylink_register(struct dsa_port *dp)
>  int dsa_port_link_register_of(struct dsa_port *dp)
>  {
>         struct dsa_switch *ds = dp->ds;
> +       struct device_node *phy_np;
>
> -       if (!ds->ops->adjust_link)
> -               return dsa_port_phylink_register(dp);
> +       if (!ds->ops->adjust_link) {
> +               phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
> +               if (of_phy_is_fixed_link(dp->dn) || phy_np)
> +                       return dsa_port_phylink_register(dp);
> +               return 0;
> +       }
>
>         dev_warn(ds->dev,
>                  "Using legacy PHYLIB callbacks. Please migrate to PHYLINK!\n");
> @@ -665,11 +670,12 @@ void dsa_port_link_unregister_of(struct dsa_port *dp)
>  {
>         struct dsa_switch *ds = dp->ds;
>
> -       if (!ds->ops->adjust_link) {
> +       if (!ds->ops->adjust_link && dp->pl) {
>                 rtnl_lock();
>                 phylink_disconnect_phy(dp->pl);
>                 rtnl_unlock();
>                 phylink_destroy(dp->pl);
> +               dp->pl = NULL;
>                 return;
>         }
>
> --
> 2.25.1
>

Thanks,
-Vladimir

Re: [PATCH net] net: dsa: Don't instantiate phylink for CPU/DSA ports unless needed

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Wed, 11 Mar 2020 16:24:24 +0100

> By default, DSA drivers should configure CPU and DSA ports to their
> maximum speed. In many configurations this is sufficient to make the
> link work.
> 
> In some cases it is necessary to configure the link to run slower,
> e.g. because of limitations of the SoC it is connected to. Or back to
> back PHYs are used and the PHY needs to be driven in order to
> establish link. In this case, phylink is used.
> 
> Only instantiate phylink if it is required. If there is no PHY, or no
> fixed link properties, phylink can upset a link which works in the
> default configuration.
> 
> Fixes: 0e27921816ad ("net: dsa: Use PHYLINK for the CPU/DSA ports")
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Applied and queued up for -stable, thanks.

Re: [PATCH net] net: dsa: Don't instantiate phylink for CPU/DSA ports unless needed

[Russell King - ARM Linux admin]

On Wed, Mar 11, 2020 at 04:24:24PM +0100, Andrew Lunn wrote:
> By default, DSA drivers should configure CPU and DSA ports to their
> maximum speed. In many configurations this is sufficient to make the
> link work.
> 
> In some cases it is necessary to configure the link to run slower,
> e.g. because of limitations of the SoC it is connected to. Or back to
> back PHYs are used and the PHY needs to be driven in order to
> establish link. In this case, phylink is used.
> 
> Only instantiate phylink if it is required. If there is no PHY, or no
> fixed link properties, phylink can upset a link which works in the
> default configuration.
> 
> Fixes: 0e27921816ad ("net: dsa: Use PHYLINK for the CPU/DSA ports")
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Hi Andrew,

This seems to conflict with the serdes changes.  For a CPU port using
a serdes connection, we claim the serdes interrupt, which then triggers
this:

[    9.069631] Unable to handle kernel NULL pointer dereference at virtual address 00000124
[    9.077785] pgd = c0004000
[    9.080528] [00000124] *pgd=00000000
[    9.084157] Internal error: Oops: 805 [#1] SMP ARM
[    9.088961] Modules linked in: tag_edsa spi_nor mtd xhci_plat_hcd mv88e6xxx(+) xhci_hcd armada_thermal marvell_cesa dsa_core ehci_orion libdes phy_armada38x_comphy at24 mcp3021 sfp evbug spi_orion sff mdio_i2c
[    9.107625] CPU: 1 PID: 214 Comm: irq/55-mv88e6xx Not tainted 5.6.0+ #470
[    9.114429] Hardware name: Marvell Armada 380/385 (Device Tree)
[    9.120371] PC is at phylink_mac_change+0x10/0x88
[    9.125129] LR is at mv88e6352_serdes_irq_status+0x74/0x94 [mv88e6xxx]

which is because dp->pl is NULL in dsa_port_phylink_mac_change() as a
result of this commit for links operating at max speed without a
fixed-link property.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up