[PATCH net 0/4] mlxsw: Various fixes

[PATCH net 0/4] mlxsw: Various fixes

[Ido Schimmel]

From: Ido Schimmel <idosch@mellanox.com>

This patch set contains various fixes for mlxsw.

Patch #1 splits the init() callback between Spectrum-2 and Spectrum-3 in
order to avoid enforcing the same firmware version for both ASICs, as
this can't possibly work. Without this patch the driver cannot boot with
the Spectrum-3 ASIC.

Patches #2-#3 from Shalom fix a long standing race condition that was
recently exposed while testing the driver on an emulator, which is very
slow compared to the actual hardware. The problem is explained in detail
in the commit message.

Patch #4 from Petr fixes a selftest.

Ido Schimmel (1):
  mlxsw: spectrum: Do not enforce same firmware version for multiple
    ASICs

Petr Machata (1):
  selftests: mlxsw: qos_mc_aware: Fix mausezahn invocation

Shalom Toledo (2):
  mlxsw: switchx2: Do not modify cloned SKBs during xmit
  mlxsw: spectrum: Do not modify cloned SKBs during xmit

 .../net/ethernet/mellanox/mlxsw/spectrum.c    | 27 ++++++++++++++++++-
 .../net/ethernet/mellanox/mlxsw/switchx2.c    |  4 +++
 .../drivers/net/mlxsw/qos_mc_aware.sh         |  8 ++++--
 3 files changed, 36 insertions(+), 3 deletions(-)

-- 
2.24.1

[PATCH net 1/4] mlxsw: spectrum: Do not enforce same firmware version for multiple ASICs

[Ido Schimmel]

From: Ido Schimmel <idosch@mellanox.com>

