Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1737616 > unrolled thread

[PATCH net-next 0/4] net: dsa: simplify port enabling

Back to article view | Back to linux.kernel

Contents

  [PATCH net-next 0/4] net: dsa: simplify port enabling Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-09-22 18:30 +0200
    [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-09-22 18:30 +0200
      Re: [PATCH net-next 2/4] net: dsa: remove phy arg from port  enable/disable Florian Fainelli <f.fainelli@gmail.com> - 2017-09-22 19:20 +0200
        Re: [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-09-22 20:20 +0200
          Re: [PATCH net-next 2/4] net: dsa: remove phy arg from port  enable/disable Florian Fainelli <f.fainelli@gmail.com> - 2017-09-22 20:30 +0200
            Re: [PATCH net-next 2/4] net: dsa: remove phy arg from port  enable/disable Andrew Lunn <andrew@lunn.ch> - 2017-09-22 21:20 +0200
    [PATCH net-next 3/4] net: dsa: make slave close symmetrical to open Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-09-22 18:30 +0200
      Re: [PATCH net-next 3/4] net: dsa: make slave close symmetrical to  open Andrew Lunn <andrew@lunn.ch> - 2017-09-22 18:40 +0200
      Re: [PATCH net-next 3/4] net: dsa: make slave close symmetrical to  open Florian Fainelli <f.fainelli@gmail.com> - 2017-09-22 19:20 +0200

#1737616 — [PATCH net-next 0/4] net: dsa: simplify port enabling

This patchset removes the unnecessary PHY device argument in port
enable/disable switch operations, makes slave open and close symmetrical
and finally provides helpers for enabling or disabling a DSA port.

Vivien Didelot (4):
  net: dsa: move up phy enabling in core
  net: dsa: remove phy arg from port enable/disable
  net: dsa: make slave close symmetrical to open
  net: dsa: add port enable and disable helpers

 drivers/net/dsa/b53/b53_common.c       |  6 +++---
 drivers/net/dsa/b53/b53_priv.h         |  4 ++--
 drivers/net/dsa/bcm_sf2.c              | 32 ++++++++----------------------
 drivers/net/dsa/lan9303-core.c         |  6 ++----
 drivers/net/dsa/microchip/ksz_common.c |  6 ++----
 drivers/net/dsa/mt7530.c               |  8 +++-----
 drivers/net/dsa/mv88e6xxx/chip.c       |  6 ++----
 drivers/net/dsa/qca8k.c                |  6 ++----
 include/net/dsa.h                      |  6 ++----
 net/dsa/dsa_priv.h                     |  3 ++-
 net/dsa/port.c                         | 31 ++++++++++++++++++++++++++++-
 net/dsa/slave.c                        | 36 ++++++++++++++++++----------------
 12 files changed, 77 insertions(+), 73 deletions(-)

-- 
2.14.1

[toc] | [next] | [standalone]

#1737620 — [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable

The .port_enable and .port_disable functions are meant to deal with the
switch ports only, and no driver is using the phy argument anyway.
Remove it.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/b53/b53_common.c       |  6 +++---
 drivers/net/dsa/b53/b53_priv.h         |  4 ++--
 drivers/net/dsa/bcm_sf2.c              | 16 +++++++---------
 drivers/net/dsa/lan9303-core.c         |  6 ++----
 drivers/net/dsa/microchip/ksz_common.c |  6 ++----
 drivers/net/dsa/mt7530.c               |  8 +++-----
 drivers/net/dsa/mv88e6xxx/chip.c       |  6 ++----
 drivers/net/dsa/qca8k.c                |  6 ++----
 include/net/dsa.h                      |  6 ++----
 net/dsa/slave.c                        |  4 ++--
 10 files changed, 27 insertions(+), 41 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index d4ce092def83..e46eb29d29f0 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -502,7 +502,7 @@ void b53_imp_vlan_setup(struct dsa_switch *ds, int cpu_port)
 }
 EXPORT_SYMBOL(b53_imp_vlan_setup);
 
-int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
+int b53_enable_port(struct dsa_switch *ds, int port)
 {
 	struct b53_device *dev = ds->priv;
 	unsigned int cpu_port = dev->cpu_port;
@@ -531,7 +531,7 @@ int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
 }
 EXPORT_SYMBOL(b53_enable_port);
 
-void b53_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
+void b53_disable_port(struct dsa_switch *ds, int port)
 {
 	struct b53_device *dev = ds->priv;
 	u8 reg;
@@ -874,7 +874,7 @@ static int b53_setup(struct dsa_switch *ds)
 		if (dsa_is_cpu_port(ds, port))
 			b53_enable_cpu_port(dev, port);
 		else if (!(BIT(port) & ds->enabled_port_mask))
-			b53_disable_port(ds, port, NULL);
+			b53_disable_port(ds, port);
 	}
 
 	return ret;
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 603c66d240d8..688d02ee6155 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -311,8 +311,8 @@ int b53_mirror_add(struct dsa_switch *ds, int port,
 		   struct dsa_mall_mirror_tc_entry *mirror, bool ingress);
 void b53_mirror_del(struct dsa_switch *ds, int port,
 		    struct dsa_mall_mirror_tc_entry *mirror);
-int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy);
-void b53_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy);
+int b53_enable_port(struct dsa_switch *ds, int port);
+void b53_disable_port(struct dsa_switch *ds, int port);
 void b53_brcm_hdr_setup(struct dsa_switch *ds, int port);
 void b53_eee_enable_set(struct dsa_switch *ds, int port, bool enable);
 int b53_eee_init(struct dsa_switch *ds, int port, struct phy_device *phy);
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index ad96b9725a2c..77e0c43f973b 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -159,8 +159,7 @@ static inline void bcm_sf2_port_intr_disable(struct bcm_sf2_priv *priv,
 	intrl2_1_writel(priv, P_IRQ_MASK(off), INTRL2_CPU_CLEAR);
 }
 
