diff mbox series

[net,2/3] nfp: flower: prevent re-adding mac index for bonded port

Message ID 20240202113719.16171-3-louis.peens@corigine.com (mailing list archive)
State Accepted
Commit 1a1c13303ff6d64e6f718dc8aa614e580ca8d9b4
Delegated to: Netdev Maintainers
Headers show
Series nfp: a few simple driver fixes | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/ynl success SINGLE THREAD; Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1064 this patch: 1064
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 1081 this patch: 1081
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1081 this patch: 1081
netdev/checkpatch warning WARNING: line length of 97 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-02-04--21-00 (tests: 721)

Commit Message

Louis Peens Feb. 2, 2024, 11:37 a.m. UTC 
From: Daniel de Villiers <daniel.devilliers@corigine.com>

When physical ports are reset (either through link failure or manually
toggled down and up again) that are slaved to a Linux bond with a tunnel
endpoint IP address on the bond device, not all tunnel packets arriving
on the bond port are decapped as expected.

The bond dev assigns the same MAC address to itself and each of its
slaves. When toggling a slave device, the same MAC address is therefore
offloaded to the NFP multiple times with different indexes.

The issue only occurs when re-adding the shared mac. The
nfp_tunnel_add_shared_mac() function has a conditional check early on
that checks if a mac entry already exists and if that mac entry is
global: (entry && nfp_tunnel_is_mac_idx_global(entry->index)). In the
case of a bonded device (For example br-ex), the mac index is obtained,
and no new index is assigned.

We therefore modify the conditional in nfp_tunnel_add_shared_mac() to
check if the port belongs to the LAG along with the existing checks to
prevent a new global mac index from being re-assigned to the slave port.

Fixes: 20cce8865098 ("nfp: flower: enable MAC address sharing for offloadable devs")
CC: stable@vger.kernel.org # 5.1+
Signed-off-by: Daniel de Villiers <daniel.devilliers@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
---
 drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Simon Horman Feb. 5, 2024, 1:32 p.m. UTC | #1 
On Fri, Feb 02, 2024 at 01:37:18PM +0200, Louis Peens wrote:
> From: Daniel de Villiers <daniel.devilliers@corigine.com>
> 
> When physical ports are reset (either through link failure or manually
> toggled down and up again) that are slaved to a Linux bond with a tunnel
> endpoint IP address on the bond device, not all tunnel packets arriving
> on the bond port are decapped as expected.
> 
> The bond dev assigns the same MAC address to itself and each of its
> slaves. When toggling a slave device, the same MAC address is therefore
> offloaded to the NFP multiple times with different indexes.
> 
> The issue only occurs when re-adding the shared mac. The
> nfp_tunnel_add_shared_mac() function has a conditional check early on
> that checks if a mac entry already exists and if that mac entry is
> global: (entry && nfp_tunnel_is_mac_idx_global(entry->index)). In the
> case of a bonded device (For example br-ex), the mac index is obtained,
> and no new index is assigned.
> 
> We therefore modify the conditional in nfp_tunnel_add_shared_mac() to
> check if the port belongs to the LAG along with the existing checks to
> prevent a new global mac index from being re-assigned to the slave port.
> 
> Fixes: 20cce8865098 ("nfp: flower: enable MAC address sharing for offloadable devs")
> CC: stable@vger.kernel.org # 5.1+
> Signed-off-by: Daniel de Villiers <daniel.devilliers@corigine.com>
> Signed-off-by: Louis Peens <louis.peens@corigine.com>

Hi Daniel and Louis,

I'd like to encourage you to update the wording of the commit message
to use more inclusive language; I'd suggest describing the patch
in terms of members of a LAG.

The code-change looks good to me.

