[PATCH net-next 0/3] net: dsa: Allow default tag protocol to be overridden from DT

This is logically the v2 of this patch:
https://lore.kernel.org/netdev/20210323102326.3677940-1-tobias@waldekranz.com/

In addition to the mv88e6xxx support to dynamically change the
protocol, it is now possible to override the protocol from the device
tree. This means that when a board vendor finds an incompatibility,
they can specify a working protocol in the DT, and users will not have
to worry about it.

Some background information:

In a system using an NXP T1023 SoC connected to a 6390X switch, we
noticed that TO_CPU frames where not reaching the CPU. This only
happened on hardware port 8. Looking at the DSA master interface
(dpaa-ethernet) we could see that an Rx error counter was bumped at
the same rate. The logs indicated a parser error.

It just so happens that a TO_CPU coming in on device 0, port 8, will
result in the first two bytes of the DSA tag being one of:

00 40
00 44
00 46

My guess was that since these values looked like 802.3 length fields,
the controller's parser would signal an error if the frame length did
not match what was in the header.

This was later confirmed using two different workarounds provided by
Vladimir. Unfortunately these either bypass or ignore the hardware
parser and thus robs working combinations of the ability to do RSS and
other nifty things. It was therefore decided to go with the option of
a DT override.

Tobias Waldekranz (3):
  net: dsa: mv88e6xxx: Allow dynamic reconfiguration of tag protocol
  net: dsa: Allow default tag protocol to be overridden from DT
  dt-bindings: net: dsa: Document dsa,tag-protocol property

 .../devicetree/bindings/net/dsa/dsa.yaml      |  7 ++
 drivers/net/dsa/mv88e6xxx/chip.c              | 41 +++++++-
 drivers/net/dsa/mv88e6xxx/chip.h              |  3 +
 include/net/dsa.h                             |  5 +
 net/dsa/dsa2.c                                | 95 +++++++++++++++----
 5 files changed, 132 insertions(+), 19 deletions(-)

-- 
2.25.1

All devices are capable of using regular DSA tags. Support for
Ethertyped DSA tags sort into three categories:

1. No support. Older chips fall into this category.

2. Full support. Datasheet explicitly supports configuring the CPU
   port to receive FORWARDs with a DSA tag.

3. Undocumented support. Datasheet lists the configuration from
   category 2 as "reserved for future use", but does empirically
   behave like a category 2 device.

Because there are ethernet controllers that do not handle regular DSA
tags in all cases, it is sometimes preferable to rely on the
undocumented behavior, as the alternative is a very crippled
system. But, in those cases, make sure to log the fact that an
undocumented feature has been enabled.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 41 +++++++++++++++++++++++++++++---
 drivers/net/dsa/mv88e6xxx/chip.h |  3 +++
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 95f07fcd4f85..e7ec883d5f6b 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c

@@ -2531,10 +2531,10 @@ static int mv88e6xxx_setup_port_mode(struct mv88e6xxx_chip *chip, int port)
 		return mv88e6xxx_set_port_mode_normal(chip, port);
 
 	/* Setup CPU port mode depending on its supported tag format */
-	if (chip->info->tag_protocol == DSA_TAG_PROTO_DSA)
+	if (chip->tag_protocol == DSA_TAG_PROTO_DSA)
 		return mv88e6xxx_set_port_mode_dsa(chip, port);
 
-	if (chip->info->tag_protocol == DSA_TAG_PROTO_EDSA)
+	if (chip->tag_protocol == DSA_TAG_PROTO_EDSA)
 		return mv88e6xxx_set_port_mode_edsa(chip, port);
 
 	return -EINVAL;

