Re: [PATCH net-next v7 08/10] net: airoha: Do not stop GDM port if it is shared

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Flawed hardware MTU recalculation logic in airoha_dev_change_mtu() for shared GDM ports
--

commit 28e02ac4611575e19c4f6eef3fa74c11d4debb6e
Author: Lorenzo Bianconi <lorenzo@kernel.org>

net: airoha: Do not stop GDM port if it is shared

This commit updates the driver to allow multiple devices to share the same
GDM port. It stops the port only when no other devices are using it and
adjusts the hardware MTU to accommodate all active devices.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 8558790eca644..c70e3ace38e25 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c

[ ... ]

> @@ -1917,13 +1945,18 @@ static void airoha_dev_get_stats64(struct net_device *netdev,
>  static int airoha_dev_change_mtu(struct net_device *netdev, int mtu)
>  {
>  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> +	u32 cur_len, len = ETH_HLEN + mtu + ETH_FCS_LEN;
>  	struct airoha_gdm_port *port = dev->port;
> -	u32 len = ETH_HLEN + mtu + ETH_FCS_LEN;
>  	struct airoha_eth *eth = dev->eth;
>  
> -	airoha_fe_rmw(eth, REG_GDM_LEN_CFG(port->id),
> -		      GDM_LONG_LEN_MASK,
> -		      FIELD_PREP(GDM_LONG_LEN_MASK, len));
> +	cur_len = airoha_fe_get(eth, REG_GDM_LEN_CFG(port->id),
> +				GDM_LONG_LEN_MASK);
> +	if (len > cur_len || !port->users ||
> +	    (port->users == 1 && netif_running(netdev)))
> +		airoha_fe_rmw(eth, REG_GDM_LEN_CFG(port->id),
> +			      GDM_LONG_LEN_MASK,
> +			      FIELD_PREP(GDM_LONG_LEN_MASK, len));
> +

The commit message states that the driver must "always set in GDM_LONG_LEN_MASK
field of REG_GDM_LEN_CFG register the maximum MTU of all running net_devices
connected to the same GDM port."

Does this logic successfully recalculate the MTU when the MTU of a running
device is decreased while sibling devices are also running? In that scenario,
len > cur_len evaluates to false, which appears to skip the update and leave
the hardware MTU stuck at the previous larger value.

Additionally, if an administratively down device has its MTU increased,
len > cur_len can evaluate to true. Does this prematurely increase the
hardware MTU, exposing the running sibling devices to larger frames based
on a stopped device's configuration?

Would it be more accurate to iterate over the devices to compute the
actual maximum MTU of all active devices here, similar to the approach taken
in airoha_dev_stop()?

>  	WRITE_ONCE(netdev->mtu, mtu);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-airoha-eth-multi-serdes-v7-0-99e0093303e2@kernel.org?part=8