This is combining a few unrelated one-liner fixes which have been floating around internally into a single series. I'm not sure what is the least amount of overhead for reviewers, this or a separate submission per-patch? I guess it probably depends on personal preference, but please let me know if there is a strong preference to rather split these in the future.
Summary:
Patch1: Fixes an old issue which was hidden because 0 just so happens to be the correct value. Patch2: Fixes a corner case for flower offloading with bond ports Patch3: Re-enables the 'NETDEV_XDP_ACT_REDIRECT', which was accidentally disabled after a previous refactor.
Daniel Basilio (1): nfp: use correct macro for LengthSelect in BAR config
Daniel de Villiers (1): nfp: flower: prevent re-adding mac index for bonded port
James Hershaw (1): nfp: enable NETDEV_XDP_ACT_REDIRECT feature flag
drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c | 2 +- drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 1 + drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c | 6 ++++-- 3 files changed, 6 insertions(+), 3 deletions(-)
From: Daniel Basilio daniel.basilio@corigine.com
The 1st and 2nd expansion BAR configuration registers are configured, when the driver starts up, in variables 'barcfg_msix_general' and 'barcfg_msix_xpb', respectively. The 'LengthSelect' field is ORed in from bit 0, which is incorrect. The 'LengthSelect' field should start from bit 27.
This has largely gone un-noticed because NFP_PCIE_BAR_PCIE2CPP_LengthSelect_32BIT happens to be 0.
Fixes: 4cb584e0ee7d ("nfp: add CPP access core") Cc: stable@vger.kernel.org # 4.11+ Signed-off-by: Daniel Basilio daniel.basilio@corigine.com Signed-off-by: Louis Peens louis.peens@corigine.com --- drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c index 33b4c2856316..3f10c5365c80 100644 --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c @@ -537,11 +537,13 @@ static int enable_bars(struct nfp6000_pcie *nfp, u16 interface) const u32 barcfg_msix_general = NFP_PCIE_BAR_PCIE2CPP_MapType( NFP_PCIE_BAR_PCIE2CPP_MapType_GENERAL) | - NFP_PCIE_BAR_PCIE2CPP_LengthSelect_32BIT; + NFP_PCIE_BAR_PCIE2CPP_LengthSelect( + NFP_PCIE_BAR_PCIE2CPP_LengthSelect_32BIT); const u32 barcfg_msix_xpb = NFP_PCIE_BAR_PCIE2CPP_MapType( NFP_PCIE_BAR_PCIE2CPP_MapType_BULK) | - NFP_PCIE_BAR_PCIE2CPP_LengthSelect_32BIT | + NFP_PCIE_BAR_PCIE2CPP_LengthSelect( + NFP_PCIE_BAR_PCIE2CPP_LengthSelect_32BIT) | NFP_PCIE_BAR_PCIE2CPP_Target_BaseAddress( NFP_CPP_TARGET_ISLAND_XPB); const u32 barcfg_explicit[4] = {
On Fri, Feb 02, 2024 at 01:37:17PM +0200, Louis Peens wrote:
From: Daniel Basilio daniel.basilio@corigine.com
The 1st and 2nd expansion BAR configuration registers are configured, when the driver starts up, in variables 'barcfg_msix_general' and 'barcfg_msix_xpb', respectively. The 'LengthSelect' field is ORed in from bit 0, which is incorrect. The 'LengthSelect' field should start from bit 27.
This has largely gone un-noticed because NFP_PCIE_BAR_PCIE2CPP_LengthSelect_32BIT happens to be 0.
Fixes: 4cb584e0ee7d ("nfp: add CPP access core") Cc: stable@vger.kernel.org # 4.11+ Signed-off-by: Daniel Basilio daniel.basilio@corigine.com Signed-off-by: Louis Peens louis.peens@corigine.com
Hi Daniel and Louis,
If I'm reading this right then this is a code-correctness issue and there is no runtime effect (because 0 is 0 regardless of shifting and masking).
If so, I'd suggest that this is net-next material. And, in turn, if so the Fixes tag should be dropped.
drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c index 33b4c2856316..3f10c5365c80 100644 --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c @@ -537,11 +537,13 @@ static int enable_bars(struct nfp6000_pcie *nfp, u16 interface) const u32 barcfg_msix_general = NFP_PCIE_BAR_PCIE2CPP_MapType( NFP_PCIE_BAR_PCIE2CPP_MapType_GENERAL) |
NFP_PCIE_BAR_PCIE2CPP_LengthSelect_32BIT;
NFP_PCIE_BAR_PCIE2CPP_LengthSelect(NFP_PCIE_BAR_PCIE2CPP_LengthSelect_32BIT);
NFP_PCIE_BAR_PCIE2CPP_LengthSelect_32BIT |
NFP_PCIE_BAR_PCIE2CPP_LengthSelect(NFP_PCIE_BAR_PCIE2CPP_LengthSelect_32BIT) |-- 2.34.1
On Mon, Feb 05, 2024 at 01:35:45PM +0000, Simon Horman wrote:
On Fri, Feb 02, 2024 at 01:37:17PM +0200, Louis Peens wrote:
From: Daniel Basilio daniel.basilio@corigine.com
The 1st and 2nd expansion BAR configuration registers are configured, when the driver starts up, in variables 'barcfg_msix_general' and 'barcfg_msix_xpb', respectively. The 'LengthSelect' field is ORed in from bit 0, which is incorrect. The 'LengthSelect' field should start from bit 27.
This has largely gone un-noticed because NFP_PCIE_BAR_PCIE2CPP_LengthSelect_32BIT happens to be 0.
Fixes: 4cb584e0ee7d ("nfp: add CPP access core") Cc: stable@vger.kernel.org # 4.11+ Signed-off-by: Daniel Basilio daniel.basilio@corigine.com Signed-off-by: Louis Peens louis.peens@corigine.com
Hi Daniel and Louis,
If I'm reading this right then this is a code-correctness issue and there is no runtime effect (because 0 is 0 regardless of shifting and masking).
You are reading this correctly yes.
If so, I'd suggest that this is net-next material. And, in turn, if so the Fixes tag should be dropped.
Thanks Simon. I was definitely flip-flopping on which tree to pick when preparing this, if not already merged I would have gladly dropped it from this net series. Thinking of it in terms of runtime effect is probably a useful angle, will try and do this more when picking a tree.
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(-)
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,
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
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.
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.
From: James Hershaw james.hershaw@corigine.com
Enable previously excluded xdp feature flag for NFD3 devices. This feature flag is required in order to bind nfp interfaces to an xdp socket and the nfp driver does in fact support the feature.
Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") Cc: stable@vger.kernel.org # 6.3+ Signed-off-by: James Hershaw james.hershaw@corigine.com Signed-off-by: Louis Peens louis.peens@corigine.com --- drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c index 3b3210d823e8..f28e769e6fda 100644 --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c @@ -2776,6 +2776,7 @@ static void nfp_net_netdev_init(struct nfp_net *nn) case NFP_NFD_VER_NFD3: netdev->netdev_ops = &nfp_nfd3_netdev_ops; netdev->xdp_features |= NETDEV_XDP_ACT_XSK_ZEROCOPY; + netdev->xdp_features |= NETDEV_XDP_ACT_REDIRECT; break; case NFP_NFD_VER_NFDK: netdev->netdev_ops = &nfp_nfdk_netdev_ops;
On Fri, Feb 02, 2024 at 01:37:19PM +0200, Louis Peens wrote:
From: James Hershaw james.hershaw@corigine.com
Enable previously excluded xdp feature flag for NFD3 devices. This feature flag is required in order to bind nfp interfaces to an xdp socket and the nfp driver does in fact support the feature.
Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") Cc: stable@vger.kernel.org # 6.3+ Signed-off-by: James Hershaw james.hershaw@corigine.com Signed-off-by: Louis Peens louis.peens@corigine.com
Reviewed-by: Simon Horman horms@kernel.org
Hello:
This series was applied to netdev/net.git (main) by David S. Miller davem@davemloft.net:
On Fri, 2 Feb 2024 13:37:16 +0200 you wrote:
This is combining a few unrelated one-liner fixes which have been floating around internally into a single series. I'm not sure what is the least amount of overhead for reviewers, this or a separate submission per-patch? I guess it probably depends on personal preference, but please let me know if there is a strong preference to rather split these in the future.
[...]
Here is the summary with links: - [net,1/3] nfp: use correct macro for LengthSelect in BAR config https://git.kernel.org/netdev/net/c/b3d4f7f22889 - [net,2/3] nfp: flower: prevent re-adding mac index for bonded port https://git.kernel.org/netdev/net/c/1a1c13303ff6 - [net,3/3] nfp: enable NETDEV_XDP_ACT_REDIRECT feature flag https://git.kernel.org/netdev/net/c/0f4d6f011bca
You are awesome, thank you!
linux-stable-mirror@lists.linaro.org
-
Louis Peens -
patchwork-bot+netdevbpf＠kernel.org -
Simon Horman