All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ido Schimmel <idosch@idosch.org>
To: Tobias Waldekranz <tobias@waldekranz.com>
Cc: davem@davemloft.net, kuba@kernel.org,
	Nikolay Aleksandrov <razor@blackwall.org>,
	Andrew Lunn <andrew@lunn.ch>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Vladimir Oltean <olteanv@gmail.com>,
	Jiri Pirko <jiri@resnulli.us>, Ivan Vecera <ivecera@redhat.com>,
	Roopa Prabhu <roopa@nvidia.com>,
	Russell King <linux@armlinux.org.uk>,
	Petr Machata <petrm@nvidia.com>, Ido Schimmel <idosch@nvidia.com>,
	Matt Johnston <matt@codeconstruct.com.au>,
	Cooper Lees <me@cooperlees.com>,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	bridge@lists.linux-foundation.org
Subject: Re: [PATCH v5 net-next 01/15] net: bridge: mst: Multiple Spanning Tree (MST) mode
Date: Mon, 9 Jan 2023 10:05:53 +0200	[thread overview]
Message-ID: <Y7vK4T18pOZ9KAKE@shredder> (raw)
In-Reply-To: <20220316150857.2442916-2-tobias@waldekranz.com>

On Wed, Mar 16, 2022 at 04:08:43PM +0100, Tobias Waldekranz wrote:
> +DEFINE_STATIC_KEY_FALSE(br_mst_used);

[...]

> +int br_mst_set_enabled(struct net_bridge *br, bool on,
> +		       struct netlink_ext_ack *extack)
> +{
> +	struct net_bridge_vlan_group *vg;
> +	struct net_bridge_port *p;
> +
> +	list_for_each_entry(p, &br->port_list, list) {
> +		vg = nbp_vlan_group(p);
> +
> +		if (!vg->num_vlans)
> +			continue;
> +
> +		NL_SET_ERR_MSG(extack,
> +			       "MST mode can't be changed while VLANs exist");
> +		return -EBUSY;
> +	}
> +
> +	if (br_opt_get(br, BROPT_MST_ENABLED) == on)
> +		return 0;
> +
> +	if (on)
> +		static_branch_enable(&br_mst_used);
> +	else
> +		static_branch_disable(&br_mst_used);
> +
> +	br_opt_toggle(br, BROPT_MST_ENABLED, on);
> +	return 0;
> +}

Hi,

I'm not actually using MST, but I ran into this code and was wondering
if the static key usage is correct. The static key is global (not
per-bridge), so what happens when two bridges have MST enabled and then
it is disabled on one? I believe it would be disabled for both. If so,
maybe use static_branch_inc() / static_branch_dec() instead?

Thanks

WARNING: multiple messages have this Message-ID (diff)
From: Ido Schimmel <idosch@idosch.org>
To: Tobias Waldekranz <tobias@waldekranz.com>
Cc: Ivan Vecera <ivecera@redhat.com>, Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Jiri Pirko <jiri@resnulli.us>, Petr Machata <petrm@nvidia.com>,
	Nikolay Aleksandrov <razor@blackwall.org>,
	bridge@lists.linux-foundation.org,
	Russell King <linux@armlinux.org.uk>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Ido Schimmel <idosch@nvidia.com>,
	netdev@vger.kernel.org, Cooper Lees <me@cooperlees.com>,
	Roopa Prabhu <roopa@nvidia.com>,
	kuba@kernel.org, Matt Johnston <matt@codeconstruct.com.au>,
	Vladimir Oltean <olteanv@gmail.com>,
	davem@davemloft.net, linux-kernel@vger.kernel.org
Subject: Re: [Bridge] [PATCH v5 net-next 01/15] net: bridge: mst: Multiple Spanning Tree (MST) mode
Date: Mon, 9 Jan 2023 10:05:53 +0200	[thread overview]
Message-ID: <Y7vK4T18pOZ9KAKE@shredder> (raw)
In-Reply-To: <20220316150857.2442916-2-tobias@waldekranz.com>

On Wed, Mar 16, 2022 at 04:08:43PM +0100, Tobias Waldekranz wrote:
> +DEFINE_STATIC_KEY_FALSE(br_mst_used);

[...]