> ---
>  drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> index e522845c7c21..0d7d138d6e0d 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> @@ -1084,7 +1084,7 @@ nfp_tunnel_add_shared_mac(struct nfp_app *app, struct net_device *netdev,
>  	u16 nfp_mac_idx = 0;
>  
>  	entry = nfp_tunnel_lookup_offloaded_macs(app, netdev->dev_addr);
> -	if (entry && nfp_tunnel_is_mac_idx_global(entry->index)) {
> +	if (entry && (nfp_tunnel_is_mac_idx_global(entry->index) || netif_is_lag_port(netdev))) {
>  		if (entry->bridge_count ||
>  		    !nfp_flower_is_supported_bridge(netdev)) {
>  			nfp_tunnel_offloaded_macs_inc_ref_and_link(entry,
> -- 
> 2.34.1
> 
>
Louis Peens Feb. 5, 2024, 2:15 p.m. UTC | #2 
On Mon, Feb 05, 2024 at 01:32:03PM +0000, Simon Horman wrote:
> On Fri, Feb 02, 2024 at 01:37:18PM +0200, Louis Peens wrote:
> > From: Daniel de Villiers <daniel.devilliers@corigine.com>
> > 
> > When physical ports are reset (either through link failure or manually
> > toggled down and up again) that are slaved to a Linux bond with a tunnel
> > endpoint IP address on the bond device, not all tunnel packets arriving
> > on the bond port are decapped as expected.
> > 
> > The bond dev assigns the same MAC address to itself and each of its
> > slaves. When toggling a slave device, the same MAC address is therefore
> > offloaded to the NFP multiple times with different indexes.
> > 
> > The issue only occurs when re-adding the shared mac. The
> > nfp_tunnel_add_shared_mac() function has a conditional check early on
> > that checks if a mac entry already exists and if that mac entry is
> > global: (entry && nfp_tunnel_is_mac_idx_global(entry->index)). In the
> > case of a bonded device (For example br-ex), the mac index is obtained,
> > and no new index is assigned.
> > 
> > We therefore modify the conditional in nfp_tunnel_add_shared_mac() to
> > check if the port belongs to the LAG along with the existing checks to
> > prevent a new global mac index from being re-assigned to the slave port.
> > 
> > Fixes: 20cce8865098 ("nfp: flower: enable MAC address sharing for offloadable devs")
> > CC: stable@vger.kernel.org # 5.1+
> > Signed-off-by: Daniel de Villiers <daniel.devilliers@corigine.com>
> > Signed-off-by: Louis Peens <louis.peens@corigine.com>
> 
> Hi Daniel and Louis,
> 
> I'd like to encourage you to update the wording of the commit message
> to use more inclusive language; I'd suggest describing the patch
> in terms of members of a LAG.
Thanks Simon, this have not even crossed my mind this time and I feel
bad - I should be more aware. Thanks for politely pointing this out.
This did get merged earlier today as-is unfortunately, I'm not sure if
there is a good way (or if it is pressing enough) to have it retracted.
I will try to be more cognizant of this in the future.
> 
> The code-change looks good to me.
>
Simon Horman Feb. 5, 2024, 5:58 p.m. UTC | #3 
On Mon, Feb 05, 2024 at 04:15:08PM +0200, Louis Peens wrote:
> On Mon, Feb 05, 2024 at 01:32:03PM +0000, Simon Horman wrote:
> > On Fri, Feb 02, 2024 at 01:37:18PM +0200, Louis Peens wrote:
> > > From: Daniel de Villiers <daniel.devilliers@corigine.com>
> > > 
> > > When physical ports are reset (either through link failure or manually
> > > toggled down and up again) that are slaved to a Linux bond with a tunnel
> > > endpoint IP address on the bond device, not all tunnel packets arriving
> > > on the bond port are decapped as expected.
> > > 
> > > The bond dev assigns the same MAC address to itself and each of its
> > > slaves. When toggling a slave device, the same MAC address is therefore
> > > offloaded to the NFP multiple times with different indexes.
> > > 
> > > The issue only occurs when re-adding the shared mac. The
> > > nfp_tunnel_add_shared_mac() function has a conditional check early on
> > > that checks if a mac entry already exists and if that mac entry is
> > > global: (entry && nfp_tunnel_is_mac_idx_global(entry->index)). In the
> > > case of a bonded device (For example br-ex), the mac index is obtained,
> > > and no new index is assigned.
> > > 
> > > We therefore modify the conditional in nfp_tunnel_add_shared_mac() to
> > > check if the port belongs to the LAG along with the existing checks to
> > > prevent a new global mac index from being re-assigned to the slave port.
> > > 
> > > Fixes: 20cce8865098 ("nfp: flower: enable MAC address sharing for offloadable devs")
> > > CC: stable@vger.kernel.org # 5.1+
> > > Signed-off-by: Daniel de Villiers <daniel.devilliers@corigine.com>
> > > Signed-off-by: Louis Peens <louis.peens@corigine.com>
> > 
> > Hi Daniel and Louis,
> > 
> > I'd like to encourage you to update the wording of the commit message
> > to use more inclusive language; I'd suggest describing the patch
> > in terms of members of a LAG.
> Thanks Simon, this have not even crossed my mind this time and I feel
> bad - I should be more aware. Thanks for politely pointing this out.
> This did get merged earlier today as-is unfortunately, I'm not sure if
> there is a good way (or if it is pressing enough) to have it retracted.
> I will try to be more cognizant of this in the future.

Hi Louis,

thanks for acknowledging my concern.

Given that the patch has been applied,
I think it would be best to do as you suggest,
and keep this in mind for next time.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
index e522845c7c21..0d7d138d6e0d 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
@@ -1084,7 +1084,7 @@  nfp_tunnel_add_shared_mac(struct nfp_app *app, struct net_device *netdev,
 	u16 nfp_mac_idx = 0;
 
 	entry = nfp_tunnel_lookup_offloaded_macs(app, netdev->dev_addr);
-	if (entry && nfp_tunnel_is_mac_idx_global(entry->index)) {
+	if (entry && (nfp_tunnel_is_mac_idx_global(entry->index) || netif_is_lag_port(netdev))) {
 		if (entry->bridge_count ||
 		    !nfp_flower_is_supported_bridge(netdev)) {
 			nfp_tunnel_offloaded_macs_inc_ref_and_link(entry,