[PATCH net 0/2] mlx4 fixes for 4.20-rc

[PATCH net 0/2] mlx4 fixes for 4.20-rc

[Tariq Toukan]

Hi Dave,

This patchset includes small fixes for the mlx4_en driver.

First patch by Eran fixes the value used to init the netdevice's
min_mtu field.
Please queue it to -stable >= v4.10.

Second patch by Saeed adds missing Kconfig build dependencies.

Series generated against net commit:
35b827b6d061 tun: forbid iface creation with rtnl ops

Thanks,
Tariq.


Eran Ben Elisha (1):
  net/mlx4_en: Change min MTU size to ETH_MIN_MTU

Saeed Mahameed (1):
  net/mlx4_en: Fix build break when CONFIG_INET is off

 drivers/net/ethernet/mellanox/mlx4/Kconfig     | 2 +-
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 4 ++--
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h   | 1 -
 3 files changed, 3 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[PATCH net 1/2] net/mlx4_en: Change min MTU size to ETH_MIN_MTU

[Tariq Toukan]

From: Eran Ben Elisha <eranbe@mellanox.com>

NIC driver minimal MTU size shall be set to ETH_MIN_MTU, as defined in
the RFC791 and in the network stack. Remove old mlx4_en only define for
it, which was set to wrong value.

Fixes: b80f71f5816f ("ethernet/mellanox: use core min/max MTU checking")
Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 4 ++--
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h   | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index b744cd49a785..6b88881b8e35 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -3493,8 +3493,8 @@ int mlx4_en_init_netdev(struct mlx4_en_dev *mdev, int port,
 		dev->gso_partial_features = NETIF_F_GSO_UDP_TUNNEL_CSUM;
 	}
 
-	/* MTU range: 46 - hw-specific max */
-	dev->min_mtu = MLX4_EN_MIN_MTU;
+	/* MTU range: 68 - hw-specific max */
+	dev->min_mtu = ETH_MIN_MTU;
 	dev->max_mtu = priv->max_mtu;
 
 	mdev->pndev[port] = dev;
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index 485d856546c6..8137454e2534 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -161,7 +161,6 @@
 #define MLX4_SELFTEST_LB_MIN_MTU (MLX4_LOOPBACK_TEST_PAYLOAD + NET_IP_ALIGN + \
 				  ETH_HLEN + PREAMBLE_LEN)
 
-#define MLX4_EN_MIN_MTU		46
 /* VLAN_HLEN is added twice,to support skb vlan tagged with multiple
  * headers. (For example: ETH_P_8021Q and ETH_P_8021AD).
  */
-- 
1.8.3.1

RE: [PATCH net 1/2] net/mlx4_en: Change min MTU size to ETH_MIN_MTU

[David Laight]

From: Tariq Toukan
> Sent: 02 December 2018 12:35
> From: Eran Ben Elisha <eranbe@mellanox.com>
> 
> NIC driver minimal MTU size shall be set to ETH_MIN_MTU, as defined in
> the RFC791 and in the network stack. Remove old mlx4_en only define for
> it, which was set to wrong value.
...
> 
> -	/* MTU range: 46 - hw-specific max */
> -	dev->min_mtu = MLX4_EN_MIN_MTU;
> +	/* MTU range: 68 - hw-specific max */
> +	dev->min_mtu = ETH_MIN_MTU;
>  	dev->max_mtu = priv->max_mtu;

Where does 68 come from?
The minimum size of an ethernet packet including the mac addresses
and CRC is 64 bytes - but that would never be an 'mtu'.

Since 64 - 46 = 18, the 46 probably excludes both MAC addresses,
the ethertype/length and the CRC.
This is 'sort of' the minimum mtu for an ethernet frame.

I'm not sure which values are supposed to be in dev->min/max_mtu.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH net 1/2] net/mlx4_en: Change min MTU size to ETH_MIN_MTU

[Eric Dumazet]

On 12/04/2018 08:59 AM, David Laight wrote:
> From: Tariq Toukan
>> Sent: 02 December 2018 12:35
>> From: Eran Ben Elisha <eranbe@mellanox.com>
>>
>> NIC driver minimal MTU size shall be set to ETH_MIN_MTU, as defined in
>> the RFC791 and in the network stack. Remove old mlx4_en only define for
>> it, which was set to wrong value.
> ...
>>
>> -	/* MTU range: 46 - hw-specific max */
>> -	dev->min_mtu = MLX4_EN_MIN_MTU;
>> +	/* MTU range: 68 - hw-specific max */
>> +	dev->min_mtu = ETH_MIN_MTU;
>>  	dev->max_mtu = priv->max_mtu;
> 
> Where does 68 come from?

Min IPv4 MTU per RFC791

> The minimum size of an ethernet packet including the mac addresses
> and CRC is 64 bytes - but that would never be an 'mtu'.
> 
> Since 64 - 46 = 18, the 46 probably excludes both MAC addresses,
> the ethertype/length and the CRC.
> This is 'sort of' the minimum mtu for an ethernet frame.
> 
> I'm not sure which values are supposed to be in dev->min/max_mtu.

I am not sure we really care, only syzkaller can possibly.

RE: [PATCH net 1/2] net/mlx4_en: Change min MTU size to ETH_MIN_MTU

[David Laight]

From: Eric Dumazet
> Sent: 04 December 2018 17:04
> 
> On 12/04/2018 08:59 AM, David Laight wrote:
> > From: Tariq Toukan
> >> Sent: 02 December 2018 12:35
> >> From: Eran Ben Elisha <eranbe@mellanox.com>
> >>
> >> NIC driver minimal MTU size shall be set to ETH_MIN_MTU, as defined in
> >> the RFC791 and in the network stack. Remove old mlx4_en only define for
> >> it, which was set to wrong value.
> > ...
> >>
> >> -	/* MTU range: 46 - hw-specific max */
> >> -	dev->min_mtu = MLX4_EN_MIN_MTU;
> >> +	/* MTU range: 68 - hw-specific max */
> >> +	dev->min_mtu = ETH_MIN_MTU;
> >>  	dev->max_mtu = priv->max_mtu;
> >
> > Where does 68 come from?
> 
> Min IPv4 MTU per RFC791

