[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240308095442.ug4pmh4entgah5l3@skbuf>
Date: Fri, 8 Mar 2024 11:54:42 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Pawel Dembicki <paweldembicki@...il.com>
Cc: netdev@...r.kernel.org, Linus Walleij <linus.walleij@...aro.org>,
Simon Horman <horms@...nel.org>, Andrew Lunn <andrew@...n.ch>,
Florian Fainelli <f.fainelli@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Claudiu Manoil <claudiu.manoil@....com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
UNGLinuxDriver@...rochip.com, Russell King <linux@...linux.org.uk>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v6 06/16] net: dsa: vsc73xx: add
port_stp_state_set function
On Fri, Mar 01, 2024 at 11:16:28PM +0100, Pawel Dembicki wrote:
> This isn't a fully functional implementation of 802.1D, but
> port_stp_state_set is required for a future tag8021q operations.
>
> This implementation handles properly all states, but vsc73xx doesn't
> forward STP packets.
>
> Signed-off-by: Pawel Dembicki <paweldembicki@...il.com>
> ---
> v6:
> - fix inconsistent indenting
> v5:
> - remove unneeded 'RECVMASK' operations
> - reorganise vsc73xx_refresh_fwd_map function
> v4:
> - fully reworked port_stp_state_set
> v3:
> - use 'VSC73XX_MAX_NUM_PORTS' define
> - add 'state == BR_STATE_DISABLED' condition
> - fix style issues
> v2:
> - fix kdoc
>
> drivers/net/dsa/vitesse-vsc73xx-core.c | 99 +++++++++++++++++++++++---
> 1 file changed, 88 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index 425999d7bf41..d1e84a9a83d1 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -1036,6 +1029,89 @@ static void vsc73xx_phylink_get_caps(struct dsa_switch *dsa, int port,
> config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000;
> }
>
> +static void vsc73xx_refresh_fwd_map(struct dsa_switch *ds, int port, u8 state)
> +{
> + struct dsa_port *other_dp, *dp = dsa_to_port(ds, port);
> + struct vsc73xx *vsc = ds->priv;
> + u16 mask;
> +
> + if (state != BR_STATE_FORWARDING) {
> + /* Ports that aren't in the forwarding state must not
> + * forward packets anywhere.
> + */
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + port,
> + VSC73XX_SRCMASKS_PORTS_MASK, 0);
> +
> + dsa_switch_for_each_available_port(other_dp, ds) {
> + if (other_dp == dp)
> + continue;
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + other_dp->index,
> + BIT(port), 0);
> + }
> +
> + return;
> + }
> +
> + /* Forwarding ports must forward to the CPU and to other ports
> + * in the same bridge
> + */
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + CPU_PORT, BIT(port), BIT(port));
> +
> + mask = BIT(CPU_PORT);
> +
> + if (dp->bridge) {
> + dsa_switch_for_each_user_port(other_dp, ds) {
> + if (other_dp->bridge == dp->bridge &&
You could use dsa_port_bridge_same(dp, other_dp) and that could
eliminate the extra "if (dp->bridge)" condition, because it explicitly
makes standalone ports isolated from other standalone ports.
> + other_dp->index != port &&
You could move the "int other_port" definition to dsa_switch_for_each_user_port()
scope, and thus reuse it here.
> + other_dp->stp_state == BR_STATE_FORWARDING) {
You could "continue" on the negated condition, and reduce the
indentation one level further.
> + int other_port = other_dp->index;
> +
> + mask |= BIT(other_port);
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER,
> + 0,
> + VSC73XX_SRCMASKS +
> + other_port,
> + BIT(port), BIT(port));
> + }
> + }
> + }
All in all, I would have written this as:
dsa_switch_for_each_user_port(other_dp, ds) {
int other_port = other_dp->index;
if (port == other_port || !dsa_port_bridge_same(dp, other_dp) ||
other_dp->stp_state != BR_STATE_FORWARDING)
continue;
mask |= BIT(other_port);
vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
VSC73XX_SRCMASKS + other_port,
BIT(port), BIT(port));
}
Anyway this does not affect functionality, and it is up to you if you
integrate these suggestions or not.
Reviewed-by: Vladimir Oltean <olteanv@...il.com>
> +
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> + VSC73XX_SRCMASKS + port,
> + VSC73XX_SRCMASKS_PORTS_MASK, mask);
> +}
Powered by blists - more mailing lists