[PATCH net 0/3] nfp: a few simple driver fixes

[PATCH net 0/3] nfp: a few simple driver fixes

[Louis Peens]

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(-)

-- 
2.34.1

[PATCH net 1/3] nfp: use correct macro for LengthSelect in BAR config

[Louis Peens]

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] = {
-- 
2.34.1

Re: [PATCH net 1/3] nfp: use correct macro for LengthSelect in BAR config

[Simon Horman]

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);
>  	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] = {
> -- 
> 2.34.1
> 
> 

Re: [PATCH net 1/3] nfp: use correct macro for LengthSelect in BAR config

[Louis Peens]

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.

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

[Louis Peens]

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,
-- 
2.34.1

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

[Simon Horman]

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
> 
> 

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

[Louis Peens]

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.
> 

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

[Simon Horman]

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.

[PATCH net 3/3] nfp: enable NETDEV_XDP_ACT_REDIRECT feature flag

[Louis Peens]

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;
-- 
2.34.1

Re: [PATCH net 3/3] nfp: enable NETDEV_XDP_ACT_REDIRECT feature flag

[Simon Horman]

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>

Re: [PATCH net 0/3] nfp: a few simple driver fixes

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html