Which has nothing to do with an ethernet driver.
Indeed, IIRC, it is the smallest maximum frame size that IPv4
can work over.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH net 1/2] net/mlx4_en: Change min MTU size to ETH_MIN_MTU

[David Laight]

From: Eric Dumazet
> Sent: 04 December 2018 17:04
> On 12/04/2018 08:59 AM, David Laight wrote:
> > From: Tariq Toukan
> >> Sent: 02 December 2018 12:35
> >> From: Eran Ben Elisha <eranbe@mellanox.com>
> >>
> >> NIC driver minimal MTU size shall be set to ETH_MIN_MTU, as defined in
> >> the RFC791 and in the network stack. Remove old mlx4_en only define for
> >> it, which was set to wrong value.
> > ...
> >>
> >> -	/* MTU range: 46 - hw-specific max */
> >> -	dev->min_mtu = MLX4_EN_MIN_MTU;
> >> +	/* MTU range: 68 - hw-specific max */
> >> +	dev->min_mtu = ETH_MIN_MTU;
> >>  	dev->max_mtu = priv->max_mtu;
> >
> > Where does 68 come from?
> 
> Min IPv4 MTU per RFC791

Maybe I'm just confused and these are the ranges that the 'maximum mtu'
can be set to.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH net 1/2] net/mlx4_en: Change min MTU size to ETH_MIN_MTU

[Cong Wang]

On Tue, Dec 4, 2018 at 9:06 AM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 12/04/2018 08:59 AM, David Laight wrote:
> > Where does 68 come from?
>
> Min IPv4 MTU per RFC791
>

What's wrong with using IPV4_MIN_MTU instead of 68
even in just comment?

[PATCH net 2/2] net/mlx4_en: Fix build break when CONFIG_INET is off

[Tariq Toukan]

From: Saeed Mahameed <saeedm@mellanox.com>

MLX4_EN depends on NETDEVICES, ETHERNET and INET Kconfigs.
Make sure they are listed in MLX4_EN Kconfig dependencies.

This fixes the following build break:

drivers/net/ethernet/mellanox/mlx4/en_rx.c:582:18: warning: ‘struct iphdr’ declared inside parameter list [enabled by default]
struct iphdr *iph)
^
drivers/net/ethernet/mellanox/mlx4/en_rx.c:582:18: warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default]
drivers/net/ethernet/mellanox/mlx4/en_rx.c: In function ‘get_fixed_ipv4_csum’:
drivers/net/ethernet/mellanox/mlx4/en_rx.c:586:20: error: dereferencing pointer to incomplete type
_u8 ipproto = iph->protocol;

Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/Kconfig b/drivers/net/ethernet/mellanox/mlx4/Kconfig
index 36054e6fb9d3..f200b8c420d5 100644
--- a/drivers/net/ethernet/mellanox/mlx4/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlx4/Kconfig
@@ -5,7 +5,7 @@
 config MLX4_EN
 	tristate "Mellanox Technologies 1/10/40Gbit Ethernet support"
 	depends on MAY_USE_DEVLINK
-	depends on PCI
+	depends on PCI && NETDEVICES && ETHERNET && INET
 	select MLX4_CORE
 	imply PTP_1588_CLOCK
 	---help---
-- 
1.8.3.1

Re: [PATCH net 2/2] net/mlx4_en: Fix build break when CONFIG_INET is off

[Cong Wang]

On Sun, Dec 2, 2018 at 4:37 AM Tariq Toukan <tariqt@mellanox.com> wrote:
>
> From: Saeed Mahameed <saeedm@mellanox.com>
>
> MLX4_EN depends on NETDEVICES, ETHERNET and INET Kconfigs.
> Make sure they are listed in MLX4_EN Kconfig dependencies.
>
> This fixes the following build break:
>
> drivers/net/ethernet/mellanox/mlx4/en_rx.c:582:18: warning: ‘struct iphdr’ declared inside parameter list [enabled by default]
> struct iphdr *iph)
> ^
> drivers/net/ethernet/mellanox/mlx4/en_rx.c:582:18: warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default]
> drivers/net/ethernet/mellanox/mlx4/en_rx.c: In function ‘get_fixed_ipv4_csum’:
> drivers/net/ethernet/mellanox/mlx4/en_rx.c:586:20: error: dereferencing pointer to incomplete type
> _u8 ipproto = iph->protocol;

I am confused with this build error. It complains about the
compiler doesn't know struct iphdr, but struct iphdr is defined
merely in include/uapi/linux/ip.h, without any dependency with
CONFIG_INET.

How could changing Kconfig fix this?

BTW, the patch itself is probably correct as mlx4 does rely on
INET to function, only the build failure confuses me.

Thanks.

Re: [PATCH net 0/2] mlx4 fixes for 4.20-rc

[David Miller]

From: Tariq Toukan <tariqt@mellanox.com>
Date: Sun,  2 Dec 2018 14:34:35 +0200

> This patchset includes small fixes for the mlx4_en driver.
> 
> First patch by Eran fixes the value used to init the netdevice's
> min_mtu field.
> Please queue it to -stable >= v4.10.
> 
> Second patch by Saeed adds missing Kconfig build dependencies.
> 
> Series generated against net commit:
> 35b827b6d061 tun: forbid iface creation with rtnl ops

Series applied and patch #1 queued up for -stable, thanks.