-static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
-			      struct phy_device *phy)
+static int bcm_sf2_port_setup(struct dsa_switch *ds, int port)
 {
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
 	unsigned int i;
@@ -191,11 +190,10 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
 	if (port == priv->moca_port)
 		bcm_sf2_port_intr_enable(priv, port);
 
-	return b53_enable_port(ds, port, phy);
+	return b53_enable_port(ds, port);
 }
 
-static void bcm_sf2_port_disable(struct dsa_switch *ds, int port,
-				 struct phy_device *phy)
+static void bcm_sf2_port_disable(struct dsa_switch *ds, int port)
 {
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
 	u32 off, reg;
@@ -214,7 +212,7 @@ static void bcm_sf2_port_disable(struct dsa_switch *ds, int port,
 	else
 		off = CORE_G_PCTL_PORT(port);
 
-	b53_disable_port(ds, port, phy);
+	b53_disable_port(ds, port);
 
 	/* Power down the port memory */
 	reg = core_readl(priv, CORE_MEM_PSM_VDD_CTRL);
@@ -613,7 +611,7 @@ static int bcm_sf2_sw_suspend(struct dsa_switch *ds)
 	for (port = 0; port < DSA_MAX_PORTS; port++) {
 		if ((1 << port) & ds->enabled_port_mask ||
 		    dsa_is_cpu_port(ds, port))
-			bcm_sf2_port_disable(ds, port, NULL);
+			bcm_sf2_port_disable(ds, port);
 	}
 
 	return 0;
@@ -636,7 +634,7 @@ static int bcm_sf2_sw_resume(struct dsa_switch *ds)
 
 	for (port = 0; port < DSA_MAX_PORTS; port++) {
 		if ((1 << port) & ds->enabled_port_mask)
-			bcm_sf2_port_setup(ds, port, NULL);
+			bcm_sf2_port_setup(ds, port);
 		else if (dsa_is_cpu_port(ds, port))
 			bcm_sf2_imp_setup(ds, port);
 	}
@@ -745,7 +743,7 @@ static int bcm_sf2_sw_setup(struct dsa_switch *ds)
 		if (dsa_is_cpu_port(ds, port))
 			bcm_sf2_imp_setup(ds, port);
 		else if (!((1 << port) & ds->enabled_port_mask))
-			bcm_sf2_port_disable(ds, port, NULL);
+			bcm_sf2_port_disable(ds, port);
 	}
 
 	bcm_sf2_sw_configure_vlan(ds);
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 07355db2ad81..0c33b02562dc 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -799,8 +799,7 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
 	}
 }
 
-static int lan9303_port_enable(struct dsa_switch *ds, int port,
-			       struct phy_device *phy)
+static int lan9303_port_enable(struct dsa_switch *ds, int port)
 {
 	struct lan9303 *chip = ds->priv;
 
@@ -817,8 +816,7 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
 	return -ENODEV;
 }
 