> +int br_mst_set_enabled(struct net_bridge *br, bool on,
> +		       struct netlink_ext_ack *extack)
> +{
> +	struct net_bridge_vlan_group *vg;
> +	struct net_bridge_port *p;
> +
> +	list_for_each_entry(p, &br->port_list, list) {
> +		vg = nbp_vlan_group(p);
> +
> +		if (!vg->num_vlans)
> +			continue;
> +
> +		NL_SET_ERR_MSG(extack,
> +			       "MST mode can't be changed while VLANs exist");
> +		return -EBUSY;
> +	}
> +
> +	if (br_opt_get(br, BROPT_MST_ENABLED) == on)
> +		return 0;
> +
> +	if (on)
> +		static_branch_enable(&br_mst_used);
> +	else
> +		static_branch_disable(&br_mst_used);
> +
> +	br_opt_toggle(br, BROPT_MST_ENABLED, on);
> +	return 0;
> +}

Hi,

I'm not actually using MST, but I ran into this code and was wondering
if the static key usage is correct. The static key is global (not
per-bridge), so what happens when two bridges have MST enabled and then
it is disabled on one? I believe it would be disabled for both. If so,
maybe use static_branch_inc() / static_branch_dec() instead?

Thanks

  reply	other threads:[~2023-01-09  8:06 UTC|newest]

