[PATCH net-next] net: stmmac: publish actual MTU restriction

[PATCH net-next] net: stmmac: publish actual MTU restriction

[Corinna Vinschen]

Apart from devices setting the max MTU value from device tree,
the initialization functions in many drivers use a default value
of JUMBO_LEN.

However, that doesn't reflect reality.  The stmmac_change_mtu
function restricts the MTU to the size of a single queue in the TX
FIFO.

For instance, on st_gmac without device tree this is by default 4096.
And so this is what you get:

  # ip -d link show dev enp0s29f1
2: enp0s29f1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
    link/ether 98:4f:ee:16:11:99 brd ff:ff:ff:ff:ff:ff promiscuity 0 minmtu 46 maxmtu 9000 [...etc...]

So maxmtu is set to 9000, but then:

  # ip link set enp0s29f1 mtu 4096
  # ip link set enp0s29f1 mtu 4097
  RTNETLINK answers: Invalid argument

Change code accordingly by not setting maxmtu to JUMBO_LEN by default.
Instead, after all is set and done, let stmmac_dvr_probe set maxmtu
according to the queue size, if it hasn't been already set via device
tree.

The only exception from this rule is dwmac-mediatek, setting maxmtu to
ETH_DATA_LEN, which should be kept intact.

Result now:

  # ip link set enp0s29f1 mtu 4096
  # ip link set enp0s29f1 mtu 4097
  Error: mtu greater than device maximum.

Tested on Intel Elkhart Lake system.

Fixes: 2618abb73c895 ("stmmac: Fix kernel crashes for jumbo frames")
Fixes: a2cd64f30140c ("net: stmmac: fix maxmtu assignment to be within valid range")
Fixes: ebecb860ed228 ("net: stmmac: pci: Add HAPS support using GMAC5")
Fixes: 58da0cfa6cf12 ("net: stmmac: create dwmac-intel.c to contain all Intel platform")
Fixes: 30bba69d7db40 ("stmmac: pci: Add dwmac support for Loongson")
Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c     | 6 ------
 drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c  | 3 ---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c     | 9 +++++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c      | 6 ------
 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 5 -----
 5 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 13aa919633b4..2912685b481c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -430,9 +430,6 @@ static void common_default_data(struct plat_stmmacenet_data *plat)
 	/* Set default value for unicast filter entries */
 	plat->unicast_filter_entries = 1;
 
-	/* Set the maxmtu to a default of JUMBO_LEN */
-	plat->maxmtu = JUMBO_LEN;
-
 	/* Set default number of RX and TX queues to use */
 	plat->tx_queues_to_use = 1;
 	plat->rx_queues_to_use = 1;
