[PATCH 0/4] Fix MV88E6131 tagging

[PATCH 0/4] Fix MV88E6131 tagging

[Andrew Lunn]

Marvell has two different tagging protocols for frames passed to a
swicth. There is the older DSA and the newer EDSA. Somewhere along the
way, we broke support for switches which only support DSA, by trying
to configure them to use EDSA. These patches add back support for
switches which only support DSA, by allowing the drivers to
dynamically indicate the tagging protocol they support to the DSA
core. This needs to be dynamic since the mv88e6xxx has to support two
protocols.

Thanks go to Jamie Lentin for reporting the problem, helping debug it,
providing some of the fix, and testing.

Andrew Lunn (3):
  net: dsa: Allow the DSA driver to indicate the tag protocol
  net: dsa: mv88e6xxx: Fix support for DSA tagging for older switches.
  dsa: mv88e6xxx: Delete ppu timer when removing module

Jamie Lentin (1):
  net: mv88e6xxx: Enable PORT_CONTROL_FORWARD_UNKNOWN for DSA-tagged CPU
    ports

 drivers/net/dsa/b53/b53_common.c      |  7 +++-
 drivers/net/dsa/bcm_sf2.c             |  7 +++-
 drivers/net/dsa/mv88e6060.c           |  7 +++-
 drivers/net/dsa/mv88e6xxx/Kconfig     |  1 +
 drivers/net/dsa/mv88e6xxx/chip.c      | 61 +++++++++++++++++++++--------------
 drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 16 +++++++--
 include/net/dsa.h                     |  5 +--
 net/dsa/dsa.c                         |  5 ++-
 net/dsa/dsa2.c                        |  4 ++-
 9 files changed, 78 insertions(+), 35 deletions(-)

-- 
2.8.1

[PATCH 1/4] net: dsa: Allow the DSA driver to indicate the tag protocol

[Andrew Lunn]

DSA drivers may drive different families of switches which need
different tag protocol. Rather than hard code the tag protocol in the
driver structure, have a callback for the DSA core to call.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/b53/b53_common.c | 7 ++++++-
 drivers/net/dsa/bcm_sf2.c        | 7 ++++++-
 drivers/net/dsa/mv88e6060.c      | 7 ++++++-
 drivers/net/dsa/mv88e6xxx/chip.c | 7 ++++++-
 include/net/dsa.h                | 5 +++--
 net/dsa/dsa.c                    | 5 ++++-
 net/dsa/dsa2.c                   | 4 +++-
 7 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 38ee10de7884..65ecb51f99e5 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1373,8 +1373,13 @@ static void b53_br_set_stp_state(struct dsa_switch *ds, int port,
 	b53_write8(dev, B53_CTRL_PAGE, B53_PORT_CTRL(port), reg);
 }
 
+static enum dsa_tag_protocol b53_get_tag_protocol(struct dsa_switch *ds)
+{
+	return DSA_TAG_PROTO_NONE;
+}
+
 static struct dsa_switch_driver b53_switch_ops = {
-	.tag_protocol		= DSA_TAG_PROTO_NONE,
+	.get_tag_protocol	= b53_get_tag_protocol,
 	.setup			= b53_setup,
 	.set_addr		= b53_set_addr,
 	.get_strings		= b53_get_strings,
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 8e6fe13dbec3..b47a74b37a42 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -136,6 +136,11 @@ static int bcm_sf2_sw_get_sset_count(struct dsa_switch *ds)
 	return BCM_SF2_STATS_SIZE;
 }
 