Thread overview: 78+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-16 15:08 [PATCH v5 net-next 00/15] net: bridge: Multiple Spanning Trees Tobias Waldekranz
2022-03-16 15:08 ` [Bridge] " Tobias Waldekranz
2022-03-16 15:08 ` [PATCH v5 net-next 01/15] net: bridge: mst: Multiple Spanning Tree (MST) mode Tobias Waldekranz
2022-03-16 15:08   ` [Bridge] " Tobias Waldekranz
2023-01-09  8:05   ` Ido Schimmel [this message]
2023-01-09  8:05     ` Ido Schimmel
2023-01-09 10:02     ` Vladimir Oltean
2023-01-09 10:02       ` [Bridge] " Vladimir Oltean
2023-01-09 11:43       ` Ido Schimmel
2023-01-09 11:43         ` [Bridge] " Ido Schimmel
2023-01-09 11:51         ` Nikolay Aleksandrov
2023-01-09 11:51           ` [Bridge] " Nikolay Aleksandrov
2023-01-09 11:56         ` Vladimir Oltean
2023-01-09 11:56           ` [Bridge] " Vladimir Oltean
2023-01-09 12:20           ` Ido Schimmel
2023-01-09 12:20             ` [Bridge] " Ido Schimmel
2023-01-09 12:29             ` Vladimir Oltean
2023-01-09 12:29               ` [Bridge] " Vladimir Oltean
2022-03-16 15:08 ` [PATCH v5 net-next 02/15] net: bridge: mst: Allow changing a VLAN's MSTI Tobias Waldekranz
2022-03-16 15:08   ` [Bridge] " Tobias Waldekranz
2022-03-16 15:08 ` [PATCH v5 net-next 03/15] net: bridge: mst: Support setting and reporting MST port states Tobias Waldekranz
2022-03-16 15:08   ` [Bridge] " Tobias Waldekranz
2022-03-17  8:55   ` Nikolay Aleksandrov
2022-03-17  8:55     ` [Bridge] " Nikolay Aleksandrov
2022-03-16 15:08 ` [PATCH v5 net-next 04/15] net: bridge: mst: Notify switchdev drivers of MST mode changes Tobias Waldekranz
2022-03-16 15:08   ` [Bridge] " Tobias Waldekranz
2022-03-17  8:56   ` Nikolay Aleksandrov
2022-03-17  8:56     ` [Bridge] " Nikolay Aleksandrov
2022-03-16 15:08 ` [PATCH v5 net-next 05/15] net: bridge: mst: Notify switchdev drivers of VLAN MSTI migrations Tobias Waldekranz
2022-03-16 15:08   ` [Bridge] " Tobias Waldekranz
2022-03-17  8:57   ` Nikolay Aleksandrov
2022-03-17  8:57     ` [Bridge] " Nikolay Aleksandrov
2022-03-16 15:08 ` [PATCH v5 net-next 06/15] net: bridge: mst: Notify switchdev drivers of MST state changes Tobias Waldekranz
2022-03-16 15:08   ` [Bridge] " Tobias Waldekranz
2022-03-17  8:57   ` Nikolay Aleksandrov
2022-03-17  8:57     ` [Bridge] " Nikolay Aleksandrov
2022-03-16 15:08 ` [PATCH v5 net-next 07/15] net: bridge: mst: Add helper to map an MSTI to a VID set Tobias Waldekranz
2022-03-16 15:08   ` [Bridge] " Tobias Waldekranz
2022-03-17  0:43   ` Vladimir Oltean
2022-03-17  0:43     ` [Bridge] " Vladimir Oltean
2022-03-17  9:01   ` Nikolay Aleksandrov
2022-03-17  9:01     ` [Bridge] " Nikolay Aleksandrov
2022-03-16 15:08 ` [PATCH v5 net-next 08/15] net: bridge: mst: Add helper to check if MST is enabled Tobias Waldekranz
2022-03-16 15:08   ` [Bridge] " Tobias Waldekranz
2022-03-17  0:42   ` Vladimir Oltean
2022-03-17  0:42     ` [Bridge] " Vladimir Oltean
2022-03-17  9:01   ` Nikolay Aleksandrov
2022-03-17  9:01     ` [Bridge] " Nikolay Aleksandrov
2022-03-16 15:08 ` [PATCH v5 net-next 09/15] net: bridge: mst: Add helper to query a port's MST state Tobias Waldekranz
2022-03-16 15:08   ` [Bridge] " Tobias Waldekranz
2022-03-17  0:41   ` Vladimir Oltean
2022-03-17  0:41     ` [Bridge] " Vladimir Oltean
2022-03-17  9:01   ` Nikolay Aleksandrov
2022-03-17  9:01     ` [Bridge] " Nikolay Aleksandrov
2022-03-16 15:08 ` [PATCH v5 net-next 10/15] net: dsa: Validate hardware support for MST Tobias Waldekranz
2022-03-16 15:08   ` [Bridge] " Tobias Waldekranz
2022-03-16 15:59   ` Vladimir Oltean
2022-03-16 15:59     ` [Bridge] " Vladimir Oltean
2022-03-16 15:08 ` [PATCH v5 net-next 11/15] net: dsa: Pass VLAN MSTI migration notifications to driver Tobias Waldekranz
2022-03-16 15:08   ` [Bridge] " Tobias Waldekranz
2022-03-16 15:08 ` [PATCH v5 net-next 12/15] net: dsa: Handle MST state changes Tobias Waldekranz
2022-03-16 15:08   ` [Bridge] " Tobias Waldekranz
2022-03-16 15:56   ` Vladimir Oltean
2022-03-16 15:56     ` [Bridge] " Vladimir Oltean
2022-03-16 15:08 ` [PATCH v5 net-next 13/15] net: dsa: mv88e6xxx: Disentangle STU from VTU Tobias Waldekranz
2022-03-16 15:08   ` [Bridge] " Tobias Waldekranz
2022-03-16 15:08 ` [PATCH v5 net-next 14/15] net: dsa: mv88e6xxx: Export STU as devlink region Tobias Waldekranz
2022-03-16 15:08   ` [Bridge] " Tobias Waldekranz
2022-03-16 15:08 ` [PATCH v5 net-next 15/15] net: dsa: mv88e6xxx: MST Offloading Tobias Waldekranz
2022-03-16 15:08   ` [Bridge] " Tobias Waldekranz
2022-03-17  9:00 ` [PATCH v5 net-next 00/15] net: bridge: Multiple Spanning Trees Nikolay Aleksandrov
2022-03-17  9:00   ` [Bridge] " Nikolay Aleksandrov
2022-03-17  9:50   ` Tobias Waldekranz
2022-03-17  9:50     ` [Bridge] " Tobias Waldekranz
2022-03-17  9:56     ` Nikolay Aleksandrov
2022-03-17  9:56       ` [Bridge] " Nikolay Aleksandrov
2022-03-18  0:20 ` patchwork-bot+netdevbpf
2022-03-18  0:20   ` [Bridge] " patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y7vK4T18pOZ9KAKE@shredder \
    --to=idosch@idosch.org \
    --cc=andrew@lunn.ch \
    --cc=bridge@lists.linux-foundation.org \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=idosch@nvidia.com \
    --cc=ivecera@redhat.com \
    --cc=jiri@resnulli.us \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=matt@codeconstruct.com.au \
    --cc=me@cooperlees.com \
    --cc=netdev@vger.kernel.org \
    --cc=olteanv@gmail.com \
    --cc=petrm@nvidia.com \
    --cc=razor@blackwall.org \
    --cc=roopa@nvidia.com \
    --cc=tobias@waldekranz.com \
    --cc=vivien.didelot@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.