In commit a72afb6879bb ("mlxsw: Enforce firmware version for
Spectrum-2") I added a required firmware version for Spectrum-2, but
missed the fact that mlxsw_sp2_init() is used by both Spectrum-2 and
Spectrum-3. This means that the same firmware version will be used for
both, which is wrong.

Fix this by creating a new init() callback for Spectrum-3.

Fixes: a72afb6879bb ("mlxsw: Enforce firmware version for Spectrum-2")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Tested-by: Shalom Toledo <shalomt@mellanox.com>
---
 .../net/ethernet/mellanox/mlxsw/spectrum.c    | 23 ++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index f7fd5e8fbf96..5408a964bd10 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -5132,6 +5132,27 @@ static int mlxsw_sp2_init(struct mlxsw_core *mlxsw_core,
 	return mlxsw_sp_init(mlxsw_core, mlxsw_bus_info, extack);
 }
 
+static int mlxsw_sp3_init(struct mlxsw_core *mlxsw_core,
+			  const struct mlxsw_bus_info *mlxsw_bus_info,
+			  struct netlink_ext_ack *extack)
+{
+	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
+
+	mlxsw_sp->kvdl_ops = &mlxsw_sp2_kvdl_ops;
+	mlxsw_sp->afa_ops = &mlxsw_sp2_act_afa_ops;
+	mlxsw_sp->afk_ops = &mlxsw_sp2_afk_ops;
+	mlxsw_sp->mr_tcam_ops = &mlxsw_sp2_mr_tcam_ops;
+	mlxsw_sp->acl_tcam_ops = &mlxsw_sp2_acl_tcam_ops;
+	mlxsw_sp->nve_ops_arr = mlxsw_sp2_nve_ops_arr;
+	mlxsw_sp->mac_mask = mlxsw_sp2_mac_mask;
+	mlxsw_sp->rif_ops_arr = mlxsw_sp2_rif_ops_arr;
+	mlxsw_sp->sb_vals = &mlxsw_sp2_sb_vals;
+	mlxsw_sp->port_type_speed_ops = &mlxsw_sp2_port_type_speed_ops;
+	mlxsw_sp->ptp_ops = &mlxsw_sp2_ptp_ops;
+
+	return mlxsw_sp_init(mlxsw_core, mlxsw_bus_info, extack);
+}
+
 static void mlxsw_sp_fini(struct mlxsw_core *mlxsw_core)
 {
 	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
@@ -5634,7 +5655,7 @@ static struct mlxsw_driver mlxsw_sp2_driver = {
 static struct mlxsw_driver mlxsw_sp3_driver = {
 	.kind				= mlxsw_sp3_driver_name,
 	.priv_size			= sizeof(struct mlxsw_sp),
-	.init				= mlxsw_sp2_init,
+	.init				= mlxsw_sp3_init,
 	.fini				= mlxsw_sp_fini,
 	.basic_trap_groups_set		= mlxsw_sp_basic_trap_groups_set,
 	.port_split			= mlxsw_sp_port_split,
-- 
2.24.1

[PATCH net 2/4] mlxsw: switchx2: Do not modify cloned SKBs during xmit

[Ido Schimmel]

From: Shalom Toledo <shalomt@mellanox.com>

The driver needs to prepend a Tx header to each packet it is transmitting.
The header includes information such as the egress port and traffic class.

The addition of the header requires the driver to modify the SKB's data
buffer and therefore the SKB must be unshared first. Otherwise, we risk
hitting various race conditions with cloned SKBs.

For example, when a packet is flooded (cloned) by the bridge driver to two
switch ports swp1 and swp2:

t0 - mlxsw_sp_port_xmit() is called for swp1. Tx header is prepended with
     swp1's port number
t1 - mlxsw_sp_port_xmit() is called for swp2. Tx header is prepended with
     swp2's port number, overwriting swp1's port number
t2 - The device processes data buffer from t0. Packet is transmitted via
     swp2
t3 - The device processes data buffer from t1. Packet is transmitted via
     swp2

Usually, the device is fast enough and transmits the packet before its
Tx header is overwritten, but this is not the case in emulated
environments.

Fix this by unsharing the SKB.

Fixes: 31557f0f9755 ("mlxsw: Introduce Mellanox SwitchX-2 ASIC support")
Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/switchx2.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/switchx2.c b/drivers/net/ethernet/mellanox/mlxsw/switchx2.c
index de6cb22f68b1..47826e905e5c 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/switchx2.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/switchx2.c
@@ -299,6 +299,10 @@ static netdev_tx_t mlxsw_sx_port_xmit(struct sk_buff *skb,
 	u64 len;
 	int err;
 
+	skb = skb_unshare(skb, GFP_ATOMIC);
+	if (unlikely(!skb))
+		return NETDEV_TX_BUSY;
+
 	memset(skb->cb, 0, sizeof(struct mlxsw_skb_cb));
 
 	if (mlxsw_core_skb_transmit_busy(mlxsw_sx->core, &tx_info))
-- 
2.24.1

Re: [PATCH net 2/4] mlxsw: switchx2: Do not modify cloned SKBs during xmit

[Jakub Kicinski]

On Sun, 12 Jan 2020 18:06:39 +0200, Ido Schimmel wrote:
> From: Shalom Toledo <shalomt@mellanox.com>
> 
> The driver needs to prepend a Tx header to each packet it is transmitting.
> The header includes information such as the egress port and traffic class.
> 
> The addition of the header requires the driver to modify the SKB's data
> buffer and therefore the SKB must be unshared first. Otherwise, we risk
> hitting various race conditions with cloned SKBs.
> 
> For example, when a packet is flooded (cloned) by the bridge driver to two
> switch ports swp1 and swp2:
> 
> t0 - mlxsw_sp_port_xmit() is called for swp1. Tx header is prepended with
>      swp1's port number
> t1 - mlxsw_sp_port_xmit() is called for swp2. Tx header is prepended with
>      swp2's port number, overwriting swp1's port number
> t2 - The device processes data buffer from t0. Packet is transmitted via
>      swp2
> t3 - The device processes data buffer from t1. Packet is transmitted via
>      swp2
> 
> Usually, the device is fast enough and transmits the packet before its
> Tx header is overwritten, but this is not the case in emulated
> environments.
> 
> Fix this by unsharing the SKB.

Isn't this what skb_cow_head() is for?

> diff --git a/drivers/net/ethernet/mellanox/mlxsw/switchx2.c b/drivers/net/ethernet/mellanox/mlxsw/switchx2.c
> index de6cb22f68b1..47826e905e5c 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/switchx2.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/switchx2.c
> @@ -299,6 +299,10 @@ static netdev_tx_t mlxsw_sx_port_xmit(struct sk_buff *skb,
>  	u64 len;
>  	int err;
>  
> +	skb = skb_unshare(skb, GFP_ATOMIC);
> +	if (unlikely(!skb))
> +		return NETDEV_TX_BUSY;
> +
>  	memset(skb->cb, 0, sizeof(struct mlxsw_skb_cb));
>  
>  	if (mlxsw_core_skb_transmit_busy(mlxsw_sx->core, &tx_info))

the next line here is:

		return NETDEV_TX_BUSY;

Is it okay to return BUSY after copying an skb? The reference to the
original skb may already be gone at this point, while the copy is going
to be leaked, right?

Re: [PATCH net 2/4] mlxsw: switchx2: Do not modify cloned SKBs during xmit

[Ido Schimmel]

On Sun, Jan 12, 2020 at 04:10:17PM -0800, Jakub Kicinski wrote:
> On Sun, 12 Jan 2020 18:06:39 +0200, Ido Schimmel wrote:
> > From: Shalom Toledo <shalomt@mellanox.com>
> > 
> > The driver needs to prepend a Tx header to each packet it is transmitting.
> > The header includes information such as the egress port and traffic class.
> > 
> > The addition of the header requires the driver to modify the SKB's data
> > buffer and therefore the SKB must be unshared first. Otherwise, we risk
> > hitting various race conditions with cloned SKBs.
> > 
> > For example, when a packet is flooded (cloned) by the bridge driver to two
> > switch ports swp1 and swp2:
> > 
> > t0 - mlxsw_sp_port_xmit() is called for swp1. Tx header is prepended with
> >      swp1's port number
> > t1 - mlxsw_sp_port_xmit() is called for swp2. Tx header is prepended with
> >      swp2's port number, overwriting swp1's port number
> > t2 - The device processes data buffer from t0. Packet is transmitted via
> >      swp2
> > t3 - The device processes data buffer from t1. Packet is transmitted via
> >      swp2
> > 
> > Usually, the device is fast enough and transmits the packet before its
> > Tx header is overwritten, but this is not the case in emulated
> > environments.
> > 
> > Fix this by unsharing the SKB.
> 
> Isn't this what skb_cow_head() is for?

Yes, this does look better. If you look further in the code, we have
this check for the headroom:

if (unlikely(skb_headroom(skb) < MLXSW_TXHDR_LEN)) {
...
}

We can remove it by replacing skb_unshare() with skb_cow_head().

> 
> > diff --git a/drivers/net/ethernet/mellanox/mlxsw/switchx2.c b/drivers/net/ethernet/mellanox/mlxsw/switchx2.c
> > index de6cb22f68b1..47826e905e5c 100644
> > --- a/drivers/net/ethernet/mellanox/mlxsw/switchx2.c
> > +++ b/drivers/net/ethernet/mellanox/mlxsw/switchx2.c
> > @@ -299,6 +299,10 @@ static netdev_tx_t mlxsw_sx_port_xmit(struct sk_buff *skb,
> >  	u64 len;
> >  	int err;
> >  
> > +	skb = skb_unshare(skb, GFP_ATOMIC);
> > +	if (unlikely(!skb))
> > +		return NETDEV_TX_BUSY;
> > +
> >  	memset(skb->cb, 0, sizeof(struct mlxsw_skb_cb));
> >  
> >  	if (mlxsw_core_skb_transmit_busy(mlxsw_sx->core, &tx_info))
> 
> the next line here is:
> 
> 		return NETDEV_TX_BUSY;
> 
> Is it okay to return BUSY after copying an skb? The reference to the
> original skb may already be gone at this point, while the copy is going
> to be leaked, right?

Yes, you're correct, but if we convert to skb_cow_head() like you
suggested, then the skb shell is not changed and only its header is
(potentially) expanded, so I believe we can keep this check as-is.

Thanks, Jakub!

P.S. I'll take care of v2 as Shalom is OOO until next week.

[PATCH net 3/4] mlxsw: spectrum: Do not modify cloned SKBs during xmit

[Ido Schimmel]

From: Shalom Toledo <shalomt@mellanox.com>

The driver needs to prepend a Tx header to each packet it is transmitting.
The header includes information such as the egress port and traffic class.

The addition of the header requires the driver to modify the SKB's data
buffer and therefore the SKB must be unshared first. Otherwise, we risk
hitting various race conditions with cloned SKBs.

For example, when a packet is flooded (cloned) by the bridge driver to two
switch ports swp1 and swp2:

t0 - mlxsw_sp_port_xmit() is called for swp1. Tx header is prepended with
     swp1's port number
t1 - mlxsw_sp_port_xmit() is called for swp2. Tx header is prepended with
     swp2's port number, overwriting swp1's port number
t2 - The device processes data buffer from t0. Packet is transmitted via
     swp2
t3 - The device processes data buffer from t1. Packet is transmitted via
     swp2

Usually, the device is fast enough and transmits the packet before its
Tx header is overwritten, but this is not the case in emulated
environments.

Fix this by unsharing the SKB.

Fixes: 56ade8fe3fe1 ("mlxsw: spectrum: Add initial support for Spectrum ASIC")
Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 5408a964bd10..6c7bf93dd804 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -860,6 +860,10 @@ static netdev_tx_t mlxsw_sp_port_xmit(struct sk_buff *skb,
 	u64 len;
 	int err;
 
+	skb = skb_unshare(skb, GFP_ATOMIC);
+	if (unlikely(!skb))
+		return NETDEV_TX_BUSY;
+
 	memset(skb->cb, 0, sizeof(struct mlxsw_skb_cb));
 
 	if (mlxsw_core_skb_transmit_busy(mlxsw_sp->core, &tx_info))
-- 
2.24.1

[PATCH net 4/4] selftests: mlxsw: qos_mc_aware: Fix mausezahn invocation

[Ido Schimmel]

From: Petr Machata <petrm@mellanox.com>

Mausezahn does not recognize "own" as a keyword on source IP address. As a
result, the MC stream is not running at all, and therefore no UC
degradation can be observed even in principle.

Fix the invocation, and tighten the test: due to the minimum shaper
configured at the MC TCs, we always expect about 20% degradation. Fail the
test if it is lower.

Fixes: 573363a68f27 ("selftests: mlxsw: Add qos_lib.sh")
Signed-off-by: Petr Machata <petrm@mellanox.com>
Reported-by: Amit Cohen <amitc@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 tools/testing/selftests/drivers/net/mlxsw/qos_mc_aware.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/mlxsw/qos_mc_aware.sh b/tools/testing/selftests/drivers/net/mlxsw/qos_mc_aware.sh
index 47315fe48d5a..24dd8ed48580 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/qos_mc_aware.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/qos_mc_aware.sh
@@ -232,7 +232,7 @@ test_mc_aware()
 	stop_traffic
 	local ucth1=${uc_rate[1]}
 
-	start_traffic $h1 own bc bc
+	start_traffic $h1 192.0.2.65 bc bc
 
 	local d0=$(date +%s)
 	local t0=$(ethtool_stats_get $h3 rx_octets_prio_0)
@@ -254,7 +254,11 @@ test_mc_aware()
 			ret = 100 * ($ucth1 - $ucth2) / $ucth1
 			if (ret > 0) { ret } else { 0 }
 		    ")
-	check_err $(bc <<< "$deg > 25")
+
+	# Minimum shaper of 200Mbps on MC TCs should cause about 20% of
+	# degradation on 1Gbps link.
+	check_err $(bc <<< "$deg < 15") "Minimum shaper not in effect"
+	check_err $(bc <<< "$deg > 25") "MC traffic degrades UC performance too much"
 
 	local interval=$((d1 - d0))
 	local mc_ir=$(rate $u0 $u1 $interval)
-- 
2.24.1