Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 10 Mar 2022 17:25:47 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Tobias Waldekranz <tobias@waldekranz.com>
Cc:     davem@davemloft.net, kuba@kernel.org, Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Jiri Pirko <jiri@resnulli.us>,
        Ivan Vecera <ivecera@redhat.com>,
        Roopa Prabhu <roopa@nvidia.com>,
        Nikolay Aleksandrov <razor@blackwall.org>,
        Russell King <linux@armlinux.org.uk>,
        Petr Machata <petrm@nvidia.com>,
        Cooper Lees <me@cooperlees.com>,
        Ido Schimmel <idosch@nvidia.com>,
        Matt Johnston <matt@codeconstruct.com.au>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        bridge@lists.linux-foundation.org
Subject: Re: [PATCH v2 net-next 10/10] net: dsa: mv88e6xxx: MST Offloading
Message-ID: <20220310152547.etuov6kpqotnyv2p@skbuf>
References: <20220301100321.951175-1-tobias@waldekranz.com>
 <20220301100321.951175-11-tobias@waldekranz.com>
 <20220303222658.7ykn6grkkp6htm7a@skbuf>
 <87k0d1n8ko.fsf@waldekranz.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87k0d1n8ko.fsf@waldekranz.com>
Precedence: bulk

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.