@@ -559,9 +556,6 @@ static int intel_mgbe_common_data(struct pci_dev *pdev,
 	/* Set default value for unicast filter entries */
 	plat->unicast_filter_entries = 1;
 
-	/* Set the maxmtu to a default of JUMBO_LEN */
-	plat->maxmtu = JUMBO_LEN;
-
 	plat->vlan_fail_q_en = true;
 
 	/* Use the last Rx queue */
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
index a25c187d3185..ec90c925ad33 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
@@ -21,9 +21,6 @@ static int loongson_default_data(struct plat_stmmacenet_data *plat)
 	/* Set default value for unicast filter entries */
 	plat->unicast_filter_entries = 1;
 
-	/* Set the maxmtu to a default of JUMBO_LEN */
-	plat->maxmtu = JUMBO_LEN;
-
 	/* Set default number of RX and TX queues to use */
 	plat->tx_queues_to_use = 1;
 	plat->rx_queues_to_use = 1;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 17310ade88dd..c5e74097d9ab 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7246,6 +7246,15 @@ int stmmac_dvr_probe(struct device *device,
 		ndev->max_mtu = JUMBO_LEN;
 	else
 		ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN);
+
+	/* stmmac_change_mtu restricts MTU to queue size.
+	 * Set maxmtu accordingly, if it hasn't been set from DT.
+	 */
+	if (priv->plat->maxmtu == 0) {
+		priv->plat->maxmtu = priv->plat->tx_fifo_size ?:
+				     priv->dma_cap.tx_fifo_size;
+		priv->plat->maxmtu /= priv->plat->tx_queues_to_use;
+	}
 	/* Will not overwrite ndev->max_mtu if plat->maxmtu > ndev->max_mtu
 	 * as well as plat->maxmtu < ndev->min_mtu which is a invalid range.
 	 */
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 644bb54f5f02..b3bb0c174b8b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -33,9 +33,6 @@ static void common_default_data(struct plat_stmmacenet_data *plat)
 	/* Set default value for unicast filter entries */
 	plat->unicast_filter_entries = 1;
 
-	/* Set the maxmtu to a default of JUMBO_LEN */
-	plat->maxmtu = JUMBO_LEN;
-
 	/* Set default number of RX and TX queues to use */
 	plat->tx_queues_to_use = 1;
 	plat->rx_queues_to_use = 1;
@@ -86,9 +83,6 @@ static int snps_gmac5_default_data(struct pci_dev *pdev,
 	/* Set default value for unicast filter entries */
 	plat->unicast_filter_entries = 1;
 
-	/* Set the maxmtu to a default of JUMBO_LEN */
-	plat->maxmtu = JUMBO_LEN;
-
 	/* Set default number of RX and TX queues to use */
 	plat->tx_queues_to_use = 4;
 	plat->rx_queues_to_use = 4;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 067a40fe0a23..857411105a0a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -468,11 +468,6 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 	plat->en_tx_lpi_clockgating =
 		of_property_read_bool(np, "snps,en-tx-lpi-clockgating");
 
-	/* Set the maxmtu to a default of JUMBO_LEN in case the
-	 * parameter is not present in the device tree.
-	 */
-	plat->maxmtu = JUMBO_LEN;
-
 	/* Set default value for multicast hash bins */
 	plat->multicast_filter_bins = HASH_TABLE_SIZE;
 
-- 
2.39.2

Re: [PATCH net-next] net: stmmac: publish actual MTU restriction

[Jakub Kicinski]

On Fri, 31 Mar 2023 11:23:44 +0200 Corinna Vinschen wrote:
> Fixes: 2618abb73c895 ("stmmac: Fix kernel crashes for jumbo frames")
> Fixes: a2cd64f30140c ("net: stmmac: fix maxmtu assignment to be within valid range")
> Fixes: ebecb860ed228 ("net: stmmac: pci: Add HAPS support using GMAC5")
> Fixes: 58da0cfa6cf12 ("net: stmmac: create dwmac-intel.c to contain all Intel platform")
> Fixes: 30bba69d7db40 ("stmmac: pci: Add dwmac support for Loongson")

I'm not sure if we need fixes tags for this.
Are any users depending on the advertised values being exactly right?

> +	/* stmmac_change_mtu restricts MTU to queue size.
> +	 * Set maxmtu accordingly, if it hasn't been set from DT.
> +	 */
> +	if (priv->plat->maxmtu == 0) {
> +		priv->plat->maxmtu = priv->plat->tx_fifo_size ?:
> +				     priv->dma_cap.tx_fifo_size;
> +		priv->plat->maxmtu /= priv->plat->tx_queues_to_use;

tx_queues_to_use may change due to reconfiguration, no?
What will happen then?

Re: [PATCH net-next] net: stmmac: publish actual MTU restriction

[Corinna Vinschen]

On Mar 31 21:52, Jakub Kicinski wrote:
> On Fri, 31 Mar 2023 11:23:44 +0200 Corinna Vinschen wrote:
> > Fixes: 2618abb73c895 ("stmmac: Fix kernel crashes for jumbo frames")
> > Fixes: a2cd64f30140c ("net: stmmac: fix maxmtu assignment to be within valid range")
> > Fixes: ebecb860ed228 ("net: stmmac: pci: Add HAPS support using GMAC5")
> > Fixes: 58da0cfa6cf12 ("net: stmmac: create dwmac-intel.c to contain all Intel platform")
> > Fixes: 30bba69d7db40 ("stmmac: pci: Add dwmac support for Loongson")
> 
> I'm not sure if we need fixes tags for this.

I can remove those in v2.

> Are any users depending on the advertised values being exactly right?

The max MTU is advertised per interface:

p -d link show dev enp0s29f1
2: enp0s29f1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
    link/ether [...] promiscuity 0 minmtu 46 maxmtu 9000 [...]

So the idea is surely that the user can check it and then set the MTU
accordingly.  If the interface claims a max MTU of 9000, the expectation
is that setting the MTU to this value just works, right?

So isn't it better if the interface only claims what it actually supports,
i. .e, 

  # ip -d link show dev enp0s29f1
  2: enp0s29f1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
      link/ether [...] promiscuity 0 minmtu 46 maxmtu 4096 [...]

?

> > +	/* stmmac_change_mtu restricts MTU to queue size.
> > +	 * Set maxmtu accordingly, if it hasn't been set from DT.
> > +	 */
> > +	if (priv->plat->maxmtu == 0) {
> > +		priv->plat->maxmtu = priv->plat->tx_fifo_size ?:
> > +				     priv->dma_cap.tx_fifo_size;
> > +		priv->plat->maxmtu /= priv->plat->tx_queues_to_use;
> 
> tx_queues_to_use may change due to reconfiguration, no?
> What will happen then?

Nothing.  tx_fifo_size is tx_queues_to_use multiplied by the size of the
queue.  All the above code does is to compute the size of the queues,
which is a fixed value limiting the size of the MTU.  It's the same
check the stmmac_change_mtu() function performs to allow or deny the MTU
change, basically:

  txfifosz = priv->plat->tx_fifo_size;
  if (txfifosz == 0)
    txfifosz = priv->dma_cap.tx_fifo_size;
  txfifosz /= priv->plat->tx_queues_to_use;
  if (txfifosz < new_mtu)
    return -EINVAL;


Corinna

Re: [PATCH net-next] net: stmmac: publish actual MTU restriction

[Jakub Kicinski]

On Mon, 3 Apr 2023 11:12:12 +0200 Corinna Vinschen wrote:
> > Are any users depending on the advertised values being exactly right?  
> 
> The max MTU is advertised per interface:
> 
> p -d link show dev enp0s29f1
> 2: enp0s29f1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
>     link/ether [...] promiscuity 0 minmtu 46 maxmtu 9000 [...]
> 
> So the idea is surely that the user can check it and then set the MTU
> accordingly.  If the interface claims a max MTU of 9000, the expectation
> is that setting the MTU to this value just works, right?
> 
> So isn't it better if the interface only claims what it actually supports,
> i. .e, 
> 
>   # ip -d link show dev enp0s29f1
>   2: enp0s29f1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
>       link/ether [...] promiscuity 0 minmtu 46 maxmtu 4096 [...]
> 
> ?

No doubt that it's better to be more precise.

The question is what about drivers which can't support full MTU with
certain features enabled. So far nobody has been updating the max MTU
dynamically, to my knowledge, so the max MTU value is the static max
under best conditions.

> > > +	/* stmmac_change_mtu restricts MTU to queue size.
> > > +	 * Set maxmtu accordingly, if it hasn't been set from DT.
> > > +	 */
> > > +	if (priv->plat->maxmtu == 0) {
> > > +		priv->plat->maxmtu = priv->plat->tx_fifo_size ?:
> > > +				     priv->dma_cap.tx_fifo_size;
> > > +		priv->plat->maxmtu /= priv->plat->tx_queues_to_use;  
> > 
> > tx_queues_to_use may change due to reconfiguration, no?
> > What will happen then?  
> 
> Nothing.  tx_fifo_size is tx_queues_to_use multiplied by the size of the
> queue.  All the above code does is to compute the size of the queues,
> which is a fixed value limiting the size of the MTU.  It's the same
> check the stmmac_change_mtu() function performs to allow or deny the MTU
> change, basically:
> 
>   txfifosz = priv->plat->tx_fifo_size;
>   if (txfifosz == 0)
>     txfifosz = priv->dma_cap.tx_fifo_size;
>   txfifosz /= priv->plat->tx_queues_to_use;
>   if (txfifosz < new_mtu)
>     return -EINVAL;

I haven't looked at the code in detail but if we start with
tx_queues_to_use = 4 and lower it via ethtool -L, won't that
make the core prevent setting higher MTU even tho the driver
would have supported it previously?

Re: [PATCH net-next] net: stmmac: publish actual MTU restriction

[Corinna Vinschen]

On Apr  3 09:30, Jakub Kicinski wrote:
> On Mon, 3 Apr 2023 11:12:12 +0200 Corinna Vinschen wrote:
> > > Are any users depending on the advertised values being exactly right?  
> > 
> > The max MTU is advertised per interface:
> > 
> > p -d link show dev enp0s29f1
> > 2: enp0s29f1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
> >     link/ether [...] promiscuity 0 minmtu 46 maxmtu 9000 [...]
> > 
> > So the idea is surely that the user can check it and then set the MTU
> > accordingly.  If the interface claims a max MTU of 9000, the expectation
> > is that setting the MTU to this value just works, right?
> > 
> > So isn't it better if the interface only claims what it actually supports,
> > i. .e, 
> > 
> >   # ip -d link show dev enp0s29f1
> >   2: enp0s29f1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
> >       link/ether [...] promiscuity 0 minmtu 46 maxmtu 4096 [...]
> > 
> > ?
> 
> No doubt that it's better to be more precise.
> 
> The question is what about drivers which can't support full MTU with
> certain features enabled. So far nobody has been updating the max MTU
> dynamically, to my knowledge, so the max MTU value is the static max
> under best conditions.

Yeah, I agree, but what this code does *is* to set the max MTU to the
best possible value.

In all(*) drivers using the stmmac core, the max MTU is restricted by
the size of a single TX queue per the code in stmmac_change_mtu().

You can change the number of queues within the limits of the HW
dynamically, but the size of the queues is not configurable.  The size
of the queues is determined at probe time, typically as tx_fifo_size,
a HW feature, divided by the max number of queues.  Or, in dwmac-intel.c,
simply set to the constant value 4096.

(*) Noticable exception is dwmac-mediatek, which doesn't support jumbo
    frames and sets max mtu to ETH_DATA_LEN.

> > > > +	/* stmmac_change_mtu restricts MTU to queue size.
> > > > +	 * Set maxmtu accordingly, if it hasn't been set from DT.
> > > > +	 */
> > > > +	if (priv->plat->maxmtu == 0) {
> > > > +		priv->plat->maxmtu = priv->plat->tx_fifo_size ?:
> > > > +				     priv->dma_cap.tx_fifo_size;
> > > > +		priv->plat->maxmtu /= priv->plat->tx_queues_to_use;  
> > > 
> > > tx_queues_to_use may change due to reconfiguration, no?
> > > What will happen then?  
> > 
> > Nothing.  tx_fifo_size is tx_queues_to_use multiplied by the size of the
> > queue.  All the above code does is to compute the size of the queues,
> > which is a fixed value limiting the size of the MTU.  It's the same
> > check the stmmac_change_mtu() function performs to allow or deny the MTU
> > change, basically:
> > 
> >   txfifosz = priv->plat->tx_fifo_size;
> >   if (txfifosz == 0)
> >     txfifosz = priv->dma_cap.tx_fifo_size;
> >   txfifosz /= priv->plat->tx_queues_to_use;
> >   if (txfifosz < new_mtu)
> >     return -EINVAL;
> 
> I haven't looked at the code in detail but if we start with
> tx_queues_to_use = 4 and lower it via ethtool -L, won't that
> make the core prevent setting higher MTU even tho the driver
> would have supported it previously?

No.  See the code snippet from stmmac_change_mtu() above.  Max MTU is
*not* restricted by the number of queues, but by the non-configurable
size of a single queue.

After probe, you can potentially change the number of queues, but not
the size of the queues.


Corinna

Re: [PATCH net-next] net: stmmac: publish actual MTU restriction

[Corinna Vinschen]

On Apr  3 20:17, Corinna Vinschen wrote:
> On Apr  3 09:30, Jakub Kicinski wrote:
> > On Mon, 3 Apr 2023 11:12:12 +0200 Corinna Vinschen wrote:
> > > > Are any users depending on the advertised values being exactly right?  
> > > 
> > > The max MTU is advertised per interface:
> > > 
> > > p -d link show dev enp0s29f1
> > > 2: enp0s29f1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
> > >     link/ether [...] promiscuity 0 minmtu 46 maxmtu 9000 [...]
> > > 
> > > So the idea is surely that the user can check it and then set the MTU
> > > accordingly.  If the interface claims a max MTU of 9000, the expectation
> > > is that setting the MTU to this value just works, right?
> > > 
> > > So isn't it better if the interface only claims what it actually supports,
> > > i. .e, 
> > > 
> > >   # ip -d link show dev enp0s29f1
> > >   2: enp0s29f1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
> > >       link/ether [...] promiscuity 0 minmtu 46 maxmtu 4096 [...]
> > > 
> > > ?
> > 
> > No doubt that it's better to be more precise.
> > 
> > The question is what about drivers which can't support full MTU with
> > certain features enabled. So far nobody has been updating the max MTU
> > dynamically, to my knowledge, so the max MTU value is the static max
> > under best conditions.
> 
> Yeah, I agree, but what this code does *is* to set the max MTU to the
> best possible value.
> 
> In all(*) drivers using the stmmac core, the max MTU is restricted by
> the size of a single TX queue per the code in stmmac_change_mtu().
> 
> You can change the number of queues within the limits of the HW
> dynamically, but the size of the queues is not configurable.

Wait.  The one thing which never changes after probe is the value
of tx_fifo_size.  It's the size of the buffer for all queues.  And if I
lower tx_queues_to_use, then the value of the queue size computed as

  tx_fifo_size / tx_queues_to_use

actually depends on the number of queues.

So I totally misinterpreted the code at this point.

Ouch.

I withdraw the patch.  Sorry for wasting your time.


Corinna