+static enum dsa_tag_protocol bcm_sf2_sw_get_tag_protocol(struct dsa_switch *ds)
+{
+	return DSA_TAG_PROTO_BRCM;
+}
+
 static void bcm_sf2_imp_vlan_setup(struct dsa_switch *ds, int cpu_port)
 {
 	struct bcm_sf2_priv *priv = ds_to_priv(ds);
@@ -1577,8 +1582,8 @@ static int bcm_sf2_sw_setup(struct dsa_switch *ds)
 }
 
 static struct dsa_switch_driver bcm_sf2_switch_driver = {
-	.tag_protocol		= DSA_TAG_PROTO_BRCM,
 	.setup			= bcm_sf2_sw_setup,
+	.get_tag_protocol	= bcm_sf2_sw_get_tag_protocol,
 	.set_addr		= bcm_sf2_sw_set_addr,
 	.get_phy_flags		= bcm_sf2_sw_get_phy_flags,
 	.get_strings		= bcm_sf2_sw_get_strings,
diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
index e36b40886bd8..1fdfbf3a50bc 100644
--- a/drivers/net/dsa/mv88e6060.c
+++ b/drivers/net/dsa/mv88e6060.c
@@ -69,6 +69,11 @@ static const char *mv88e6060_get_name(struct mii_bus *bus, int sw_addr)
 	return NULL;
 }
 
+static enum dsa_tag_protocol mv88e6060_get_tag_protocol(struct dsa_switch *ds)
+{
+	return DSA_TAG_PROTO_TRAILER;
+}
+
 static const char *mv88e6060_drv_probe(struct device *dsa_dev,
 				       struct device *host_dev, int sw_addr,
 				       void **_priv)