-static void lan9303_port_disable(struct dsa_switch *ds, int port,
-				 struct phy_device *phy)
+static void lan9303_port_disable(struct dsa_switch *ds, int port)
 {
 	struct lan9303 *chip = ds->priv;
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 56cd6d365352..4095c50ae111 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -418,8 +418,7 @@ static int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val)
 	return 0;
 }
 
-static int ksz_enable_port(struct dsa_switch *ds, int port,
-			   struct phy_device *phy)
+static int ksz_enable_port(struct dsa_switch *ds, int port)
 {
 	struct ksz_device *dev = ds->priv;
 
@@ -429,8 +428,7 @@ static int ksz_enable_port(struct dsa_switch *ds, int port,
 	return 0;
 }
 
-static void ksz_disable_port(struct dsa_switch *ds, int port,
-			     struct phy_device *phy)
+static void ksz_disable_port(struct dsa_switch *ds, int port)
 {
 	struct ksz_device *dev = ds->priv;
 
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index faa3b88d2206..0a7f6209767f 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -693,8 +693,7 @@ mt7530_cpu_port_enable(struct mt7530_priv *priv,
 }
 
 static int
-mt7530_port_enable(struct dsa_switch *ds, int port,
-		   struct phy_device *phy)
+mt7530_port_enable(struct dsa_switch *ds, int port)
 {
 	struct mt7530_priv *priv = ds->priv;
 
@@ -719,8 +718,7 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
 }
 
 static void
-mt7530_port_disable(struct dsa_switch *ds, int port,
-		    struct phy_device *phy)
+mt7530_port_disable(struct dsa_switch *ds, int port)
 {
 	struct mt7530_priv *priv = ds->priv;
 
@@ -1006,7 +1004,7 @@ mt7530_setup(struct dsa_switch *ds)
 		if (dsa_is_cpu_port(ds, i))
 			mt7530_cpu_port_enable(priv, i);
 		else
-			mt7530_port_disable(ds, i, NULL);
+			mt7530_port_disable(ds, i);
 	}
 
 	/* Flush the FDB table */
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c6678aa9b4ef..e47898fb7dbc 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1862,8 +1862,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_DEFAULT_VLAN, 0);
 }
 
-static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port,
-				 struct phy_device *phydev)
+static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int err;
@@ -1875,8 +1874,7 @@ static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port,
 	return err;
 }
 
-static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port,
-				   struct phy_device *phydev)
+static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 82f09711ac1a..622ee9b8e72b 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -743,8 +743,7 @@ qca8k_port_bridge_leave(struct dsa_switch *ds, int port, struct net_device *br)
 }
 
 static int
-qca8k_port_enable(struct dsa_switch *ds, int port,
-		  struct phy_device *phy)
+qca8k_port_enable(struct dsa_switch *ds, int port)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 
@@ -755,8 +754,7 @@ qca8k_port_enable(struct dsa_switch *ds, int port,
 }
 
 static void
