Re: [PATCH net-next v4 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 3 potential issue(s) to consider:
- [Medium] `airoha_dev_change_mtu()` improperly prevents valid software MTU configuration on shared ports.
- [Low] Redundant and conceptually misleading hardware teardown call in `airoha_dev_stop()`.
- [Low] Unnecessary use of `atomic_t` for `port->users`.
--

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

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

This commit adds reference counting for GDM ports to prevent stopping a
shared port if it is still used by another active net_device. It also
prevents MTU reductions if the port is shared with an active interface.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 0253919714e0c..1c4927c1aeb07 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c

[ ... ]

> @@ -1762,8 +1766,12 @@ static int airoha_dev_stop(struct net_device *netdev)
>  	for (i = 0; i < netdev->num_tx_queues; i++)
>  		netdev_tx_reset_subqueue(netdev, i);
>  
> -	airoha_set_gdm_port_fwd_cfg(qdma->eth, REG_GDM_FWD_CFG(port->id),
> -				    FE_PSE_PORT_DROP);
> +	if (atomic_dec_and_test(&port->users)) {
> +		airoha_set_vip_for_gdm_port(dev, false);

Is this call to airoha_set_vip_for_gdm_port() redundant?

Looking at airoha_dev_stop(), airoha_set_vip_for_gdm_port(dev, false) is
already called unconditionally earlier in the function. Since get_vip_port()
returns a device-specific bitmask based on dev->nbq, does the unconditional
call correctly clear the hardware bits for the specific device being stopped
without affecting other devices on the same shared port?

> +		airoha_set_gdm_port_fwd_cfg(qdma->eth,
> +					    REG_GDM_FWD_CFG(port->id),
> +					    FE_PSE_PORT_DROP);
> +	}
>  
>  	if (atomic_dec_and_test(&qdma->users)) {

[ ... ]

> @@ -1915,10 +1923,22 @@ 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;
>  
> +	cur_len = airoha_fe_get(eth, REG_GDM_LEN_CFG(port->id),
> +				GDM_LONG_LEN_MASK);
> +	if (len < cur_len) {
> +		u8 port_refcnt = atomic_read(&port->users);
> +
> +		/* We can decrease the device MTU just if the GDM port is
> +		 * not shared or if the other device is not running.
> +		 */
> +		if (port_refcnt > 1 || (port_refcnt && !netif_running(netdev)))
> +			return -EBUSY;

Does returning -EBUSY here improperly prevent valid software MTU
configurations?

Instead of failing the ndo_change_mtu callback and preventing the software
netdev->mtu from updating, should this code unconditionally allow the software
update (netdev->mtu = mtu) and simply skip the hardware register
(REG_GDM_LEN_CFG) downgrade to safely preserve the larger hardware limit
required by active shared siblings?

> +	}
> +
>  	airoha_fe_rmw(eth, REG_GDM_LEN_CFG(port->id),

[ ... ]

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index 207c75152fde0..3a313ac439e79 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h

[ ... ]

> @@ -553,6 +553,8 @@ struct airoha_gdm_port {
>  	struct airoha_gdm_dev *devs[AIROHA_MAX_NUM_GDM_DEVS];
>  	int id;
>  
> +	atomic_t users;
> +

Is the use of an atomic_t necessary for tracking users here?

The port->users reference count appears to only be modified in ndo_open and
ndo_stop, and read in ndo_change_mtu. Since all of these callback paths are
inherently strictly serialized by the kernel's rtnl_lock(), could a simple
integer be used instead to avoid unnecessary atomic overhead?

>  	struct airoha_hw_stats stats;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260507-airoha-eth-multi-serdes-v4-0-af613b61ae02@kernel.org?part=8