| Message ID | 20220301100321.951175-11-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: bridge: Multiple Spanning Trees | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | fail | Errors and warnings before: 5 this patch: 5 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | fail | Errors and warnings before: 20 this patch: 20 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 5 this patch: 5 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 237 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, Mar 01, 2022 at 11:03:21AM +0100, Tobias Waldekranz wrote: > Allocate a SID in the STU for each MSTID in use by a bridge and handle > the mapping of MSTIDs to VLANs using the SID field of each VTU entry. > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > --- > drivers/net/dsa/mv88e6xxx/chip.c | 178 +++++++++++++++++++++++++++++++ > drivers/net/dsa/mv88e6xxx/chip.h | 13 +++ > 2 files changed, 191 insertions(+) > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index c14a62aa6a6c..4fb4ec1dff79 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -1818,6 +1818,137 @@ static int mv88e6xxx_stu_setup(struct mv88e6xxx_chip *chip) > return mv88e6xxx_stu_loadpurge(chip, &stu); > } > > +static int mv88e6xxx_sid_new(struct mv88e6xxx_chip *chip, u8 *sid) > +{ > + DECLARE_BITMAP(busy, MV88E6XXX_N_SID) = { 0 }; > + struct mv88e6xxx_mst *mst; > + > + set_bit(0, busy); > + > + list_for_each_entry(mst, &chip->msts, node) { > + set_bit(mst->stu.sid, busy); > + } > + > + *sid = find_first_zero_bit(busy, MV88E6XXX_N_SID); > + > + return (*sid >= mv88e6xxx_max_sid(chip)) ? -ENOSPC : 0; > +} > + > +static int mv88e6xxx_sid_put(struct mv88e6xxx_chip *chip, u8 sid) > +{ > + struct mv88e6xxx_mst *mst, *tmp; > + int err = 0; > + > + list_for_each_entry_safe(mst, tmp, &chip->msts, node) { > + if (mst->stu.sid == sid) { > + if (refcount_dec_and_test(&mst->refcnt)) { > + mst->stu.valid = false; > + err = mv88e6xxx_stu_loadpurge(chip, &mst->stu); It is interesting what to do if this fails. Possibly not this, because the entry remains in hardware but not in software. > + list_del(&mst->node); > + kfree(mst); > + } > + > + return err; > + } > + } > + > + return -ENOENT; > +} > + > +static int mv88e6xxx_sid_get(struct mv88e6xxx_chip *chip, struct net_device *br, > + u16 msti, u8 *sid) > +{ > + struct mv88e6xxx_mst *mst; > + int err, i; > + > + if (!br) > + return 0; Is this condition possible? > + > + if (!mv88e6xxx_has_stu(chip)) > + return -EOPNOTSUPP; > + > + list_for_each_entry(mst, &chip->msts, node) { > + if (mst->br == br && mst->msti == msti) { > + refcount_inc(&mst->refcnt); > + *sid = mst->stu.sid; > + return 0; > + } > + } > + > + err = mv88e6xxx_sid_new(chip, sid); > + if (err) > + return err; > + > + mst = kzalloc(sizeof(*mst), GFP_KERNEL); > + if (!mst) > + return -ENOMEM; This leaks the new SID. > + > + INIT_LIST_HEAD(&mst->node); > + refcount_set(&mst->refcnt, 1); > + mst->br = br; > + mst->msti = msti; > + mst->stu.valid = true; > + mst->stu.sid = *sid; > + > + /* The bridge starts out all ports in the disabled state. But > + * a STU state of disabled means to go by the port-global > + * state. So we set all user port's initial state to blocking, > + * to match the bridge's behavior. > + */ > + for (i = 0; i < mv88e6xxx_num_ports(chip); i++) > + mst->stu.state[i] = dsa_is_user_port(chip->ds, i) ? > + MV88E6XXX_PORT_CTL0_STATE_BLOCKING : > + MV88E6XXX_PORT_CTL0_STATE_DISABLED; > + > + list_add_tail(&mst->node, &chip->msts); > + return mv88e6xxx_stu_loadpurge(chip, &mst->stu); And this doesn't behave too well on failure (the MSTID exists in software but not in hardware). > +} > + > +static int mv88e6xxx_port_mst_state_set(struct dsa_switch *ds, int port, > + const struct switchdev_mst_state *st) > +{ > + struct dsa_port *dp = dsa_to_port(ds, port); > + struct mv88e6xxx_chip *chip = ds->priv; > + struct mv88e6xxx_mst *mst; > + u8 state; > + int err; > + > + if (!mv88e6xxx_has_stu(chip)) > + return -EOPNOTSUPP; > + > + switch (st->state) { > + case BR_STATE_DISABLED: > + case BR_STATE_BLOCKING: > + case BR_STATE_LISTENING: > + state = MV88E6XXX_PORT_CTL0_STATE_BLOCKING; > + break; > + case BR_STATE_LEARNING: > + state = MV88E6XXX_PORT_CTL0_STATE_LEARNING; > + break; > + case BR_STATE_FORWARDING: > + state = MV88E6XXX_PORT_CTL0_STATE_FORWARDING; > + break; > + default: > + return -EINVAL; > + } > + > + list_for_each_entry(mst, &chip->msts, node) { > + if (mst->br == dsa_port_bridge_dev_get(dp) && > + mst->msti == st->msti) { > + if (mst->stu.state[port] == state) > + return 0; > + > + mst->stu.state[port] = state; > + mv88e6xxx_reg_lock(chip); > + err = mv88e6xxx_stu_loadpurge(chip, &mst->stu); > + mv88e6xxx_reg_unlock(chip); > + return err; > + } > + } > + > + return -ENOENT; > +} > + > static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port, > u16 vid) > { > @@ -2437,6 +2568,12 @@ static int mv88e6xxx_port_vlan_leave(struct mv88e6xxx_chip *chip, > if (err) > return err; > > + if (!vlan.valid && vlan.sid) { > + err = mv88e6xxx_sid_put(chip, vlan.sid); > + if (err) > + return err; > + } > + > return mv88e6xxx_g1_atu_remove(chip, vlan.fid, port, false); > } > > @@ -2482,6 +2619,44 @@ static int mv88e6xxx_port_vlan_del(struct dsa_switch *ds, int port, > return err; > } > > +static int mv88e6xxx_vlan_msti_set(struct dsa_switch *ds, > + const struct switchdev_attr *attr) > +{ > + const struct switchdev_vlan_attr *vattr = &attr->u.vlan_attr; > + struct mv88e6xxx_chip *chip = ds->priv; > + struct mv88e6xxx_vtu_entry vlan; > + u8 new_sid; > + int err; > + > + mv88e6xxx_reg_lock(chip); > + > + err = mv88e6xxx_vtu_get(chip, vattr->vid, &vlan); > + if (err) > + goto unlock; > + > + if (!vlan.valid) { > + err = -EINVAL; > + goto unlock; > + } > + > + err = mv88e6xxx_sid_get(chip, attr->orig_dev, vattr->msti, &new_sid); > + if (err) > + goto unlock; > + > + if (vlan.sid) { > + err = mv88e6xxx_sid_put(chip, vlan.sid); > + if (err) > + goto unlock; > + } > + > + vlan.sid = new_sid; > + err = mv88e6xxx_vtu_loadpurge(chip, &vlan); Maybe you could move mv88e6xxx_sid_put() after this succeeds? > + > +unlock: > + mv88e6xxx_reg_unlock(chip); > + return err; > +} > + > static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port, > const unsigned char *addr, u16 vid, > struct dsa_db db) > @@ -6008,6 +6183,7 @@ static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev) > mutex_init(&chip->reg_lock); > INIT_LIST_HEAD(&chip->mdios); > idr_init(&chip->policies); > + INIT_LIST_HEAD(&chip->msts); > > return chip; > } > @@ -6540,10 +6716,12 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = { > .port_pre_bridge_flags = mv88e6xxx_port_pre_bridge_flags, > .port_bridge_flags = mv88e6xxx_port_bridge_flags, > .port_stp_state_set = mv88e6xxx_port_stp_state_set, > + .port_mst_state_set = mv88e6xxx_port_mst_state_set, > .port_fast_age = mv88e6xxx_port_fast_age, > .port_vlan_filtering = mv88e6xxx_port_vlan_filtering, > .port_vlan_add = mv88e6xxx_port_vlan_add, > .port_vlan_del = mv88e6xxx_port_vlan_del, > + .vlan_msti_set = mv88e6xxx_vlan_msti_set, > .port_fdb_add = mv88e6xxx_port_fdb_add, > .port_fdb_del = mv88e6xxx_port_fdb_del, > .port_fdb_dump = mv88e6xxx_port_fdb_dump, > diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h > index 6d4daa24d3e5..6a0b66354e1d 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.h > +++ b/drivers/net/dsa/mv88e6xxx/chip.h > @@ -297,6 +297,16 @@ struct mv88e6xxx_region_priv { > enum mv88e6xxx_region_id id; > }; > > +struct mv88e6xxx_mst { > + struct list_head node; > + > + refcount_t refcnt; > + struct net_device *br; > + u16 msti; > + > + struct mv88e6xxx_stu_entry stu; > +}; > + > struct mv88e6xxx_chip { > const struct mv88e6xxx_info *info; > > @@ -397,6 +407,9 @@ struct mv88e6xxx_chip { > > /* devlink regions */ > struct devlink_region *regions[_MV88E6XXX_REGION_MAX]; > + > + /* Bridge MST to SID mappings */ > + struct list_head msts; > }; > > struct mv88e6xxx_bus_ops { > -- > 2.25.1 >
On Fri, Mar 04, 2022 at 00:26, Vladimir Oltean <olteanv@gmail.com> wrote: > On Tue, Mar 01, 2022 at 11:03:21AM +0100, Tobias Waldekranz wrote: >> Allocate a SID in the STU for each MSTID in use by a bridge and handle >> the mapping of MSTIDs to VLANs using the SID field of each VTU entry. >> >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> >> --- >> drivers/net/dsa/mv88e6xxx/chip.c | 178 +++++++++++++++++++++++++++++++ >> drivers/net/dsa/mv88e6xxx/chip.h | 13 +++ >> 2 files changed, 191 insertions(+) >> >> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c >> index c14a62aa6a6c..4fb4ec1dff79 100644 >> --- a/drivers/net/dsa/mv88e6xxx/chip.c >> +++ b/drivers/net/dsa/mv88e6xxx/chip.c >> @@ -1818,6 +1818,137 @@ static int mv88e6xxx_stu_setup(struct mv88e6xxx_chip *chip) >> return mv88e6xxx_stu_loadpurge(chip, &stu); >> } >> >> +static int mv88e6xxx_sid_new(struct mv88e6xxx_chip *chip, u8 *sid) >> +{ >> + DECLARE_BITMAP(busy, MV88E6XXX_N_SID) = { 0 }; >> + struct mv88e6xxx_mst *mst; >> + >> + set_bit(0, busy); >> + >> + list_for_each_entry(mst, &chip->msts, node) { >> + set_bit(mst->stu.sid, busy); >> + } >> + >> + *sid = find_first_zero_bit(busy, MV88E6XXX_N_SID); >> + >> + return (*sid >= mv88e6xxx_max_sid(chip)) ? -ENOSPC : 0; >> +} >> + >> +static int mv88e6xxx_sid_put(struct mv88e6xxx_chip *chip, u8 sid) >> +{ >> + struct mv88e6xxx_mst *mst, *tmp; >> + int err = 0; >> + >> + list_for_each_entry_safe(mst, tmp, &chip->msts, node) { >> + if (mst->stu.sid == sid) { >> + if (refcount_dec_and_test(&mst->refcnt)) { >> + mst->stu.valid = false; >> + err = mv88e6xxx_stu_loadpurge(chip, &mst->stu); > > It is interesting what to do if this fails. Possibly not this, because > the entry remains in hardware but not in software. True, I will let the error bubble up and keep the SW state in sync with the hardware. >> + list_del(&mst->node); >> + kfree(mst); >> + } >> + >> + return err; >> + } >> + } >> + >> + return -ENOENT; >> +} >> + >> +static int mv88e6xxx_sid_get(struct mv88e6xxx_chip *chip, struct net_device *br, >> + u16 msti, u8 *sid) >> +{ >> + struct mv88e6xxx_mst *mst; >> + int err, i; >> + >> + if (!br) >> + return 0; > > Is this condition possible? Removing. >> + >> + if (!mv88e6xxx_has_stu(chip)) >> + return -EOPNOTSUPP; >> + >> + list_for_each_entry(mst, &chip->msts, node) { >> + if (mst->br == br && mst->msti == msti) { >> + refcount_inc(&mst->refcnt); >> + *sid = mst->stu.sid; >> + return 0; >> + } >> + } >> + >> + err = mv88e6xxx_sid_new(chip, sid); >> + if (err) >> + return err; >> + >> + mst = kzalloc(sizeof(*mst), GFP_KERNEL); >> + if (!mst) >> + return -ENOMEM; > > This leaks the new SID. I don't think so, the SID is just calculated based on what is in chip->msts. However: - The naming is bad. Will change. >> + >> + INIT_LIST_HEAD(&mst->node); >> + refcount_set(&mst->refcnt, 1); >> + mst->br = br; >> + mst->msti = msti; >> + mst->stu.valid = true; >> + mst->stu.sid = *sid; >> + >> + /* The bridge starts out all ports in the disabled state. But >> + * a STU state of disabled means to go by the port-global >> + * state. So we set all user port's initial state to blocking, >> + * to match the bridge's behavior. >> + */ >> + for (i = 0; i < mv88e6xxx_num_ports(chip); i++) >> + mst->stu.state[i] = dsa_is_user_port(chip->ds, i) ? >> + MV88E6XXX_PORT_CTL0_STATE_BLOCKING : >> + MV88E6XXX_PORT_CTL0_STATE_DISABLED; >> + >> + list_add_tail(&mst->node, &chip->msts); >> + return mv88e6xxx_stu_loadpurge(chip, &mst->stu); > > And this doesn't behave too well on failure (the MSTID exists in > software but not in hardware). Yes, fixing in v3. >> +} >> + >> +static int mv88e6xxx_port_mst_state_set(struct dsa_switch *ds, int port, >> + const struct switchdev_mst_state *st) >> +{ >> + struct dsa_port *dp = dsa_to_port(ds, port); >> + struct mv88e6xxx_chip *chip = ds->priv; >> + struct mv88e6xxx_mst *mst; >> + u8 state; >> + int err; >> + >> + if (!mv88e6xxx_has_stu(chip)) >> + return -EOPNOTSUPP; >> + >> + switch (st->state) { >> + case BR_STATE_DISABLED: >> + case BR_STATE_BLOCKING: >> + case BR_STATE_LISTENING: >> + state = MV88E6XXX_PORT_CTL0_STATE_BLOCKING; >> + break; >> + case BR_STATE_LEARNING: >> + state = MV88E6XXX_PORT_CTL0_STATE_LEARNING; >> + break; >> + case BR_STATE_FORWARDING: >> + state = MV88E6XXX_PORT_CTL0_STATE_FORWARDING; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + list_for_each_entry(mst, &chip->msts, node) { >> + if (mst->br == dsa_port_bridge_dev_get(dp) && >> + mst->msti == st->msti) { >> + if (mst->stu.state[port] == state) >> + return 0; >> + >> + mst->stu.state[port] = state; >> + mv88e6xxx_reg_lock(chip); >> + err = mv88e6xxx_stu_loadpurge(chip, &mst->stu); >> + mv88e6xxx_reg_unlock(chip); >> + return err; >> + } >> + } >> + >> + return -ENOENT; >> +} >> + >> static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port, >> u16 vid) >> { >> @@ -2437,6 +2568,12 @@ static int mv88e6xxx_port_vlan_leave(struct mv88e6xxx_chip *chip, >> if (err) >> return err; >> >> + if (!vlan.valid && vlan.sid) { >> + err = mv88e6xxx_sid_put(chip, vlan.sid); >> + if (err) >> + return err; >> + } >> + >> return mv88e6xxx_g1_atu_remove(chip, vlan.fid, port, false); >> } >> >> @@ -2482,6 +2619,44 @@ static int mv88e6xxx_port_vlan_del(struct dsa_switch *ds, int port, >> return err; >> } >> >> +static int mv88e6xxx_vlan_msti_set(struct dsa_switch *ds, >> + const struct switchdev_attr *attr) >> +{ >> + const struct switchdev_vlan_attr *vattr = &attr->u.vlan_attr; >> + struct mv88e6xxx_chip *chip = ds->priv; >> + struct mv88e6xxx_vtu_entry vlan; >> + u8 new_sid; >> + int err; >> + >> + mv88e6xxx_reg_lock(chip); >> + >> + err = mv88e6xxx_vtu_get(chip, vattr->vid, &vlan); >> + if (err) >> + goto unlock; >> + >> + if (!vlan.valid) { >> + err = -EINVAL; >> + goto unlock; >> + } >> + >> + err = mv88e6xxx_sid_get(chip, attr->orig_dev, vattr->msti, &new_sid); >> + if (err) >> + goto unlock; >> + >> + if (vlan.sid) { >> + err = mv88e6xxx_sid_put(chip, vlan.sid); >> + if (err) >> + goto unlock; >> + } >> + >> + vlan.sid = new_sid; >> + err = mv88e6xxx_vtu_loadpurge(chip, &vlan); > > Maybe you could move mv88e6xxx_sid_put() after this succeeds? Yep. Also made sure to avoid needless updates of the VTU entry if it already belonged to the correct SID. Thanks for the great review!
On Thu, Mar 10, 2022 at 04:14:31PM +0100, Tobias Waldekranz wrote: > On Fri, Mar 04, 2022 at 00:26, Vladimir Oltean <olteanv@gmail.com> wrote: > > On Tue, Mar 01, 2022 at 11:03:21AM +0100, Tobias Waldekranz wrote: > >> Allocate a SID in the STU for each MSTID in use by a bridge and handle > >> the mapping of MSTIDs to VLANs using the SID field of each VTU entry. > >> > >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > >> --- > >> drivers/net/dsa/mv88e6xxx/chip.c | 178 +++++++++++++++++++++++++++++++ > >> drivers/net/dsa/mv88e6xxx/chip.h | 13 +++ > >> 2 files changed, 191 insertions(+) > >> > >> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > >> index c14a62aa6a6c..4fb4ec1dff79 100644 > >> --- a/drivers/net/dsa/mv88e6xxx/chip.c > >> +++ b/drivers/net/dsa/mv88e6xxx/chip.c > >> @@ -1818,6 +1818,137 @@ static int mv88e6xxx_stu_setup(struct mv88e6xxx_chip *chip) > >> return mv88e6xxx_stu_loadpurge(chip, &stu); > >> } > >> > >> +static int mv88e6xxx_sid_new(struct mv88e6xxx_chip *chip, u8 *sid) > >> +{ > >> + DECLARE_BITMAP(busy, MV88E6XXX_N_SID) = { 0 }; > >> + struct mv88e6xxx_mst *mst; > >> + > >> + set_bit(0, busy); > >> + > >> + list_for_each_entry(mst, &chip->msts, node) { > >> + set_bit(mst->stu.sid, busy); > >> + } > >> + > >> + *sid = find_first_zero_bit(busy, MV88E6XXX_N_SID); > >> + > >> + return (*sid >= mv88e6xxx_max_sid(chip)) ? -ENOSPC : 0; > >> +} > >> + > >> +static int mv88e6xxx_sid_put(struct mv88e6xxx_chip *chip, u8 sid) > >> +{ > >> + struct mv88e6xxx_mst *mst, *tmp; > >> + int err = 0; > >> + > >> + list_for_each_entry_safe(mst, tmp, &chip->msts, node) { > >> + if (mst->stu.sid == sid) { > >> + if (refcount_dec_and_test(&mst->refcnt)) { > >> + mst->stu.valid = false; > >> + err = mv88e6xxx_stu_loadpurge(chip, &mst->stu); > > > > It is interesting what to do if this fails. Possibly not this, because > > the entry remains in hardware but not in software. > > True, I will let the error bubble up and keep the SW state in sync with > the hardware. Ok. For what it's worth, if you bump a refcount from 0 to 1 as part of the error handling here, you need to do so using refcount_set(1), not refcount_inc(). I found this out in commit 232deb3f9567 ("net: dsa: avoid refcount warnings when ->port_{fdb,mdb}_del returns error"). Just thought I'd mention it in case you didn't know, to avoid a future respin for that reason. > >> + list_del(&mst->node); > >> + kfree(mst); > >> + } > >> + > >> + return err; > >> + } > >> + } > >> + > >> + return -ENOENT; > >> +} > >> + > >> +static int mv88e6xxx_sid_get(struct mv88e6xxx_chip *chip, struct net_device *br, > >> + u16 msti, u8 *sid) > >> +{ > >> + struct mv88e6xxx_mst *mst; > >> + int err, i; > >> + > >> + if (!br) > >> + return 0; > > > > Is this condition possible? > > Removing. > > >> + > >> + if (!mv88e6xxx_has_stu(chip)) > >> + return -EOPNOTSUPP; > >> + > >> + list_for_each_entry(mst, &chip->msts, node) { > >> + if (mst->br == br && mst->msti == msti) { > >> + refcount_inc(&mst->refcnt); > >> + *sid = mst->stu.sid; > >> + return 0; > >> + } > >> + } > >> + > >> + err = mv88e6xxx_sid_new(chip, sid); > >> + if (err) > >> + return err; > >> + > >> + mst = kzalloc(sizeof(*mst), GFP_KERNEL); > >> + if (!mst) > >> + return -ENOMEM; > > > > This leaks the new SID. > > I don't think so, the SID is just calculated based on what is in > chip->msts. However: > > - The naming is bad. Will change. I see now. My bad. What are you renaming it to? If it isn't as concise you could still keep it sid_new(). I see atu_new() is based on the same find_first_zero_bit() concept. > >> + > >> + INIT_LIST_HEAD(&mst->node); > >> + refcount_set(&mst->refcnt, 1); > >> + mst->br = br; > >> + mst->msti = msti; > >> + mst->stu.valid = true; > >> + mst->stu.sid = *sid; > >> + > >> + /* The bridge starts out all ports in the disabled state. But > >> + * a STU state of disabled means to go by the port-global > >> + * state. So we set all user port's initial state to blocking, > >> + * to match the bridge's behavior. > >> + */ > >> + for (i = 0; i < mv88e6xxx_num_ports(chip); i++) > >> + mst->stu.state[i] = dsa_is_user_port(chip->ds, i) ? > >> + MV88E6XXX_PORT_CTL0_STATE_BLOCKING : > >> + MV88E6XXX_PORT_CTL0_STATE_DISABLED; > >> + > >> + list_add_tail(&mst->node, &chip->msts); > >> + return mv88e6xxx_stu_loadpurge(chip, &mst->stu); > > > > And this doesn't behave too well on failure (the MSTID exists in > > software but not in hardware). > > Yes, fixing in v3. > > >> +} > >> + > >> +static int mv88e6xxx_port_mst_state_set(struct dsa_switch *ds, int port, > >> + const struct switchdev_mst_state *st) > >> +{ > >> + struct dsa_port *dp = dsa_to_port(ds, port); > >> + struct mv88e6xxx_chip *chip = ds->priv; > >> + struct mv88e6xxx_mst *mst; > >> + u8 state; > >> + int err; > >> + > >> + if (!mv88e6xxx_has_stu(chip)) > >> + return -EOPNOTSUPP; > >> + > >> + switch (st->state) { > >> + case BR_STATE_DISABLED: > >> + case BR_STATE_BLOCKING: > >> + case BR_STATE_LISTENING: > >> + state = MV88E6XXX_PORT_CTL0_STATE_BLOCKING; > >> + break; > >> + case BR_STATE_LEARNING: > >> + state = MV88E6XXX_PORT_CTL0_STATE_LEARNING; > >> + break; > >> + case BR_STATE_FORWARDING: > >> + state = MV88E6XXX_PORT_CTL0_STATE_FORWARDING; > >> + break; > >> + default: > >> + return -EINVAL; > >> + } > >> + > >> + list_for_each_entry(mst, &chip->msts, node) { > >> + if (mst->br == dsa_port_bridge_dev_get(dp) && > >> + mst->msti == st->msti) { > >> + if (mst->stu.state[port] == state) > >> + return 0; > >> + > >> + mst->stu.state[port] = state; > >> + mv88e6xxx_reg_lock(chip); > >> + err = mv88e6xxx_stu_loadpurge(chip, &mst->stu); > >> + mv88e6xxx_reg_unlock(chip); > >> + return err; > >> + } > >> + } > >> + > >> + return -ENOENT; > >> +} > >> + > >> static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port, > >> u16 vid) > >> { > >> @@ -2437,6 +2568,12 @@ static int mv88e6xxx_port_vlan_leave(struct mv88e6xxx_chip *chip, > >> if (err) > >> return err; > >> > >> + if (!vlan.valid && vlan.sid) { > >> + err = mv88e6xxx_sid_put(chip, vlan.sid); > >> + if (err) > >> + return err; > >> + } > >> + > >> return mv88e6xxx_g1_atu_remove(chip, vlan.fid, port, false); > >> } > >> > >> @@ -2482,6 +2619,44 @@ static int mv88e6xxx_port_vlan_del(struct dsa_switch *ds, int port, > >> return err; > >> } > >> > >> +static int mv88e6xxx_vlan_msti_set(struct dsa_switch *ds, > >> + const struct switchdev_attr *attr) > >> +{ > >> + const struct switchdev_vlan_attr *vattr = &attr->u.vlan_attr; > >> + struct mv88e6xxx_chip *chip = ds->priv; > >> + struct mv88e6xxx_vtu_entry vlan; > >> + u8 new_sid; > >> + int err; > >> + > >> + mv88e6xxx_reg_lock(chip); > >> + > >> + err = mv88e6xxx_vtu_get(chip, vattr->vid, &vlan); > >> + if (err) > >> + goto unlock; > >> + > >> + if (!vlan.valid) { > >> + err = -EINVAL; > >> + goto unlock; > >> + } > >> + > >> + err = mv88e6xxx_sid_get(chip, attr->orig_dev, vattr->msti, &new_sid); > >> + if (err) > >> + goto unlock; > >> + > >> + if (vlan.sid) { > >> + err = mv88e6xxx_sid_put(chip, vlan.sid); > >> + if (err) > >> + goto unlock; > >> + } > >> + > >> + vlan.sid = new_sid; > >> + err = mv88e6xxx_vtu_loadpurge(chip, &vlan); > > > > Maybe you could move mv88e6xxx_sid_put() after this succeeds? > > Yep. Also made sure to avoid needless updates of the VTU entry if it > already belonged to the correct SID. > > Thanks for the great review! I realize I gave you conflicting advice here, first with inverting the refcount_inc() with the refcount_dec(), then with having fast handling of noop-changes to vlan.sid. I hope you're able to make some sense out of that and avoid the obvious issue with the refcount temporarily dropping to zero before going back to 1, which makes the sanity checker complain.
On Thu, Mar 10, 2022 at 05:25:47PM +0200, Vladimir Oltean wrote: > > >> + err = mv88e6xxx_sid_get(chip, attr->orig_dev, vattr->msti, &new_sid); > > >> + if (err) > > >> + goto unlock; > > >> + > > >> + if (vlan.sid) { > > >> + err = mv88e6xxx_sid_put(chip, vlan.sid); > > >> + if (err) > > >> + goto unlock; > > >> + } > > >> + > > >> + vlan.sid = new_sid; > > >> + err = mv88e6xxx_vtu_loadpurge(chip, &vlan); > > > > > > Maybe you could move mv88e6xxx_sid_put() after this succeeds? > > > > Yep. Also made sure to avoid needless updates of the VTU entry if it > > already belonged to the correct SID. > > I realize I gave you conflicting advice here, first with inverting the > refcount_inc() with the refcount_dec(), then with having fast handling > of noop-changes to vlan.sid. I hope you're able to make some sense out > of that and avoid the obvious issue with the refcount temporarily > dropping to zero before going back to 1, which makes the sanity checker > complain. Oh wow... I didn't look at the code again, and commented based on a false memory. Disregard, sorry. You aren't reversing sid_get with sid_put, nor did I suggest that. There's a lot that happened just in my head, apparently.
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index c14a62aa6a6c..4fb4ec1dff79 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -1818,6 +1818,137 @@ static int mv88e6xxx_stu_setup(struct mv88e6xxx_chip *chip) return mv88e6xxx_stu_loadpurge(chip, &stu); } +static int mv88e6xxx_sid_new(struct mv88e6xxx_chip *chip, u8 *sid) +{ + DECLARE_BITMAP(busy, MV88E6XXX_N_SID) = { 0 }; + struct mv88e6xxx_mst *mst; + + set_bit(0, busy); + + list_for_each_entry(mst, &chip->msts, node) { + set_bit(mst->stu.sid, busy); + } + + *sid = find_first_zero_bit(busy, MV88E6XXX_N_SID); + + return (*sid >= mv88e6xxx_max_sid(chip)) ? -ENOSPC : 0; +} + +static int mv88e6xxx_sid_put(struct mv88e6xxx_chip *chip, u8 sid) +{ + struct mv88e6xxx_mst *mst, *tmp; + int err = 0; + + list_for_each_entry_safe(mst, tmp, &chip->msts, node) { + if (mst->stu.sid == sid) { + if (refcount_dec_and_test(&mst->refcnt)) { + mst->stu.valid = false; + err = mv88e6xxx_stu_loadpurge(chip, &mst->stu); + list_del(&mst->node); + kfree(mst); + } + + return err; + } + } + + return -ENOENT; +} + +static int mv88e6xxx_sid_get(struct mv88e6xxx_chip *chip, struct net_device *br, + u16 msti, u8 *sid) +{ + struct mv88e6xxx_mst *mst; + int err, i; + + if (!br) + return 0; + + if (!mv88e6xxx_has_stu(chip)) + return -EOPNOTSUPP; + + list_for_each_entry(mst, &chip->msts, node) { + if (mst->br == br && mst->msti == msti) { + refcount_inc(&mst->refcnt); + *sid = mst->stu.sid; + return 0; + } + } + + err = mv88e6xxx_sid_new(chip, sid); + if (err) + return err; + + mst = kzalloc(sizeof(*mst), GFP_KERNEL); + if (!mst) + return -ENOMEM; + + INIT_LIST_HEAD(&mst->node); + refcount_set(&mst->refcnt, 1); + mst->br = br; + mst->msti = msti; + mst->stu.valid = true; + mst->stu.sid = *sid; + + /* The bridge starts out all ports in the disabled state. But + * a STU state of disabled means to go by the port-global + * state. So we set all user port's initial state to blocking, + * to match the bridge's behavior. + */ + for (i = 0; i < mv88e6xxx_num_ports(chip); i++) + mst->stu.state[i] = dsa_is_user_port(chip->ds, i) ? + MV88E6XXX_PORT_CTL0_STATE_BLOCKING : + MV88E6XXX_PORT_CTL0_STATE_DISABLED; + + list_add_tail(&mst->node, &chip->msts); + return mv88e6xxx_stu_loadpurge(chip, &mst->stu); +} + +static int mv88e6xxx_port_mst_state_set(struct dsa_switch *ds, int port, + const struct switchdev_mst_state *st) +{ + struct dsa_port *dp = dsa_to_port(ds, port); + struct mv88e6xxx_chip *chip = ds->priv; + struct mv88e6xxx_mst *mst; + u8 state; + int err; + + if (!mv88e6xxx_has_stu(chip)) + return -EOPNOTSUPP; + + switch (st->state) { + case BR_STATE_DISABLED: + case BR_STATE_BLOCKING: + case BR_STATE_LISTENING: + state = MV88E6XXX_PORT_CTL0_STATE_BLOCKING; + break; + case BR_STATE_LEARNING: + state = MV88E6XXX_PORT_CTL0_STATE_LEARNING; + break; + case BR_STATE_FORWARDING: + state = MV88E6XXX_PORT_CTL0_STATE_FORWARDING; + break; + default: + return -EINVAL; + } + + list_for_each_entry(mst, &chip->msts, node) { + if (mst->br == dsa_port_bridge_dev_get(dp) && + mst->msti == st->msti) { + if (mst->stu.state[port] == state) + return 0; + + mst->stu.state[port] = state; + mv88e6xxx_reg_lock(chip); + err = mv88e6xxx_stu_loadpurge(chip, &mst->stu); + mv88e6xxx_reg_unlock(chip); + return err; + } + } + + return -ENOENT; +} + static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port, u16 vid) { @@ -2437,6 +2568,12 @@ static int mv88e6xxx_port_vlan_leave(struct mv88e6xxx_chip *chip, if (err) return err; + if (!vlan.valid && vlan.sid) { + err = mv88e6xxx_sid_put(chip, vlan.sid); + if (err) + return err; + } + return mv88e6xxx_g1_atu_remove(chip, vlan.fid, port, false); } @@ -2482,6 +2619,44 @@ static int mv88e6xxx_port_vlan_del(struct dsa_switch *ds, int port, return err; } +static int mv88e6xxx_vlan_msti_set(struct dsa_switch *ds, + const struct switchdev_attr *attr) +{ + const struct switchdev_vlan_attr *vattr = &attr->u.vlan_attr; + struct mv88e6xxx_chip *chip = ds->priv; + struct mv88e6xxx_vtu_entry vlan; + u8 new_sid; + int err; + + mv88e6xxx_reg_lock(chip); + + err = mv88e6xxx_vtu_get(chip, vattr->vid, &vlan); + if (err) + goto unlock; + + if (!vlan.valid) { + err = -EINVAL; + goto unlock; + } + + err = mv88e6xxx_sid_get(chip, attr->orig_dev, vattr->msti, &new_sid); + if (err) + goto unlock; + + if (vlan.sid) { + err = mv88e6xxx_sid_put(chip, vlan.sid); + if (err) + goto unlock; + } + + vlan.sid = new_sid; + err = mv88e6xxx_vtu_loadpurge(chip, &vlan); + +unlock: + mv88e6xxx_reg_unlock(chip); + return err; +} + static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port, const unsigned char *addr, u16 vid, struct dsa_db db) @@ -6008,6 +6183,7 @@ static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev) mutex_init(&chip->reg_lock); INIT_LIST_HEAD(&chip->mdios); idr_init(&chip->policies); + INIT_LIST_HEAD(&chip->msts); return chip; } @@ -6540,10 +6716,12 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = { .port_pre_bridge_flags = mv88e6xxx_port_pre_bridge_flags, .port_bridge_flags = mv88e6xxx_port_bridge_flags, .port_stp_state_set = mv88e6xxx_port_stp_state_set, + .port_mst_state_set = mv88e6xxx_port_mst_state_set, .port_fast_age = mv88e6xxx_port_fast_age, .port_vlan_filtering = mv88e6xxx_port_vlan_filtering, .port_vlan_add = mv88e6xxx_port_vlan_add, .port_vlan_del = mv88e6xxx_port_vlan_del, + .vlan_msti_set = mv88e6xxx_vlan_msti_set, .port_fdb_add = mv88e6xxx_port_fdb_add, .port_fdb_del = mv88e6xxx_port_fdb_del, .port_fdb_dump = mv88e6xxx_port_fdb_dump, diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index 6d4daa24d3e5..6a0b66354e1d 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -297,6 +297,16 @@ struct mv88e6xxx_region_priv { enum mv88e6xxx_region_id id; }; +struct mv88e6xxx_mst { + struct list_head node; + + refcount_t refcnt; + struct net_device *br; + u16 msti; + + struct mv88e6xxx_stu_entry stu; +}; + struct mv88e6xxx_chip { const struct mv88e6xxx_info *info; @@ -397,6 +407,9 @@ struct mv88e6xxx_chip { /* devlink regions */ struct devlink_region *regions[_MV88E6XXX_REGION_MAX]; + + /* Bridge MST to SID mappings */ + struct list_head msts; }; struct mv88e6xxx_bus_ops {
Allocate a SID in the STU for each MSTID in use by a bridge and handle the mapping of MSTIDs to VLANs using the SID field of each VTU entry. Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- drivers/net/dsa/mv88e6xxx/chip.c | 178 +++++++++++++++++++++++++++++++ drivers/net/dsa/mv88e6xxx/chip.h | 13 +++ 2 files changed, 191 insertions(+)