@@ -5564,7 +5564,39 @@ static enum dsa_tag_protocol mv88e6xxx_get_tag_protocol(struct dsa_switch *ds,
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 
-	return chip->info->tag_protocol;
+	return chip->tag_protocol;
+}
+
+static int mv88e6xxx_change_tag_protocol(struct dsa_switch *ds, int port,
+					 enum dsa_tag_protocol proto)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	enum dsa_tag_protocol old_protocol;
+	int err;
+
+	switch (proto) {
+	case DSA_TAG_PROTO_EDSA:
+		if (chip->info->tag_protocol != DSA_TAG_PROTO_EDSA)
+			dev_warn(chip->dev, "Relying on undocumented EDSA tagging behavior\n");
+
+		break;
+	case DSA_TAG_PROTO_DSA:
+		break;
+	default:
+		return -EPROTONOSUPPORT;
+	}
+
+	old_protocol = chip->tag_protocol;
+	chip->tag_protocol = proto;
+
+	mv88e6xxx_reg_lock(chip);
+	err = mv88e6xxx_setup_port_mode(chip, port);
+	mv88e6xxx_reg_unlock(chip);
+
+	if (err)
+		chip->tag_protocol = old_protocol;
+
+	return err;
 }
 
 static int mv88e6xxx_port_mdb_add(struct dsa_switch *ds, int port,

@@ -6029,6 +6061,7 @@ static int mv88e6xxx_crosschip_lag_leave(struct dsa_switch *ds, int sw_index,
 
 static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.get_tag_protocol	= mv88e6xxx_get_tag_protocol,
+	.change_tag_protocol	= mv88e6xxx_change_tag_protocol,
 	.setup			= mv88e6xxx_setup,
 	.teardown		= mv88e6xxx_teardown,
 	.phylink_validate	= mv88e6xxx_validate,

@@ -6209,6 +6242,8 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	if (err)
 		goto out;
 
+	chip->tag_protocol = chip->info->tag_protocol;
+
 	mv88e6xxx_phy_init(chip);
 
 	if (chip->info->ops->get_eeprom) {

diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index bce6e0dc8535..96b775f3fda2 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h

@@ -261,6 +261,9 @@ struct mv88e6xxx_region_priv {
 struct mv88e6xxx_chip {
 	const struct mv88e6xxx_info *info;
 
+	/* Currently configured tagging protocol */
+	enum dsa_tag_protocol tag_protocol;
+
 	/* The dsa_switch this private structure is related to */
 	struct dsa_switch *ds;
 

-- 
2.25.1

Some combinations of tag protocols and Ethernet controllers are
incompatible, and it is hard for the driver to keep track of these.

Therefore, allow the device tree author (typically the board vendor)
to inform the driver of this fact by selecting an alternate protocol
that is known to work.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---

This deviates from the advise given by Andrew in that we store the
switches' default protocol on the tree rather than the override
value. I chose this because you want to make sure that the tagger
(default or overwritten) is available at probe time, and since that
means we actually load it, it made more sense to keep it loaded.

Then we just check during setup if we are running a different tagger
than what the driver would expect.

Somewhat related: since we know the default now, users could easily
revert to it via sysfs: `echo >/sys/class/eth0/dsa/tagging`. Not sure
if that would be useful.

 include/net/dsa.h |  5 +++
 net/dsa/dsa2.c    | 95 +++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 84 insertions(+), 16 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 57b2c49f72f4..2385ba317888 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h

@@ -149,6 +149,11 @@ struct dsa_switch_tree {
 	/* Tagging protocol operations */
 	const struct dsa_device_ops *tag_ops;
 
+	/* Default tagging protocol preferred by the switches in this
+	 * tree.
+	 */
+	enum dsa_tag_protocol default_proto;
+
 	/*
 	 * Configuration data for the platform device that owns
 	 * this dsa switch tree instance.

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 4d4956ed303b..52afe70f75af 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c

@@ -668,6 +668,35 @@ static const struct devlink_ops dsa_devlink_ops = {
 	.sb_occ_tc_port_bind_get	= dsa_devlink_sb_occ_tc_port_bind_get,
 };
 
+static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
+{
+	const struct dsa_device_ops *tag_ops = ds->dst->tag_ops;
+	struct dsa_switch_tree *dst = ds->dst;
+	int port, err;
+
+	if (tag_ops->proto == dst->default_proto)
+		return 0;
+
+	if (!ds->ops->change_tag_protocol) {
+		dev_err(ds->dev, "Tag protocol cannot be modified\n");
+		return -EINVAL;
+	}
+
+	for (port = 0; port < ds->num_ports; port++) {
+		if (!(dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port)))
+			continue;
+
+		err = ds->ops->change_tag_protocol(ds, port, tag_ops->proto);
+		if (err) {
+			dev_err(ds->dev, "Tag protocol \"%s\" is not supported\n",
+				tag_ops->name);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 static int dsa_switch_setup(struct dsa_switch *ds)
 {
 	struct dsa_devlink_priv *dl_priv;

@@ -718,6 +747,10 @@ static int dsa_switch_setup(struct dsa_switch *ds)
 	if (err < 0)
 		goto unregister_notifier;
 
+	err = dsa_switch_setup_tag_protocol(ds);
+	if (err)
+		goto teardown;
+
 	devlink_params_publish(ds->devlink);
 
 	if (!ds->slave_mii_bus && ds->ops->phy_read) {

@@ -1062,32 +1095,60 @@ static enum dsa_tag_protocol dsa_get_tag_protocol(struct dsa_port *dp,
 	return ds->ops->get_tag_protocol(ds, dp->index, tag_protocol);
 }
 
-static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master)
+static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master,
+			      const char *user_protocol)
 {
 	struct dsa_switch *ds = dp->ds;
 	struct dsa_switch_tree *dst = ds->dst;
-	enum dsa_tag_protocol tag_protocol;
+	const struct dsa_device_ops *tag_ops;
+	enum dsa_tag_protocol default_proto;
+
+	/* Find out which protocol the switch would prefer. */
+	default_proto = dsa_get_tag_protocol(dp, master);
+	if (dst->default_proto) {
+		if (dst->default_proto != default_proto) {
+			dev_err(ds->dev,
+				"A DSA switch tree can have only one tagging protovol\n");
+			return -EINVAL;
+		}
+	} else {
+		dst->default_proto = default_proto;
+	}
+
+	/* See if the user wants to override that preference. */
+	if (user_protocol && ds->ops->change_tag_protocol) {
+		tag_ops = dsa_find_tagger_by_name(user_protocol);
+	} else {
+		if (user_protocol)
+			dev_warn(ds->dev,
+				 "Tag protocol cannot be modified, using default\n");
+
+		tag_ops = dsa_tag_driver_get(default_proto);
+	}
+
+	if (IS_ERR(tag_ops)) {
+		if (PTR_ERR(tag_ops) == -ENOPROTOOPT)
+			return -EPROBE_DEFER;
+
+		dev_warn(ds->dev, "No tagger for this switch\n");
+		return PTR_ERR(tag_ops);
+	}
 
-	tag_protocol = dsa_get_tag_protocol(dp, master);
 	if (dst->tag_ops) {
-		if (dst->tag_ops->proto != tag_protocol) {
+		if (dst->tag_ops != tag_ops) {
 			dev_err(ds->dev,
 				"A DSA switch tree can have only one tagging protocol\n");
+
+			dsa_tag_driver_put(tag_ops);
 			return -EINVAL;
 		}
+
 		/* In the case of multiple CPU ports per switch, the tagging
-		 * protocol is still reference-counted only per switch tree, so
-		 * nothing to do here.
+		 * protocol is still reference-counted only per switch tree.
 		 */
+		dsa_tag_driver_put(tag_ops);
 	} else {
-		dst->tag_ops = dsa_tag_driver_get(tag_protocol);
-		if (IS_ERR(dst->tag_ops)) {
-			if (PTR_ERR(dst->tag_ops) == -ENOPROTOOPT)
-				return -EPROBE_DEFER;
-			dev_warn(ds->dev, "No tagger for this switch\n");
-			dp->master = NULL;
-			return PTR_ERR(dst->tag_ops);
-		}
+		dst->tag_ops = tag_ops;
 	}
 
 	dp->master = master;

@@ -1108,12 +1169,14 @@ static int dsa_port_parse_of(struct dsa_port *dp, struct device_node *dn)
 
 	if (ethernet) {
 		struct net_device *master;
+		const char *user_protocol;
 
 		master = of_find_net_device_by_node(ethernet);
 		if (!master)
 			return -EPROBE_DEFER;
 
-		return dsa_port_parse_cpu(dp, master);
+		user_protocol = of_get_property(dn, "dsa,tag-protocol", NULL);
+		return dsa_port_parse_cpu(dp, master, user_protocol);
 	}
 
 	if (link)

@@ -1225,7 +1288,7 @@ static int dsa_port_parse(struct dsa_port *dp, const char *name,
 
 		dev_put(master);
 
-		return dsa_port_parse_cpu(dp, master);
+		return dsa_port_parse_cpu(dp, master, NULL);
 	}
 
 	if (!strcmp(name, "dsa"))

-- 
2.25.1

The 'dsa,tag-protocol' is used to force a switch tree to use a
particular tag protocol, typically because the Ethernet controller
that it is connected to is not compatible with the default one.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 Documentation/devicetree/bindings/net/dsa/dsa.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
index 8a3494db4d8d..5dcfab049aa2 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml

@@ -70,6 +70,13 @@ patternProperties:
               device is what the switch port is connected to
             $ref: /schemas/types.yaml#/definitions/phandle
 
+          dsa,tag-protocol:
+            description:
+              Instead of the default, the switch will use this tag protocol if
+              possible. Useful when a device supports multiple protcols and
+              the default is incompatible with the Ethernet device.
+            $ref: /schemas/types.yaml#/definitions/string
+
           phy-handle: true
 
           phy-mode: true

-- 
2.25.1

Hi Tobias,

On Fri, Mar 26, 2021 at 11:56:47AM +0100, Tobias Waldekranz wrote:

 	} else {
-		dst->tag_ops = dsa_tag_driver_get(tag_protocol);
-		if (IS_ERR(dst->tag_ops)) {
-			if (PTR_ERR(dst->tag_ops) == -ENOPROTOOPT)
-				return -EPROBE_DEFER;
-			dev_warn(ds->dev, "No tagger for this switch\n");
-			dp->master = NULL;
-			return PTR_ERR(dst->tag_ops);
-		}
+		dst->tag_ops = tag_ops;
 	}

This will conflict with George's bug fix for 'net', am I right?
https://patchwork.kernel.org/project/netdevbpf/patch/20210322202650.45776-1-george.mccollister@gmail.com/

Would you mind resending after David merges 'net' into 'net-next'?

This process usually looks like commit d489ded1a369 ("Merge
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net"). However,
during this kernel development cycle, I have seen no merge of 'net' into
'net-next' since commit 05a59d79793d ("Merge
git://git.kernel.org:/pub/scm/linux/kernel/git/netdev/net"), but that
comes directly from Linus Torvalds' v5.12-rc2.

Nonetheless, at some point (and sooner rather than later, I think),
David or Jakub should merge the two trees. I would prefer to do it this
way because the merge is going to be a bit messy otherwise, and I might
want to cherry-pick these patches to some trees and it would be nice if
the history was linear.

Thanks!

On Fri, Mar 26, 2021 at 14:57, Vladimir Oltean [off-list ref] wrote:

Hi Tobias,

On Fri, Mar 26, 2021 at 11:56:47AM +0100, Tobias Waldekranz wrote:

quoted

 	} else {
-		dst->tag_ops = dsa_tag_driver_get(tag_protocol);
-		if (IS_ERR(dst->tag_ops)) {
-			if (PTR_ERR(dst->tag_ops) == -ENOPROTOOPT)
-				return -EPROBE_DEFER;
-			dev_warn(ds->dev, "No tagger for this switch\n");
-			dp->master = NULL;
-			return PTR_ERR(dst->tag_ops);
-		}
+		dst->tag_ops = tag_ops;
 	}

This will conflict with George's bug fix for 'net', am I right?
https://patchwork.kernel.org/project/netdevbpf/patch/20210322202650.45776-1-george.mccollister@gmail.com/

Yes; this version also fixes George's problem I think, as we do not
assign dst->tag_ops until we know it is good, but it will not merge
cleanly.

Would you mind resending after David merges 'net' into 'net-next'?

Sure thing. Should I then call that v2 or a resend of v1? The patches
will not be identical, so v2 I guess?

This process usually looks like commit d489ded1a369 ("Merge
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net"). However,
during this kernel development cycle, I have seen no merge of 'net' into
'net-next' since commit 05a59d79793d ("Merge
git://git.kernel.org:/pub/scm/linux/kernel/git/netdev/net"), but that
comes directly from Linus Torvalds' v5.12-rc2.

Nonetheless, at some point (and sooner rather than later, I think),
David or Jakub should merge the two trees. I would prefer to do it this
way because the merge is going to be a bit messy otherwise, and I might
want to cherry-pick these patches to some trees and it would be nice if
the history was linear.

Thanks!

On Fri, Mar 26, 2021 at 11:56:48AM +0100, Tobias Waldekranz wrote:

'dsa' is not a vendor.

+            description:
+              Instead of the default, the switch will use this tag protocol if
+              possible. Useful when a device supports multiple protcols and
+              the default is incompatible with the Ethernet device.
+            $ref: /schemas/types.yaml#/definitions/string

You need to define the possible strings.

+
           phy-handle: true
 
           phy-mode: true
-- 
2.25.1

On Fri, Mar 26, 2021 at 11:56:46AM +0100, Tobias Waldekranz wrote:

All devices are capable of using regular DSA tags. Support for
Ethertyped DSA tags sort into three categories:

1. No support. Older chips fall into this category.

2. Full support. Datasheet explicitly supports configuring the CPU
   port to receive FORWARDs with a DSA tag.

3. Undocumented support. Datasheet lists the configuration from
   category 2 as "reserved for future use", but does empirically
   behave like a category 2 device.

+static int mv88e6xxx_change_tag_protocol(struct dsa_switch *ds, int port,
+					 enum dsa_tag_protocol proto)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	enum dsa_tag_protocol old_protocol;
+	int err;
+
+	switch (proto) {
+	case DSA_TAG_PROTO_EDSA:
+		if (chip->info->tag_protocol != DSA_TAG_PROTO_EDSA)
+			dev_warn(chip->dev, "Relying on undocumented EDSA tagging behavior\n");
+
+		break;
+	case DSA_TAG_PROTO_DSA:
+		break;
+	default:
+		return -EPROTONOSUPPORT;
+	}

You are handling cases 2 and 3 here, but not 1. Which makes it a bit
of a foot cannon for older devices.

Now that we have chip->tag_protocol, maybe we should change
chip->info->tag_protocol to mean supported protocols?

BIT(0) DSA
BIT(1) EDSA
BIT(2) Undocumented EDSA

Andrew

+static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
+{
+	const struct dsa_device_ops *tag_ops = ds->dst->tag_ops;
+	struct dsa_switch_tree *dst = ds->dst;
+	int port, err;
+
+	if (tag_ops->proto == dst->default_proto)
+		return 0;
+
+	if (!ds->ops->change_tag_protocol) {
+		dev_err(ds->dev, "Tag protocol cannot be modified\n");
+		return -EINVAL;
+	}
+
+	for (port = 0; port < ds->num_ports; port++) {
+		if (!(dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port)))
+			continue;

dsa_is_dsa_port() is interesting. Do we care about the tagging
protocol on DSA ports? We never see that traffic?

+
+		err = ds->ops->change_tag_protocol(ds, port, tag_ops->proto);
+		if (err) {
+			dev_err(ds->dev, "Tag protocol \"%s\" is not supported\n",
+				tag_ops->name);
+			return err;
+		}
+	}
+
+	return 0;
+}
+

-static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master)
+static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master,
+			      const char *user_protocol)
 {
 	struct dsa_switch *ds = dp->ds;
 	struct dsa_switch_tree *dst = ds->dst;
-	enum dsa_tag_protocol tag_protocol;
+	const struct dsa_device_ops *tag_ops;
+	enum dsa_tag_protocol default_proto;
+
+	/* Find out which protocol the switch would prefer. */
+	default_proto = dsa_get_tag_protocol(dp, master);
+	if (dst->default_proto) {
+		if (dst->default_proto != default_proto) {
+			dev_err(ds->dev,
+				"A DSA switch tree can have only one tagging protovol\n");
+			return -EINVAL;
+		}
+	} else {
+		dst->default_proto = default_proto;
+	}
+
+	/* See if the user wants to override that preference. */
+	if (user_protocol && ds->ops->change_tag_protocol) {
+		tag_ops = dsa_find_tagger_by_name(user_protocol);
+	} else {
+		if (user_protocol)
+			dev_warn(ds->dev,
+				 "Tag protocol cannot be modified, using default\n");

I would probably error out here. I don't think it is a good idea to
ignore what DT says. We also potentially have forward compatibility
problems. Somebody cut/pastes a DT fragment including an invalid
override. But the driver does not support it, so it just gives this
warning and keeps going. Sometime in the future, change support is
added, it then becomes a real error, and the driver stops probing.

       Andrew

On Sun, Mar 28, 2021 at 05:52:43PM +0200, Andrew Lunn wrote:

quoted

+static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
+{
+	const struct dsa_device_ops *tag_ops = ds->dst->tag_ops;
+	struct dsa_switch_tree *dst = ds->dst;
+	int port, err;
+
+	if (tag_ops->proto == dst->default_proto)
+		return 0;
+
+	if (!ds->ops->change_tag_protocol) {
+		dev_err(ds->dev, "Tag protocol cannot be modified\n");
+		return -EINVAL;
+	}
+
+	for (port = 0; port < ds->num_ports; port++) {
+		if (!(dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port)))
+			continue;

dsa_is_dsa_port() is interesting. Do we care about the tagging
protocol on DSA ports? We never see that traffic?

I believe this comes from me (see dsa_switch_tag_proto_match). I did not
take into consideration at that time the fact that Marvell switches can
translate between DSA and EDSA. So I assumed that every switch in the
tree needs a notification about the tagging protocol, not just the
top-most one.

On Mon, Mar 29, 2021 at 12:53:09AM +0300, Vladimir Oltean wrote:

On Sun, Mar 28, 2021 at 05:52:43PM +0200, Andrew Lunn wrote:

quoted

quoted

+static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
+{
+	const struct dsa_device_ops *tag_ops = ds->dst->tag_ops;
+	struct dsa_switch_tree *dst = ds->dst;
+	int port, err;
+
+	if (tag_ops->proto == dst->default_proto)
+		return 0;
+
+	if (!ds->ops->change_tag_protocol) {
+		dev_err(ds->dev, "Tag protocol cannot be modified\n");
+		return -EINVAL;
+	}
+
+	for (port = 0; port < ds->num_ports; port++) {
+		if (!(dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port)))
+			continue;

dsa_is_dsa_port() is interesting. Do we care about the tagging
protocol on DSA ports? We never see that traffic?

I believe this comes from me (see dsa_switch_tag_proto_match). I did not
take into consideration at that time the fact that Marvell switches can
translate between DSA and EDSA. So I assumed that every switch in the
tree needs a notification about the tagging protocol, not just the
top-most one.

Hi Vladimir

static int mv88e6xxx_setup_port_mode(struct mv88e6xxx_chip *chip, int port)
{
        if (dsa_is_dsa_port(chip->ds, port))
                return mv88e6xxx_set_port_mode_dsa(chip, port);

So DSA ports, the ports connecting two switches together, are hard
coded to use DSA.

        if (dsa_is_user_port(chip->ds, port))
                return mv88e6xxx_set_port_mode_normal(chip, port);

        /* Setup CPU port mode depending on its supported tag format */
        if (chip->info->tag_protocol == DSA_TAG_PROTO_DSA)
                return mv88e6xxx_set_port_mode_dsa(chip, port);

        if (chip->info->tag_protocol == DSA_TAG_PROTO_EDSA)
                return mv88e6xxx_set_port_mode_edsa(chip, port);

CPU ports can be configured to DSA or EDSA.

The switches seem happy to translate between DSA and EDSA as needed.

    Andrew

On Mon, Mar 29, 2021 at 12:04:39AM +0200, Andrew Lunn wrote:

On Mon, Mar 29, 2021 at 12:53:09AM +0300, Vladimir Oltean wrote:

quoted

On Sun, Mar 28, 2021 at 05:52:43PM +0200, Andrew Lunn wrote:

quoted

quoted

+static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
+{
+	const struct dsa_device_ops *tag_ops = ds->dst->tag_ops;
+	struct dsa_switch_tree *dst = ds->dst;
+	int port, err;
+
+	if (tag_ops->proto == dst->default_proto)
+		return 0;
+
+	if (!ds->ops->change_tag_protocol) {
+		dev_err(ds->dev, "Tag protocol cannot be modified\n");
+		return -EINVAL;
+	}
+
+	for (port = 0; port < ds->num_ports; port++) {
+		if (!(dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port)))
+			continue;

dsa_is_dsa_port() is interesting. Do we care about the tagging
protocol on DSA ports? We never see that traffic?

I believe this comes from me (see dsa_switch_tag_proto_match). I did not
take into consideration at that time the fact that Marvell switches can
translate between DSA and EDSA. So I assumed that every switch in the
tree needs a notification about the tagging protocol, not just the
top-most one.

Hi Vladimir

static int mv88e6xxx_setup_port_mode(struct mv88e6xxx_chip *chip, int port)
{
        if (dsa_is_dsa_port(chip->ds, port))
                return mv88e6xxx_set_port_mode_dsa(chip, port);

So DSA ports, the ports connecting two switches together, are hard
coded to use DSA.

        if (dsa_is_user_port(chip->ds, port))
                return mv88e6xxx_set_port_mode_normal(chip, port);

        /* Setup CPU port mode depending on its supported tag format */
        if (chip->info->tag_protocol == DSA_TAG_PROTO_DSA)
                return mv88e6xxx_set_port_mode_dsa(chip, port);

        if (chip->info->tag_protocol == DSA_TAG_PROTO_EDSA)
                return mv88e6xxx_set_port_mode_edsa(chip, port);

CPU ports can be configured to DSA or EDSA.

The switches seem happy to translate between DSA and EDSA as needed.

I agree, and this is a particular feature of Marvell, not something that
can be said in general. However, I have nothing against deleting the
dsa_is_dsa_port checks from dsa_switch_tag_proto_match and from Tobias'
patch, since nobody needs them at the moment.

On Fri, Mar 26, 2021 at 02:57:20PM +0200, Vladimir Oltean wrote:

Hi Tobias,

On Fri, Mar 26, 2021 at 11:56:47AM +0100, Tobias Waldekranz wrote:

quoted

 	} else {
-		dst->tag_ops = dsa_tag_driver_get(tag_protocol);
-		if (IS_ERR(dst->tag_ops)) {
-			if (PTR_ERR(dst->tag_ops) == -ENOPROTOOPT)
-				return -EPROBE_DEFER;
-			dev_warn(ds->dev, "No tagger for this switch\n");
-			dp->master = NULL;
-			return PTR_ERR(dst->tag_ops);
-		}
+		dst->tag_ops = tag_ops;
 	}

This will conflict with George's bug fix for 'net', am I right?
https://patchwork.kernel.org/project/netdevbpf/patch/20210322202650.45776-1-george.mccollister@gmail.com/

Would you mind resending after David merges 'net' into 'net-next'?

This process usually looks like commit d489ded1a369 ("Merge
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net"). However,
during this kernel development cycle, I have seen no merge of 'net' into
'net-next' since commit 05a59d79793d ("Merge
git://git.kernel.org:/pub/scm/linux/kernel/git/netdev/net"), but that
comes directly from Linus Torvalds' v5.12-rc2.

Nonetheless, at some point (and sooner rather than later, I think),
David or Jakub should merge the two trees. I would prefer to do it this
way because the merge is going to be a bit messy otherwise, and I might
want to cherry-pick these patches to some trees and it would be nice if
the history was linear.

Thanks!

Tobias, I think you can safely resend now, I see George's change is in
net-next:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/dsa/dsa2.c#n1084

On Sun, Mar 28, 2021 at 17:24, Andrew Lunn [off-list ref] wrote:

On Fri, Mar 26, 2021 at 11:56:46AM +0100, Tobias Waldekranz wrote:

quoted

All devices are capable of using regular DSA tags. Support for
Ethertyped DSA tags sort into three categories:

1. No support. Older chips fall into this category.

2. Full support. Datasheet explicitly supports configuring the CPU
   port to receive FORWARDs with a DSA tag.

3. Undocumented support. Datasheet lists the configuration from
   category 2 as "reserved for future use", but does empirically
   behave like a category 2 device.

quoted

+static int mv88e6xxx_change_tag_protocol(struct dsa_switch *ds, int port,
+					 enum dsa_tag_protocol proto)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	enum dsa_tag_protocol old_protocol;
+	int err;
+
+	switch (proto) {
+	case DSA_TAG_PROTO_EDSA:
+		if (chip->info->tag_protocol != DSA_TAG_PROTO_EDSA)
+			dev_warn(chip->dev, "Relying on undocumented EDSA tagging behavior\n");
+
+		break;
+	case DSA_TAG_PROTO_DSA:
+		break;
+	default:
+		return -EPROTONOSUPPORT;
+	}

You are handling cases 2 and 3 here, but not 1. Which makes it a bit
of a foot cannon for older devices.

Now that we have chip->tag_protocol, maybe we should change
chip->info->tag_protocol to mean supported protocols?

BIT(0) DSA
BIT(1) EDSA
BIT(2) Undocumented EDSA

Since DSA is supported on all devices, perhaps we should just have:

enum mv88e6xxx_edsa_support {
     MV88E6XXX_EDSA_UNSUPPORTED,
     MV88E6XXX_EDSA_UNDOCUMENTED,
     MV88E6XXX_EDSA_SUPPORTED,
};

?

Do we also want to default to DSA on all devices unless there is a
DT-property saying something else? Using EDSA does not really give you
anything over bare tags anymore. You have fixed the tcpdump-issue, and
the tagger drivers have been unified so there should be no risk of any
regressions there either.

On Sat, Mar 27, 2021 at 12:13, Rob Herring [off-list ref] wrote:

On Fri, Mar 26, 2021 at 11:56:48AM +0100, Tobias Waldekranz wrote:

quoted

The 'dsa,tag-protocol' is used to force a switch tree to use a
particular tag protocol, typically because the Ethernet controller
that it is connected to is not compatible with the default one.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 Documentation/devicetree/bindings/net/dsa/dsa.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
index 8a3494db4d8d..5dcfab049aa2 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml

@@ -70,6 +70,13 @@ patternProperties:
               device is what the switch port is connected to
             $ref: /schemas/types.yaml#/definitions/phandle
 
+          dsa,tag-protocol:

'dsa' is not a vendor.

It is not. The property is intended to be consumed by the
vendor-independent driver. So should it be linux,tag-protocol? Just
tag-protocol?

quoted

+            description:
+              Instead of the default, the switch will use this tag protocol if
+              possible. Useful when a device supports multiple protcols and
+              the default is incompatible with the Ethernet device.
+            $ref: /schemas/types.yaml#/definitions/string

You need to define the possible strings.

Alright.

Andrew, Vladimir: I will just list dsa and edsa for now. If it is needed
on other devices, people can add them to the list after they have tested
their drivers. Fair?

quoted

+
           phy-handle: true
 
           phy-mode: true
-- 
2.25.1

Since DSA is supported on all devices, perhaps we should just have:

enum mv88e6xxx_edsa_support {
     MV88E6XXX_EDSA_UNSUPPORTED,
     MV88E6XXX_EDSA_UNDOCUMENTED,
     MV88E6XXX_EDSA_SUPPORTED,
};

Yes, that is O.K.
 

Do we also want to default to DSA on all devices unless there is a
DT-property saying something else? Using EDSA does not really give you
anything over bare tags anymore. You have fixed the tcpdump-issue, and
the tagger drivers have been unified so there should be no risk of any
regressions there either.

The regressions with be exactly what you are trying to fix here. A MAC
which does not understand the DSA tag and does the wrong thing, where
as currently it is using EDSA and working.

So i would keep things as they are by default.

   Andrew

Andrew, Vladimir: I will just list dsa and edsa for now. If it is needed
on other devices, people can add them to the list after they have tested
their drivers. Fair?

O.K.

	Andrew

On Tue, Apr 06, 2021 at 03:30:46PM +0200, Andrew Lunn wrote:

quoted

Andrew, Vladimir: I will just list dsa and edsa for now. If it is needed
on other devices, people can add them to the list after they have tested
their drivers. Fair?

O.K.

Same here.