Re: [PATCH] net: dsa: qca8k: implement the port MTU callbacks

[Vladimir Oltean]

On Sat, Jul 18, 2020 at 10:35:55AM +0100, Jonathan McDowell wrote:
> This switch has a single max frame size configuration register, so we
> track the requested MTU for each port and apply the largest.
> 
> Signed-off-by: Jonathan McDowell <noodles@xxxxxxxx>
> ---
>  drivers/net/dsa/qca8k.c | 38 ++++++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/qca8k.h |  3 +++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 4acad5fa0c84..3690f02aea3a 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -670,6 +670,12 @@ qca8k_setup(struct dsa_switch *ds)
>  		}
>  	}
>  
> +	/* Setup our port MTUs to match power on defaults */
> +	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> +		priv->port_mtu[i] = ETH_FRAME_LEN + ETH_FCS_LEN;
> +	}

I am not quite sure the curly brackets are needed. And nowhere else in
qca8k.c is this convention being used.

> +	qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, ETH_FRAME_LEN + ETH_FCS_LEN);
> +
>  	/* Flush the FDB table */
>  	qca8k_fdb_flush(priv);
>  
> @@ -1098,6 +1104,36 @@ qca8k_port_disable(struct dsa_switch *ds, int port)
>  	priv->port_sts[port].enabled = 0;
>  }
>  
> +static int
> +qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
> +{
> +	struct qca8k_priv *priv = ds->priv;
> +	int i, mtu;
> +
> +	if ((new_mtu < ETH_MIN_MTU) || (new_mtu > QCA8K_MAX_MTU)) {
> +		return -EINVAL;
> +	}

I'm pretty sure this check should not be needed.
The only reason why slave_dev->min_mtu is 0 seems to be:

commit 8b1efc0f83f1f75b8f85c70d2211007de8fd7633
Author: Jarod Wilson <jarod@xxxxxxxxxx>
Date:   Thu Oct 20 23:25:27 2016 -0400

    net: remove MTU limits on a few ether_setup callers

    These few drivers call ether_setup(), but have no ndo_change_mtu, and thus
    were overlooked for changes to MTU range checking behavior. They
    previously had no range checks, so for feature-parity, set their min_mtu
    to 0 and max_mtu to ETH_MAX_MTU (65535), instead of the 68 and 1500
    inherited from the ether_setup() changes. Fine-tuning can come after we get
    back to full feature-parity here.

    CC: netdev@xxxxxxxxxxxxxxx
    Reported-by: Asbjoern Sloth Toennesen <asbjorn@xxxxxxxxxx>
    CC: Asbjoern Sloth Toennesen <asbjorn@xxxxxxxxxx>
    CC: R Parameswaran <parameswaran.r7@xxxxxxxxx>
    Signed-off-by: Jarod Wilson <jarod@xxxxxxxxxx>
    Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>

which is an oversight on my part. Since now DSA supports
ndo_change_mtu(), the "slave_dev->min_mtu = 0;" line in net/dsa/slave.c
can be removed and so can this check.

> +
> +	priv->port_mtu[port] = new_mtu;
> +
> +	mtu = 0;

I think it's more typical to initialize mtu to 0 at declaration time.

> +	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> +		if (priv->port_mtu[port] > mtu)
> +			mtu = priv->port_mtu[port];
> +	}

Again, curly brackets are not needed here, although some might feel it
aids readability.

> +
> +	/* Include L2 header / FCS length */
> +	qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN);
> +
> +	return 0;
> +}
> +
> +static int
> +qca8k_port_max_mtu(struct dsa_switch *ds, int port)
> +{
> +	return QCA8K_MAX_MTU;

So what is the maximum value that you can write into
QCA8K_MAX_FRAME_SIZE? 9000 or 9018? If it's 9000, you should report a
max MTU of 8982, and let the network stack do the range check for you,
that's why this callback exists in the first place.

Do you know how are VLAN tags accounted for (i.e. does iperf3 TCP work
over a VLAN sub-interface after your patch)? There are 2 options:
- The ports automatically increase the maximum accepted frame size by 4
  (or 8, in case of double tag) bytes if they see VLAN tagged traffic.
  Case in which you don't need to do anything.
- You need to manually account for the possibility that VLAN-tagged
  traffic will be received, since the 802.1Q header is not part of the
  SDU whose max length is measured by the MTU. So you might want to
  write a value to QCA8K_MAX_FRAME_SIZE that is either "mtu +
  VLAN_ETH_HLEN + ETH_FCS_LEN", or "mtu + ETH_HLEN + 2 * VLAN_HLEN +
  ETH_FCS_LEN", depending on whether you foresee double-tagging being
  used.

> +}
> +
>  static int
>  qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
>  		      u16 port_mask, u16 vid)
> @@ -1174,6 +1210,8 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
>  	.set_mac_eee		= qca8k_set_mac_eee,
>  	.port_enable		= qca8k_port_enable,
>  	.port_disable		= qca8k_port_disable,
> +	.port_change_mtu	= qca8k_port_change_mtu,
> +	.port_max_mtu		= qca8k_port_max_mtu,
>  	.port_stp_state_set	= qca8k_port_stp_state_set,
>  	.port_bridge_join	= qca8k_port_bridge_join,
>  	.port_bridge_leave	= qca8k_port_bridge_leave,
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 10ef2bca2cde..31439396401c 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -13,6 +13,7 @@
>  #include <linux/gpio.h>
>  
>  #define QCA8K_NUM_PORTS					7
> +#define QCA8K_MAX_MTU					9000
>  
>  #define PHY_ID_QCA8337					0x004dd036
>  #define QCA8K_ID_QCA8337				0x13
> @@ -58,6 +59,7 @@
>  #define   QCA8K_MDIO_MASTER_MAX_REG			32
>  #define QCA8K_GOL_MAC_ADDR0				0x60
>  #define QCA8K_GOL_MAC_ADDR1				0x64
> +#define QCA8K_MAX_FRAME_SIZE				0x78
>  #define QCA8K_REG_PORT_STATUS(_i)			(0x07c + (_i) * 4)
>  #define   QCA8K_PORT_STATUS_SPEED			GENMASK(1, 0)
>  #define   QCA8K_PORT_STATUS_SPEED_10			0
> @@ -189,6 +191,7 @@ struct qca8k_priv {
>  	struct device *dev;
>  	struct dsa_switch_ops ops;
>  	struct gpio_desc *reset_gpio;
> +	unsigned int port_mtu[QCA8K_NUM_PORTS];
>  };
>  
>  struct qca8k_mib_desc {
> -- 
> 2.27.0
> 

Thanks,
-Vladimir