-qca8k_port_disable(struct dsa_switch *ds, int port,
-		   struct phy_device *phy)
+qca8k_port_disable(struct dsa_switch *ds, int port)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 8dee216a5a9b..65b031a69c19 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -337,10 +337,8 @@ struct dsa_switch_ops {
 	/*
 	 * Port enable/disable
 	 */
-	int	(*port_enable)(struct dsa_switch *ds, int port,
-			       struct phy_device *phy);
-	void	(*port_disable)(struct dsa_switch *ds, int port,
-				struct phy_device *phy);
+	int	(*port_enable)(struct dsa_switch *ds, int port);
+	void	(*port_disable)(struct dsa_switch *ds, int port);
 
 	/*
 	 * Port's MAC EEE settings
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 606812160fd5..6290741e496a 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -100,7 +100,7 @@ static int dsa_slave_open(struct net_device *dev)
 	}
 
 	if (ds->ops->port_enable) {
-		err = ds->ops->port_enable(ds, p->dp->index, p->phy);
+		err = ds->ops->port_enable(ds, p->dp->index);
 		if (err)
 			goto clear_promisc;
 	}
@@ -155,7 +155,7 @@ static int dsa_slave_close(struct net_device *dev)
 		dev_uc_del(master, dev->dev_addr);
 
 	if (ds->ops->port_disable)
-		ds->ops->port_disable(ds, p->dp->index, p->phy);
+		ds->ops->port_disable(ds, p->dp->index);
 
 	dsa_port_set_state_now(p->dp, BR_STATE_DISABLED);
 
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1737702 — Re: [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable

On 09/22/2017 09:17 AM, Vivien Didelot wrote:
> The .port_enable and .port_disable functions are meant to deal with the
> switch ports only, and no driver is using the phy argument anyway.
> Remove it.

I don't think this makes sense, there are perfectly legit reasons why a
switch driver may have something to do with the PHY device attached to
its per-port network interface, we should definitively keep that around,
unless you think we should be accessing the PHY within the switch
drivers by doing:

struct phy_device *phydev = ds->ports[port].netdev->phydev?

> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  drivers/net/dsa/b53/b53_common.c       |  6 +++---
>  drivers/net/dsa/b53/b53_priv.h         |  4 ++--
>  drivers/net/dsa/bcm_sf2.c              | 16 +++++++---------
>  drivers/net/dsa/lan9303-core.c         |  6 ++----
>  drivers/net/dsa/microchip/ksz_common.c |  6 ++----
>  drivers/net/dsa/mt7530.c               |  8 +++-----
>  drivers/net/dsa/mv88e6xxx/chip.c       |  6 ++----
>  drivers/net/dsa/qca8k.c                |  6 ++----
>  include/net/dsa.h                      |  6 ++----
>  net/dsa/slave.c                        |  4 ++--
>  10 files changed, 27 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index d4ce092def83..e46eb29d29f0 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -502,7 +502,7 @@ void b53_imp_vlan_setup(struct dsa_switch *ds, int cpu_port)
>  }
>  EXPORT_SYMBOL(b53_imp_vlan_setup);
>  
> -int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
> +int b53_enable_port(struct dsa_switch *ds, int port)
>  {
>  	struct b53_device *dev = ds->priv;
>  	unsigned int cpu_port = dev->cpu_port;
> @@ -531,7 +531,7 @@ int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
>  }
>  EXPORT_SYMBOL(b53_enable_port);
>  
> -void b53_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
> +void b53_disable_port(struct dsa_switch *ds, int port)
>  {
>  	struct b53_device *dev = ds->priv;
>  	u8 reg;
> @@ -874,7 +874,7 @@ static int b53_setup(struct dsa_switch *ds)
>  		if (dsa_is_cpu_port(ds, port))
>  			b53_enable_cpu_port(dev, port);
>  		else if (!(BIT(port) & ds->enabled_port_mask))
> -			b53_disable_port(ds, port, NULL);
> +			b53_disable_port(ds, port);
>  	}
>  
>  	return ret;
> diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
> index 603c66d240d8..688d02ee6155 100644
> --- a/drivers/net/dsa/b53/b53_priv.h
> +++ b/drivers/net/dsa/b53/b53_priv.h
> @@ -311,8 +311,8 @@ int b53_mirror_add(struct dsa_switch *ds, int port,
>  		   struct dsa_mall_mirror_tc_entry *mirror, bool ingress);
>  void b53_mirror_del(struct dsa_switch *ds, int port,
>  		    struct dsa_mall_mirror_tc_entry *mirror);
> -int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy);
> -void b53_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy);
> +int b53_enable_port(struct dsa_switch *ds, int port);
> +void b53_disable_port(struct dsa_switch *ds, int port);
>  void b53_brcm_hdr_setup(struct dsa_switch *ds, int port);
>  void b53_eee_enable_set(struct dsa_switch *ds, int port, bool enable);
>  int b53_eee_init(struct dsa_switch *ds, int port, struct phy_device *phy);
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index ad96b9725a2c..77e0c43f973b 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -159,8 +159,7 @@ static inline void bcm_sf2_port_intr_disable(struct bcm_sf2_priv *priv,
>  	intrl2_1_writel(priv, P_IRQ_MASK(off), INTRL2_CPU_CLEAR);
>  }
>  
> -static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
> -			      struct phy_device *phy)
> +static int bcm_sf2_port_setup(struct dsa_switch *ds, int port)
>  {
>  	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
>  	unsigned int i;
> @@ -191,11 +190,10 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
>  	if (port == priv->moca_port)
>  		bcm_sf2_port_intr_enable(priv, port);
>  
> -	return b53_enable_port(ds, port, phy);
> +	return b53_enable_port(ds, port);
>  }
>  
> -static void bcm_sf2_port_disable(struct dsa_switch *ds, int port,
> -				 struct phy_device *phy)
> +static void bcm_sf2_port_disable(struct dsa_switch *ds, int port)
>  {
>  	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
>  	u32 off, reg;
> @@ -214,7 +212,7 @@ static void bcm_sf2_port_disable(struct dsa_switch *ds, int port,
>  	else
>  		off = CORE_G_PCTL_PORT(port);
>  
> -	b53_disable_port(ds, port, phy);
> +	b53_disable_port(ds, port);
>  
>  	/* Power down the port memory */
>  	reg = core_readl(priv, CORE_MEM_PSM_VDD_CTRL);
> @@ -613,7 +611,7 @@ static int bcm_sf2_sw_suspend(struct dsa_switch *ds)
>  	for (port = 0; port < DSA_MAX_PORTS; port++) {
>  		if ((1 << port) & ds->enabled_port_mask ||
>  		    dsa_is_cpu_port(ds, port))
> -			bcm_sf2_port_disable(ds, port, NULL);
> +			bcm_sf2_port_disable(ds, port);
>  	}
>  
>  	return 0;
> @@ -636,7 +634,7 @@ static int bcm_sf2_sw_resume(struct dsa_switch *ds)
>  
>  	for (port = 0; port < DSA_MAX_PORTS; port++) {
>  		if ((1 << port) & ds->enabled_port_mask)
> -			bcm_sf2_port_setup(ds, port, NULL);
> +			bcm_sf2_port_setup(ds, port);
>  		else if (dsa_is_cpu_port(ds, port))
>  			bcm_sf2_imp_setup(ds, port);
>  	}
> @@ -745,7 +743,7 @@ static int bcm_sf2_sw_setup(struct dsa_switch *ds)
>  		if (dsa_is_cpu_port(ds, port))
>  			bcm_sf2_imp_setup(ds, port);
>  		else if (!((1 << port) & ds->enabled_port_mask))
> -			bcm_sf2_port_disable(ds, port, NULL);
> +			bcm_sf2_port_disable(ds, port);
>  	}
>  
>  	bcm_sf2_sw_configure_vlan(ds);
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 07355db2ad81..0c33b02562dc 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -799,8 +799,7 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
>  	}
>  }
>  
> -static int lan9303_port_enable(struct dsa_switch *ds, int port,
> -			       struct phy_device *phy)
> +static int lan9303_port_enable(struct dsa_switch *ds, int port)
>  {
>  	struct lan9303 *chip = ds->priv;
>  
> @@ -817,8 +816,7 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
>  	return -ENODEV;
>  }
>  
> -static void lan9303_port_disable(struct dsa_switch *ds, int port,
> -				 struct phy_device *phy)
> +static void lan9303_port_disable(struct dsa_switch *ds, int port)
>  {
>  	struct lan9303 *chip = ds->priv;
>  
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 56cd6d365352..4095c50ae111 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -418,8 +418,7 @@ static int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val)
>  	return 0;
>  }
>  
> -static int ksz_enable_port(struct dsa_switch *ds, int port,
> -			   struct phy_device *phy)
> +static int ksz_enable_port(struct dsa_switch *ds, int port)
>  {
>  	struct ksz_device *dev = ds->priv;
>  
> @@ -429,8 +428,7 @@ static int ksz_enable_port(struct dsa_switch *ds, int port,
>  	return 0;
>  }
>  
> -static void ksz_disable_port(struct dsa_switch *ds, int port,
> -			     struct phy_device *phy)
> +static void ksz_disable_port(struct dsa_switch *ds, int port)
>  {
>  	struct ksz_device *dev = ds->priv;
>  
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index faa3b88d2206..0a7f6209767f 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -693,8 +693,7 @@ mt7530_cpu_port_enable(struct mt7530_priv *priv,
>  }
>  
>  static int
> -mt7530_port_enable(struct dsa_switch *ds, int port,
> -		   struct phy_device *phy)
> +mt7530_port_enable(struct dsa_switch *ds, int port)
>  {
>  	struct mt7530_priv *priv = ds->priv;
>  
> @@ -719,8 +718,7 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
>  }
>  
>  static void
> -mt7530_port_disable(struct dsa_switch *ds, int port,
> -		    struct phy_device *phy)
> +mt7530_port_disable(struct dsa_switch *ds, int port)
>  {
>  	struct mt7530_priv *priv = ds->priv;
>  
> @@ -1006,7 +1004,7 @@ mt7530_setup(struct dsa_switch *ds)
>  		if (dsa_is_cpu_port(ds, i))
>  			mt7530_cpu_port_enable(priv, i);
>  		else
> -			mt7530_port_disable(ds, i, NULL);
> +			mt7530_port_disable(ds, i);
>  	}
>  
>  	/* Flush the FDB table */
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index c6678aa9b4ef..e47898fb7dbc 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1862,8 +1862,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
>  	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_DEFAULT_VLAN, 0);
>  }
>  
> -static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port,
> -				 struct phy_device *phydev)
> +static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port)
>  {
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  	int err;
> @@ -1875,8 +1874,7 @@ static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port,
>  	return err;
>  }
>  
> -static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port,
> -				   struct phy_device *phydev)
> +static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port)
>  {
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 82f09711ac1a..622ee9b8e72b 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -743,8 +743,7 @@ qca8k_port_bridge_leave(struct dsa_switch *ds, int port, struct net_device *br)
>  }
>  
>  static int
> -qca8k_port_enable(struct dsa_switch *ds, int port,
> -		  struct phy_device *phy)
> +qca8k_port_enable(struct dsa_switch *ds, int port)
>  {
>  	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
>  
> @@ -755,8 +754,7 @@ qca8k_port_enable(struct dsa_switch *ds, int port,
>  }
>  
>  static void
> -qca8k_port_disable(struct dsa_switch *ds, int port,
> -		   struct phy_device *phy)
> +qca8k_port_disable(struct dsa_switch *ds, int port)
>  {
>  	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
>  
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 8dee216a5a9b..65b031a69c19 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -337,10 +337,8 @@ struct dsa_switch_ops {
>  	/*
>  	 * Port enable/disable
>  	 */
> -	int	(*port_enable)(struct dsa_switch *ds, int port,
> -			       struct phy_device *phy);
> -	void	(*port_disable)(struct dsa_switch *ds, int port,
> -				struct phy_device *phy);
> +	int	(*port_enable)(struct dsa_switch *ds, int port);
> +	void	(*port_disable)(struct dsa_switch *ds, int port);
>  
>  	/*
>  	 * Port's MAC EEE settings
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 606812160fd5..6290741e496a 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -100,7 +100,7 @@ static int dsa_slave_open(struct net_device *dev)
>  	}
>  
>  	if (ds->ops->port_enable) {
> -		err = ds->ops->port_enable(ds, p->dp->index, p->phy);
> +		err = ds->ops->port_enable(ds, p->dp->index);
>  		if (err)
>  			goto clear_promisc;
>  	}
> @@ -155,7 +155,7 @@ static int dsa_slave_close(struct net_device *dev)
>  		dev_uc_del(master, dev->dev_addr);
>  
>  	if (ds->ops->port_disable)
> -		ds->ops->port_disable(ds, p->dp->index, p->phy);
> +		ds->ops->port_disable(ds, p->dp->index);
>  
>  	dsa_port_set_state_now(p->dp, BR_STATE_DISABLED);
>  
> 


-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1737723 — Re: [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable

Hi Florian,

Florian Fainelli <f.fainelli@gmail.com> writes:

> On 09/22/2017 09:17 AM, Vivien Didelot wrote:
>> The .port_enable and .port_disable functions are meant to deal with the
>> switch ports only, and no driver is using the phy argument anyway.
>> Remove it.
>
> I don't think this makes sense, there are perfectly legit reasons why a
> switch driver may have something to do with the PHY device attached to
> its per-port network interface, we should definitively keep that around,
> unless you think we should be accessing the PHY within the switch
> drivers by doing:
>
> struct phy_device *phydev = ds->ports[port].netdev->phydev?

bcm_sf2 is the only user for this phy argument right now. The reason I'm
doing this is because I prefer to discourage switch drivers to dig into
the phy device themselves while as you said there must be a cleaner
solution. This must be handled somehow elsewhere in the stack.

In the meantime, moving the PHY device up to the dsa_port structure is a
good solution, in order not to expose it in switch ops, but still make
it available to more complex drivers.

Do you know if netdev->phydev is usable? Why do DSA has its own copy in
dsa_slave_priv then?


I'll respin, thanks.

    Vivien

[toc] | [prev] | [next] | [standalone]

#1737726 — Re: [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable

On 09/22/2017 11:12 AM, Vivien Didelot wrote:
> Hi Florian,
> 
> Florian Fainelli <f.fainelli@gmail.com> writes:
> 
>> On 09/22/2017 09:17 AM, Vivien Didelot wrote:
>>> The .port_enable and .port_disable functions are meant to deal with the
>>> switch ports only, and no driver is using the phy argument anyway.
>>> Remove it.
>>
>> I don't think this makes sense, there are perfectly legit reasons why a
>> switch driver may have something to do with the PHY device attached to
>> its per-port network interface, we should definitively keep that around,
>> unless you think we should be accessing the PHY within the switch
>> drivers by doing:
>>
>> struct phy_device *phydev = ds->ports[port].netdev->phydev?
> 
> bcm_sf2 is the only user for this phy argument right now. The reason I'm
> doing this is because I prefer to discourage switch drivers to dig into
> the phy device themselves while as you said there must be a cleaner
> solution. This must be handled somehow elsewhere in the stack.

The current approach of passing the phy_device reference as an argument
is certainly a cleaner way then. The port_enable caller can provide the
correct phy_device and that lifts the switch driver from having to dig
it itself from its per-port netdev.

> 
> In the meantime, moving the PHY device up to the dsa_port structure is a
> good solution, in order not to expose it in switch ops, but still make
> it available to more complex drivers.
> 
> Do you know if netdev->phydev is usable? Why do DSA has its own copy in
> dsa_slave_priv then?

Historical reasons mostly. Considering the complexity of
dsa_slave_phy_setup(), I would certainly be extremely careful in
changing any of this, the potential for breakage is pretty big. At first
glance, I would say that this is a safe conversion to do, and I can test
this on the HW I have here anyway.
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1737770 — Re: [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable

> Historical reasons mostly. Considering the complexity of
> dsa_slave_phy_setup(), I would certainly be extremely careful in
> changing any of this, the potential for breakage is pretty big.

Yes, i took a look at this, wondering how to convert to phylink. I
went away and got a stiff drink :-)

     Andrew

[toc] | [prev] | [next] | [standalone]

#1737622 — [PATCH net-next 3/4] net: dsa: make slave close symmetrical to open

The DSA slave open function configures the unicast MAC addresses on the
master device, enable the switch port, change its STP state, then start
the PHY device.

Make the close function symmetric, by first stopping the PHY device,
then changing the STP state, disabling the switch port and restore the
master device.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/slave.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 6290741e496a..235a5c95dfcc 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -144,6 +144,11 @@ static int dsa_slave_close(struct net_device *dev)
 	if (p->phy)
 		phy_stop(p->phy);
 
+	dsa_port_set_state_now(p->dp, BR_STATE_DISABLED);
+
+	if (ds->ops->port_disable)
+		ds->ops->port_disable(ds, p->dp->index);
+
 	dev_mc_unsync(master, dev);
 	dev_uc_unsync(master, dev);
 	if (dev->flags & IFF_ALLMULTI)
@@ -154,11 +159,6 @@ static int dsa_slave_close(struct net_device *dev)
 	if (!ether_addr_equal(dev->dev_addr, master->dev_addr))
 		dev_uc_del(master, dev->dev_addr);
 
-	if (ds->ops->port_disable)
-		ds->ops->port_disable(ds, p->dp->index);
-
-	dsa_port_set_state_now(p->dp, BR_STATE_DISABLED);
-
 	return 0;
 }
 
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1737635 — Re: [PATCH net-next 3/4] net: dsa: make slave close symmetrical to open

On Fri, Sep 22, 2017 at 12:17:52PM -0400, Vivien Didelot wrote:
> The DSA slave open function configures the unicast MAC addresses on the
> master device, enable the switch port, change its STP state, then start
> the PHY device.
> 
> Make the close function symmetric, by first stopping the PHY device,
> then changing the STP state, disabling the switch port and restore the
> master device.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

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

    Andrew

[toc] | [prev] | [next] | [standalone]

#1737700 — Re: [PATCH net-next 3/4] net: dsa: make slave close symmetrical to open

On 09/22/2017 09:17 AM, Vivien Didelot wrote:
> The DSA slave open function configures the unicast MAC addresses on the
> master device, enable the switch port, change its STP state, then start
> the PHY device.
> 
> Make the close function symmetric, by first stopping the PHY device,
> then changing the STP state, disabling the switch port and restore the
> master device.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web