| Message ID | 20210326105648.2492411-2-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: Allow default tag protocol to be overridden from DT | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to net-next |
| netdev/tree_selection | success | Clearly marked for net-next |
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
On Sun, Mar 28, 2021 at 17:24, Andrew Lunn <andrew@lunn.ch> wrote: > 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 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.
> 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
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;
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(-)