Tobias Waldekranz
2022-Mar-10 15:14 UTC
[Bridge] [PATCH v2 net-next 10/10] net: dsa: mv88e6xxx: MST Offloading
On Fri, Mar 04, 2022 at 00:26, Vladimir Oltean <olteanv at 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 at 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!
Vladimir Oltean
2022-Mar-10 15:25 UTC
[Bridge] [PATCH v2 net-next 10/10] net: dsa: mv88e6xxx: MST Offloading
On Thu, Mar 10, 2022 at 04:14:31PM +0100, Tobias Waldekranz wrote:> On Fri, Mar 04, 2022 at 00:26, Vladimir Oltean <olteanv at 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 at 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.