@@ -248,7 +253,7 @@ mv88e6060_phy_write(struct dsa_switch *ds, int port, int regnum, u16 val)
 }
 
 static struct dsa_switch_driver mv88e6060_switch_driver = {
-	.tag_protocol	= DSA_TAG_PROTO_TRAILER,
+	.get_tag_protocol = mv88e6060_get_tag_protocol,
 	.probe		= mv88e6060_drv_probe,
 	.setup		= mv88e6060_setup,
 	.set_addr	= mv88e6060_set_addr,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 014b52bd72f1..63cad6c00bc7 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3924,6 +3924,11 @@ static int mv88e6xxx_smi_init(struct mv88e6xxx_chip *chip,
 	return 0;
 }
 
+static enum dsa_tag_protocol mv88e6xxx_get_tag_protocol(struct dsa_switch *ds)
+{
+	return DSA_TAG_PROTO_EDSA;
+}
+
 static const char *mv88e6xxx_drv_probe(struct device *dsa_dev,
 				       struct device *host_dev, int sw_addr,
 				       void **priv)
@@ -3967,8 +3972,8 @@ free:
 }
 
 static struct dsa_switch_driver mv88e6xxx_switch_driver = {
-	.tag_protocol		= DSA_TAG_PROTO_EDSA,
 	.probe			= mv88e6xxx_drv_probe,
+	.get_tag_protocol	= mv88e6xxx_get_tag_protocol,
 	.setup			= mv88e6xxx_setup,
 	.set_addr		= mv88e6xxx_set_addr,
 	.adjust_link		= mv88e6xxx_adjust_link,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index d00c392bc9f8..8ca2684c5358 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -239,14 +239,15 @@ struct switchdev_obj_port_vlan;
 struct dsa_switch_driver {
 	struct list_head	list;
 
-	enum dsa_tag_protocol	tag_protocol;
-
 	/*
 	 * Probing and setup.
 	 */
 	const char	*(*probe)(struct device *dsa_dev,
 				  struct device *host_dev, int sw_addr,
 				  void **priv);
+
+	enum dsa_tag_protocol (*get_tag_protocol)(struct dsa_switch *ds);
+
 	int	(*setup)(struct dsa_switch *ds);
 	int	(*set_addr)(struct dsa_switch *ds, u8 *addr);
 	u32	(*get_phy_flags)(struct dsa_switch *ds, int port);
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 8bda74e595a5..8d3a28d4e99d 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -354,7 +354,10 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 	 * switch.
 	 */
 	if (dst->cpu_switch == index) {
-		dst->tag_ops = dsa_resolve_tag_protocol(drv->tag_protocol);
+		enum dsa_tag_protocol tag_protocol;
+
+		tag_protocol = drv->get_tag_protocol(ds);
+		dst->tag_ops = dsa_resolve_tag_protocol(tag_protocol);
 		if (IS_ERR(dst->tag_ops)) {
 			ret = PTR_ERR(dst->tag_ops);
 			goto out;
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index f30bad9678f0..2e343221464c 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -443,6 +443,7 @@ static int dsa_cpu_parse(struct device_node *port, u32 index,
 			 struct dsa_switch_tree *dst,
 			 struct dsa_switch *ds)
 {
+	enum dsa_tag_protocol tag_protocol;
 	struct net_device *ethernet_dev;
 	struct device_node *ethernet;
 
@@ -465,7 +466,8 @@ static int dsa_cpu_parse(struct device_node *port, u32 index,
 		dst->cpu_port = index;
 	}
 
-	dst->tag_ops = dsa_resolve_tag_protocol(ds->drv->tag_protocol);
+	tag_protocol = ds->drv->get_tag_protocol(ds);
+	dst->tag_ops = dsa_resolve_tag_protocol(tag_protocol);
 	if (IS_ERR(dst->tag_ops)) {
 		dev_warn(ds->dev, "No tagger for this switch\n");
 		return PTR_ERR(dst->tag_ops);
-- 
2.8.1

Re: [PATCH 1/4] net: dsa: Allow the DSA driver to indicate the tag protocol

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> DSA drivers may drive different families of switches which need
> different tag protocol. Rather than hard code the tag protocol in the
> driver structure, have a callback for the DSA core to call.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Great, this DSA structure finally becomes operation-only.

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Thanks,

        Vivien

[PATCH 2/4] net: dsa: mv88e6xxx: Fix support for DSA tagging for older switches.

[Andrew Lunn]

Older chips only support DSA tagging on the CPU port. New devices
support both DSA and EDSA. The driver needs to tell the core the tag
protocol to use, and configure the switch for what is available.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/Kconfig     |  1 +
 drivers/net/dsa/mv88e6xxx/chip.c      | 41 +++++++++++++++--------------------
 drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 16 +++++++++++---
 3 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/Kconfig b/drivers/net/dsa/mv88e6xxx/Kconfig
index 490bc06f993e..ac77737bbd87 100644
--- a/drivers/net/dsa/mv88e6xxx/Kconfig
+++ b/drivers/net/dsa/mv88e6xxx/Kconfig
@@ -2,6 +2,7 @@ config NET_DSA_MV88E6XXX
 	tristate "Marvell 88E6xxx Ethernet switch fabric support"
 	depends on NET_DSA
 	select NET_DSA_TAG_EDSA
+	select NET_DSA_TAG_DSA
 	help
 	  This driver adds support for most of the Marvell 88E6xxx models of
 	  Ethernet switch chips, except 88E6060.
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 63cad6c00bc7..b315769aa5be 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2483,28 +2483,13 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 		PORT_CONTROL_USE_TAG | PORT_CONTROL_USE_IP |
 		PORT_CONTROL_STATE_FORWARDING;
 	if (dsa_is_cpu_port(ds, port)) {
-		if (mv88e6xxx_6095_family(chip) || mv88e6xxx_6185_family(chip))
-			reg |= PORT_CONTROL_DSA_TAG;
-		if (mv88e6xxx_6352_family(chip) ||
-		    mv88e6xxx_6351_family(chip) ||
-		    mv88e6xxx_6165_family(chip) ||
-		    mv88e6xxx_6097_family(chip) ||
-		    mv88e6xxx_6320_family(chip)) {
+		if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_EDSA))
 			reg |= PORT_CONTROL_FRAME_ETHER_TYPE_DSA |
 				PORT_CONTROL_FORWARD_UNKNOWN |
 				PORT_CONTROL_FORWARD_UNKNOWN_MC;
-		}
-
-		if (mv88e6xxx_6352_family(chip) ||
-		    mv88e6xxx_6351_family(chip) ||
-		    mv88e6xxx_6165_family(chip) ||
-		    mv88e6xxx_6097_family(chip) ||
-		    mv88e6xxx_6095_family(chip) ||
-		    mv88e6xxx_6065_family(chip) ||
-		    mv88e6xxx_6185_family(chip) ||
-		    mv88e6xxx_6320_family(chip)) {
-			reg |= PORT_CONTROL_EGRESS_ADD_TAG;
-		}
+		else
+			reg |= PORT_CONTROL_DSA_TAG;
+		reg |= PORT_CONTROL_EGRESS_ADD_TAG;
 	}
 	if (dsa_is_dsa_port(ds, port)) {
 		if (mv88e6xxx_6095_family(chip) ||
@@ -2632,10 +2617,13 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 		/* Port Ethertype: use the Ethertype DSA Ethertype
 		 * value.
 		 */
-		ret = _mv88e6xxx_reg_write(chip, REG_PORT(port),
-					   PORT_ETH_TYPE, ETH_P_EDSA);
-		if (ret)
-			return ret;
+		if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_EDSA)) {
+			ret = _mv88e6xxx_reg_write(chip, REG_PORT(port),
+						   PORT_ETH_TYPE, ETH_P_EDSA);
+			if (ret)
+				return ret;
+		}
+
 		/* Tag Remap: use an identity 802.1p prio -> switch
 		 * prio mapping.
 		 */
@@ -3926,7 +3914,12 @@ static int mv88e6xxx_smi_init(struct mv88e6xxx_chip *chip,
 
 static enum dsa_tag_protocol mv88e6xxx_get_tag_protocol(struct dsa_switch *ds)
 {
-	return DSA_TAG_PROTO_EDSA;
+	struct mv88e6xxx_chip *chip = ds_to_priv(ds);
+
+	if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_EDSA))
+		return DSA_TAG_PROTO_EDSA;
+
+	return DSA_TAG_PROTO_DSA;
 }
 
 static const char *mv88e6xxx_drv_probe(struct device *dsa_dev,
diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
index 1f9bab5891b1..e157d4f69864 100644
--- a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
@@ -386,6 +386,12 @@ enum mv88e6xxx_family {
 };
 
 enum mv88e6xxx_cap {
+	/* Two different tag protocols can be used by the driver. All
+	 * switches support DSA, but only later generations support
+	 * EDSA.
+	 */
+	MV88E6XXX_CAP_EDSA,
+
 	/* Energy Efficient Ethernet.
 	 */
 	MV88E6XXX_CAP_EEE,
@@ -447,6 +453,7 @@ enum mv88e6xxx_cap {
 };
 
 /* Bitmask of capabilities */
+#define MV88E6XXX_FLAG_EDSA		BIT(MV88E6XXX_CAP_EDSA)
 #define MV88E6XXX_FLAG_EEE		BIT(MV88E6XXX_CAP_EEE)
 
 #define MV88E6XXX_FLAG_SMI_CMD		BIT(MV88E6XXX_CAP_SMI_CMD)
@@ -547,7 +554,8 @@ enum mv88e6xxx_cap {
 	 MV88E6XXX_FLAG_VTU)
 
 #define MV88E6XXX_FLAGS_FAMILY_6320	\
-	(MV88E6XXX_FLAG_EEE |		\
+	(MV88E6XXX_FLAG_EDSA |		\
+	 MV88E6XXX_FLAG_EEE |		\
 	 MV88E6XXX_FLAG_GLOBAL2 |	\
 	 MV88E6XXX_FLAG_G2_MGMT_EN_2X |	\
 	 MV88E6XXX_FLAG_G2_MGMT_EN_0X |	\
@@ -564,7 +572,8 @@ enum mv88e6xxx_cap {
 	 MV88E6XXX_FLAGS_SMI_PHY)
 
 #define MV88E6XXX_FLAGS_FAMILY_6351	\
-	(MV88E6XXX_FLAG_GLOBAL2 |	\
+	(MV88E6XXX_FLAG_EDSA |		\
+	 MV88E6XXX_FLAG_GLOBAL2 |	\
 	 MV88E6XXX_FLAG_G2_MGMT_EN_2X |	\
 	 MV88E6XXX_FLAG_G2_MGMT_EN_0X |	\
 	 MV88E6XXX_FLAG_G2_SWITCH_MAC |	\
@@ -579,7 +588,8 @@ enum mv88e6xxx_cap {
 	 MV88E6XXX_FLAGS_SMI_PHY)
 
 #define MV88E6XXX_FLAGS_FAMILY_6352	\
-	(MV88E6XXX_FLAG_EEE |		\
+	(MV88E6XXX_FLAG_EDSA |		\
+	 MV88E6XXX_FLAG_EEE |		\
 	 MV88E6XXX_FLAG_GLOBAL2 |	\
 	 MV88E6XXX_FLAG_G2_MGMT_EN_2X |	\
 	 MV88E6XXX_FLAG_G2_MGMT_EN_0X |	\
-- 
2.8.1

Re: [PATCH 2/4] net: dsa: mv88e6xxx: Fix support for DSA tagging for older switches.

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> Older chips only support DSA tagging on the CPU port. New devices
> support both DSA and EDSA. The driver needs to tell the core the tag
> protocol to use, and configure the switch for what is available.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

I'll send a follow-up patch to add a MV88E6XXX_FLAG_DSA flag, to prepare
support for older chips like 6060 which only support the Trailer proto.

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Thanks,

        Vivien

[PATCH 3/4] dsa: mv88e6xxx: Delete ppu timer when removing module

[Andrew Lunn]

The PPU method of accessing PHYs makes use of a timer. Make sure this
timer is deleted before unloading the driver.

Reported-by: Jamie Lentin <jm@lentin.co.uk>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index b315769aa5be..1d5f9576e62a 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -486,6 +486,11 @@ static void mv88e6xxx_ppu_state_init(struct mv88e6xxx_chip *chip)
 	chip->ppu_timer.function = mv88e6xxx_ppu_reenable_timer;
 }
 
+static void mv88e6xxx_ppu_state_destroy(struct mv88e6xxx_chip *chip)
+{
+	del_timer_sync(&chip->ppu_timer);
+}
+
 static int mv88e6xxx_phy_ppu_read(struct mv88e6xxx_chip *chip, int addr,
 				  int reg, u16 *val)
 {
@@ -3892,6 +3897,13 @@ static void mv88e6xxx_phy_init(struct mv88e6xxx_chip *chip)
 	}
 }
 
+static void mv88e6xxx_phy_destroy(struct mv88e6xxx_chip *chip)
+{
+	if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_PPU)) {
+		mv88e6xxx_ppu_state_destroy(chip);
+	}
+}
+
 static int mv88e6xxx_smi_init(struct mv88e6xxx_chip *chip,
 			      struct mii_bus *bus, int sw_addr)
 {
@@ -4080,6 +4092,7 @@ static void mv88e6xxx_remove(struct mdio_device *mdiodev)
 	struct dsa_switch *ds = dev_get_drvdata(&mdiodev->dev);
 	struct mv88e6xxx_chip *chip = ds_to_priv(ds);
 
+	mv88e6xxx_phy_destroy(chip);
 	mv88e6xxx_unregister_switch(chip);
 	mv88e6xxx_mdio_unregister(chip);
 }
-- 
2.8.1

Re: [PATCH 3/4] dsa: mv88e6xxx: Delete ppu timer when removing module

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> The PPU method of accessing PHYs makes use of a timer. Make sure this
> timer is deleted before unloading the driver.
>
> Reported-by: Jamie Lentin <jm@lentin.co.uk>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

I'm wondering if the PPU shouldn't be disabled before unloading the
module... Otherwise:

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Thanks,

        Vivien

Re: [PATCH 3/4] dsa: mv88e6xxx: Delete ppu timer when removing module

[Andrew Lunn]

On Mon, Aug 22, 2016 at 10:51:03AM -0400, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > The PPU method of accessing PHYs makes use of a timer. Make sure this
> > timer is deleted before unloading the driver.
> >
> > Reported-by: Jamie Lentin <jm@lentin.co.uk>
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> 
> I'm wondering if the PPU shouldn't be disabled before unloading the
> module...

Humm, i did not thing about that.

Actually, we have a general issue here. On unload, we leave the switch
'running'. The SF2 driver disables all the ports. The b53 seems to
leave the switch running.

We should define a general policy about this, and implement it in all
drivers. As such, your comment is valid, but a different patch set.

	Andrew

Re: [PATCH 3/4] dsa: mv88e6xxx: Delete ppu timer when removing module

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Mon, Aug 22, 2016 at 10:51:03AM -0400, Vivien Didelot wrote:
>> Hi Andrew,
>> 
>> Andrew Lunn <andrew@lunn.ch> writes:
>> 
>> > The PPU method of accessing PHYs makes use of a timer. Make sure this
>> > timer is deleted before unloading the driver.
>> >
>> > Reported-by: Jamie Lentin <jm@lentin.co.uk>
>> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>> 
>> I'm wondering if the PPU shouldn't be disabled before unloading the
>> module...
>
> Humm, i did not thing about that.
>
> Actually, we have a general issue here. On unload, we leave the switch
> 'running'. The SF2 driver disables all the ports. The b53 seems to
> leave the switch running.

That is true. We also certainly want to put the common logic in the DSA
layer.

> We should define a general policy about this, and implement it in all
> drivers. As such, your comment is valid, but a different patch set.

I agree, that's why I added my tag to that patch ;-)

Thanks,

        Vivien

Re: [PATCH 3/4] dsa: mv88e6xxx: Delete ppu timer when removing module

[Sergei Shtylyov]

Hello.

On 8/22/2016 5:01 PM, Andrew Lunn wrote:

> The PPU method of accessing PHYs makes use of a timer. Make sure this
> timer is deleted before unloading the driver.
>
> Reported-by: Jamie Lentin <jm@lentin.co.uk>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index b315769aa5be..1d5f9576e62a 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
[...]
> @@ -3892,6 +3897,13 @@ static void mv88e6xxx_phy_init(struct mv88e6xxx_chip *chip)
>  	}
>  }
>
> +static void mv88e6xxx_phy_destroy(struct mv88e6xxx_chip *chip)
> +{
> +	if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_PPU)) {
> +		mv88e6xxx_ppu_state_destroy(chip);
> +	}

    {} not needed here. See Documentation/CodingStyle, chapter 3.

> +}
> +
>  static int mv88e6xxx_smi_init(struct mv88e6xxx_chip *chip,
>  			      struct mii_bus *bus, int sw_addr)
>  {
[...]

MBR, Sergei

Re: [PATCH 3/4] dsa: mv88e6xxx: Delete ppu timer when removing module

[Andrew Lunn]

> >+static void mv88e6xxx_phy_destroy(struct mv88e6xxx_chip *chip)
> >+{
> >+	if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_PPU)) {
> >+		mv88e6xxx_ppu_state_destroy(chip);
> >+	}
> 
>    {} not needed here. See Documentation/CodingStyle, chapter 3.

Agreed, I will send a follow up patch, since the patchset has been
accepted.

Thanks
	Andrew

[PATCH 4/4] net: mv88e6xxx: Enable PORT_CONTROL_FORWARD_UNKNOWN for DSA-tagged CPU ports

[Andrew Lunn]

From: Jamie Lentin <jm@lentin.co.uk>

Without it, a mv88e6131 switch will not forward incoming unicast
packets to the CPU port.

Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 1d5f9576e62a..82d45165803c 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2490,11 +2490,11 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 	if (dsa_is_cpu_port(ds, port)) {
 		if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_EDSA))
 			reg |= PORT_CONTROL_FRAME_ETHER_TYPE_DSA |
-				PORT_CONTROL_FORWARD_UNKNOWN |
 				PORT_CONTROL_FORWARD_UNKNOWN_MC;
 		else
 			reg |= PORT_CONTROL_DSA_TAG;
-		reg |= PORT_CONTROL_EGRESS_ADD_TAG;
+		reg |= PORT_CONTROL_EGRESS_ADD_TAG |
+			PORT_CONTROL_FORWARD_UNKNOWN;
 	}
 	if (dsa_is_dsa_port(ds, port)) {
 		if (mv88e6xxx_6095_family(chip) ||
-- 
2.8.1

Re: [PATCH 4/4] net: mv88e6xxx: Enable PORT_CONTROL_FORWARD_UNKNOWN for DSA-tagged CPU ports

[Vivien Didelot]

Hi,

Andrew Lunn <andrew@lunn.ch> writes:

> From: Jamie Lentin <jm@lentin.co.uk>
>
> Without it, a mv88e6131 switch will not forward incoming unicast
> packets to the CPU port.
>
> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 1d5f9576e62a..82d45165803c 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -2490,11 +2490,11 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
>  	if (dsa_is_cpu_port(ds, port)) {
>  		if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_EDSA))
>  			reg |= PORT_CONTROL_FRAME_ETHER_TYPE_DSA |
> -				PORT_CONTROL_FORWARD_UNKNOWN |
>  				PORT_CONTROL_FORWARD_UNKNOWN_MC;
>  		else
>  			reg |= PORT_CONTROL_DSA_TAG;
> -		reg |= PORT_CONTROL_EGRESS_ADD_TAG;
> +		reg |= PORT_CONTROL_EGRESS_ADD_TAG |
> +			PORT_CONTROL_FORWARD_UNKNOWN;
>  	}
>  	if (dsa_is_dsa_port(ds, port)) {
>  		if (mv88e6xxx_6095_family(chip) ||
> -- 
> 2.8.1

Bits 2-3 in newer chips such as 6352 became an Egress Flooding Mode.
0x3 needs to be set to forward unknown unicast *and* multicast frames.

I'll extract that from the EDSA check and add flags for this mode
later. In the meantime, this works as a quick fix for 6131 and
compatibles.

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Thanks,

        Vivien

Re: [PATCH 0/4] Fix MV88E6131 tagging

[Jamie Lentin]

On Mon, 22 Aug 2016, Andrew Lunn wrote:

> Marvell has two different tagging protocols for frames passed to a
> swicth. There is the older DSA and the newer EDSA. Somewhere along the
> way, we broke support for switches which only support DSA, by trying
> to configure them to use EDSA. These patches add back support for
> switches which only support DSA, by allowing the drivers to
> dynamically indicate the tagging protocol they support to the DSA
> core. This needs to be dynamic since the mv88e6xxx has to support two
> protocols.
>
> Thanks go to Jamie Lentin for reporting the problem, helping debug it,
> providing some of the fix, and testing.

Entire series tested on a Netgear WNR854T With a mv88e6131 switch chip.

Tested-By: Jamie Lentin <jm@lentin.co.uk>

> Andrew Lunn (3):
>  net: dsa: Allow the DSA driver to indicate the tag protocol
>  net: dsa: mv88e6xxx: Fix support for DSA tagging for older switches.
>  dsa: mv88e6xxx: Delete ppu timer when removing module
>
> Jamie Lentin (1):
>  net: mv88e6xxx: Enable PORT_CONTROL_FORWARD_UNKNOWN for DSA-tagged CPU
>    ports
>
> drivers/net/dsa/b53/b53_common.c      |  7 +++-
> drivers/net/dsa/bcm_sf2.c             |  7 +++-
> drivers/net/dsa/mv88e6060.c           |  7 +++-
> drivers/net/dsa/mv88e6xxx/Kconfig     |  1 +
> drivers/net/dsa/mv88e6xxx/chip.c      | 61 +++++++++++++++++++++--------------
> drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 16 +++++++--
> include/net/dsa.h                     |  5 +--
> net/dsa/dsa.c                         |  5 ++-
> net/dsa/dsa2.c                        |  4 ++-
> 9 files changed, 78 insertions(+), 35 deletions(-)
>
>

-- 
Jamie Lentin

Re: [PATCH 0/4] Fix MV88E6131 tagging

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Mon, 22 Aug 2016 16:01:00 +0200

> Marvell has two different tagging protocols for frames passed to a
> swicth. There is the older DSA and the newer EDSA. Somewhere along the
> way, we broke support for switches which only support DSA, by trying
> to configure them to use EDSA. These patches add back support for
> switches which only support DSA, by allowing the drivers to
> dynamically indicate the tagging protocol they support to the DSA
> core. This needs to be dynamic since the mv88e6xxx has to support two
> protocols.
> 
> Thanks go to Jamie Lentin for reporting the problem, helping debug it,
> providing some of the fix, and testing.

Series applied, thanks.

As per tree-wide policy, when a driver unloads the device should be
disabled and in fact if possible configured to not draw power.