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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: airoha: Do not stop GDM port if it is shared

This patch adds a per-port users refcount so the GDM forward configuration
is only reset to FE_PSE_PORT_DROP when the last netdev attached to the
shared GDM port stops, and it adjusts the MTU/length register updates to
cope with the shared case.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 0253919714e0..1c4927c1aeb0 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -1714,8 +1714,8 @@ static int airoha_dev_open(struct net_device *netdev)
>  	int err, len = ETH_HLEN + netdev->mtu + ETH_FCS_LEN;
>  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
>  	struct airoha_gdm_port *port = dev->port;
> +	u32 cur_len, pse_port = FE_PSE_PORT_PPE1;
>  	struct airoha_qdma *qdma = dev->qdma;
> -	u32 pse_port = FE_PSE_PORT_PPE1;
>  
>  	netif_tx_start_all_queues(netdev);
>  	err = airoha_set_vip_for_gdm_port(dev, true);
> @@ -1729,10 +1729,14 @@ static int airoha_dev_open(struct net_device *netdev)
>  		airoha_fe_clear(qdma->eth, REG_GDM_INGRESS_CFG(port->id),
>  				GDM_STAG_EN_MASK);
>  
> -	airoha_fe_rmw(qdma->eth, REG_GDM_LEN_CFG(port->id),
> -		      GDM_SHORT_LEN_MASK | GDM_LONG_LEN_MASK,
> -		      FIELD_PREP(GDM_SHORT_LEN_MASK, 60) |
> -		      FIELD_PREP(GDM_LONG_LEN_MASK, len));
> +	cur_len = airoha_fe_get(qdma->eth, REG_GDM_LEN_CFG(port->id),
> +				GDM_LONG_LEN_MASK);
> +	if (!atomic_read(&port->users) || len > cur_len)
> +		airoha_fe_rmw(qdma->eth, REG_GDM_LEN_CFG(port->id),
> +			      GDM_SHORT_LEN_MASK | GDM_LONG_LEN_MASK,
> +			      FIELD_PREP(GDM_SHORT_LEN_MASK, 60) |
> +			      FIELD_PREP(GDM_LONG_LEN_MASK, len));
> +	atomic_inc(&port->users);

[Medium]
When a second netdev opens with a larger MTU, this unconditionally
widens GDM_LONG_LEN_MASK for the already-running sibling that has a
smaller MTU:

	if (!atomic_read(&port->users) || len > cur_len)
		airoha_fe_rmw(qdma->eth, REG_GDM_LEN_CFG(port->id),
			      GDM_SHORT_LEN_MASK | GDM_LONG_LEN_MASK,
			      FIELD_PREP(GDM_SHORT_LEN_MASK, 60) |
			      FIELD_PREP(GDM_LONG_LEN_MASK, len));

Meanwhile airoha_dev_change_mtu() rejects shrinks (len < cur_len) with
-EBUSY when the port is shared. Is this asymmetry intentional?

With the new policy the hardware long-length ends up as the maximum of
the two MTUs, so the smaller-MTU sibling silently accepts frames larger
than its advertised MTU at the MAC level. Should opens that would
increase the shared length also be gated (symmetric policy), or should
the commit message and a comment document the chosen ceiling/floor
behaviour?

>  
>  	airoha_qdma_set(qdma, REG_QDMA_GLOBAL_CFG,
>  			GLOBAL_CFG_TX_DMA_EN_MASK |
> @@ -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);
> +		airoha_set_gdm_port_fwd_cfg(qdma->eth,
> +					    REG_GDM_FWD_CFG(port->id),
> +					    FE_PSE_PORT_DROP);
> +	}

[Low]
Is the airoha_set_vip_for_gdm_port(dev, false) call inside the
atomic_dec_and_test() block intentional?

Looking at airoha_dev_stop() as a whole, the same call is already made
unconditionally a few lines above:

	netif_tx_disable(netdev);
	airoha_set_vip_for_gdm_port(dev, false);
	for (i = 0; i < netdev->num_tx_queues; i++)
		netdev_tx_reset_subqueue(netdev, i);

	if (atomic_dec_and_test(&port->users)) {
		airoha_set_vip_for_gdm_port(dev, false);
		...
	}

Since get_vip_port() derives the bit mask per-(port, dev->nbq) and
airoha_fe_clear() of already-cleared bits is a no-op, the second call
looks like dead code today. The commit log only mentions the
FE_PSE_PORT_DROP gating, not VIP handling. Should the duplicate be
dropped in v2?

>  
>  	if (atomic_dec_and_test(&qdma->users)) {
>  		airoha_qdma_clear(qdma, REG_QDMA_GLOBAL_CFG,
> @@ -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;
> +	}
> +

[Low]
Why is port_refcnt typed as u8 here when atomic_read() returns int?

	u8 port_refcnt = atomic_read(&port->users);

With AIROHA_MAX_NUM_GDM_DEVS currently 2 this cannot overflow, but a
future bump of AIROHA_MAX_NUM_GDM_DEVS, or a refcount leak, that pushed
port->users to 256 would wrap to 0 and quietly bypass both the
port_refcnt > 1 and port_refcnt && !netif_running(netdev) checks,
permitting MTU shrinks that are meant to be rejected. Would using int
here (the type atomic_read() returns) be preferable?

>  	airoha_fe_rmw(eth, REG_GDM_LEN_CFG(port->id),
>  		      GDM_LONG_LEN_MASK,
>  		      FIELD_PREP(GDM_LONG_LEN_MASK, len));