Re: [PATCH net-next v2 09/11] net: airoha: Support multiple net_devices for a single FE GDM port

[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.
---
Subject: net: airoha: Support multiple net_devices for a single FE GDM port

This commit introduces support for multiple net_devices connected to the
same Frame Engine (FE) GDM port via an external hardware arbiter. It
adds logic to parse device tree child nodes and instantiate separate
net_devices for TDM traffic from external SerDes.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 5e489e8593ba..0780fb8aacfb 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
[ ... ]
> @@ -614,9 +616,8 @@ static int airoha_qdma_rx_process(struct airoha_queue *q, int budget)
>  		u32 hash, reason, msg1 = le32_to_cpu(desc->msg1);
>  		struct page *page = virt_to_head_page(e->buf);
>  		u32 desc_ctrl = le32_to_cpu(desc->ctrl);
> -		struct airoha_gdm_port *port;
> -		struct net_device *netdev;
> -		int data_len, len, p;
> +		struct airoha_gdm_dev *dev;
> +		int data_len, len;
>  
>  		if (!(desc_ctrl & QDMA_DESC_DONE_MASK))
>  			break;
> @@ -633,12 +634,10 @@ static int airoha_qdma_rx_process(struct airoha_queue *q, int budget)
>  		if (!len || data_len < len)
>  			goto free_frag;
>  
> -		p = airoha_qdma_get_gdm_port(eth, desc);
> -		if (p < 0 || !eth->ports[p])
> +		dev = airoha_qdma_get_gdm_dev(eth, desc);
> +		if (IS_ERR(dev))
>  			goto free_frag;

If a scattered frame is being processed (q->skb is not NULL) and IS_ERR(dev)
triggers the goto free_frag, the error handler frees the skb:

free_frag:
	if (q->skb) {
		dev_kfree_skb(q->skb);
		q->skb = NULL;
	} else {
		page_pool_put_full_page(q->page_pool, page, true);
	}

Since the current fragment's page is not yet attached to q->skb, does this
skip the else branch and permanently leak the page pool buffer?

[ ... ]
> @@ -1747,9 +1748,7 @@ static int airoha_set_gdm2_loopback(struct airoha_gdm_dev *dev)
>  	airoha_fe_clear(eth, REG_FE_VIP_PORT_EN, BIT(AIROHA_GDM2_IDX));
>  	airoha_fe_clear(eth, REG_FE_IFC_PORT_EN, BIT(AIROHA_GDM2_IDX));
>  
> -	/* XXX: handle XSI_USB_PORT and XSI_PCE1_PORT */
> -	nbq = port->id == AIROHA_GDM3_IDX && airoha_is_7581(eth) ? 4 : 0;
> -	src_port = eth->soc->ops.get_src_port_id(port, nbq);
> +	src_port = eth->soc->ops.get_src_port_id(port, dev->nbq);
>  	if (src_port < 0)
>  		return src_port;

With multiple net_devices instantiated for the same physical GDM port, does
the initialization of sibling devices overwrite shared hardware registers?

For example, when airoha_set_gdm2_loopback() uses src_port to configure
REG_FE_WAN_PORT, it explicitly clears the WAN port routing masks:

	airoha_fe_rmw(eth, REG_FE_WAN_PORT,
		      WAN1_EN_MASK | WAN1_MASK | WAN0_MASK,
		      FIELD_PREP(WAN0_MASK, src_port));

Will the last initialized device clobber the WAN routing configurations and
hardware MAC filters for the others, causing their traffic to be dropped?

[ ... ]
> @@ -1970,7 +1969,8 @@ static netdev_tx_t airoha_dev_xmit(struct sk_buff *skb,
>  	}
>  
>  	fport = airoha_get_fe_port(dev);
> -	msg1 = FIELD_PREP(QDMA_ETH_TXMSG_FPORT_MASK, fport) |
> +	msg1 = FIELD_PREP(QDMA_ETH_TXMSG_NBOQ_MASK, dev->nbq) |
> +	       FIELD_PREP(QDMA_ETH_TXMSG_FPORT_MASK, fport) |
>  	       FIELD_PREP(QDMA_ETH_TXMSG_METER_MASK, 0x7f);
>  
>  	q = &qdma->q_tx[qid];

When multiple net_devices share a single QDMA instance and its TX rings
(qdma->q_tx), a TX queue stall might occur here.

If the shared hardware ring fills up, netif_tx_stop_queue() stops the
transmitting device's queue. Later, when descriptors are freed, the driver
only wakes the queue corresponding to the specific packet that just completed.

If multiple devices' TX queues are stopped, will the other devices' queues
remain stalled unless their specific packets happen to be processed?

[ ... ]
> @@ -2897,10 +2900,11 @@ bool airoha_is_valid_gdm_dev(struct airoha_eth *eth,
>  
>  static int airoha_alloc_gdm_device(struct airoha_eth *eth,
>  				   struct airoha_gdm_port *port,
> -				   struct device_node *np)
> +				   int nbq, struct device_node *np)
>  {
> -	struct airoha_gdm_dev *dev;
>  	struct net_device *netdev;
> +	struct airoha_gdm_dev *dev;
> +	u8 index;
>  	int err;
[ ... ]
> +	/* Allowed nbq for EN7581 on GDM3 port are 4 and 5 for PCIE0
> +	 * and PCIE1 respectively.
> +	 */
> +	index = nbq;
> +	if (airoha_is_7581(eth) && port->id == AIROHA_GDM3_IDX)
> +		index -= 4;
> +
> +	if (index >= ARRAY_SIZE(port->devs) || port->devs[index]) {
> +		dev_err(eth->dev, "invalid nbq id: %d\n", nbq);
> +		return -EINVAL;
> +	}

There are two potential issues regarding the new index logic:

First, does this break probing for older device trees without child nodes?
When falling back to airoha_alloc_gdm_device(..., 0, np) with nbq = 0,
the index -= 4 operation for AIROHA_GDM3_IDX will cause the u8 index to
underflow to 252. The bounds check 252 >= 2 fails, returning -EINVAL.

Second, can an invalid device tree value bypass the bounds check?
The int nbq parameter is directly assigned to the u8 index. If a large value
like reg = <256> is provided, it truncates to 0, passing the index >= 2
check. The un-truncated int nbq is then stored in dev->nbq, potentially
causing out-of-bounds shifts when used in hardware macros.

[ ... ]
> @@ -2986,7 +3004,36 @@ static int airoha_alloc_gdm_port(struct airoha_eth *eth,
>  	if (err)
>  		return err;
>  
> -	return airoha_alloc_gdm_device(eth, port, np);
> +	for_each_child_of_node(np, node) {
> +		/* Multiple external serdes connected to the FE GDM port via an
> +		 * external arbiter.
> +		 */
> +		const __be32 *nbq_ptr;
> +
> +		if (!of_device_is_compatible(node, "airoha,eth-port"))
> +			continue;
> +
> +		d++;
> +		if (!of_device_is_available(node))
> +			continue;
> +
> +		nbq_ptr = of_get_property(node, "reg", NULL);
> +		if (!nbq_ptr) {
> +			dev_err(eth->dev, "missing nbq id\n");
> +			err = -EINVAL;
> +			of_node_put(node);
> +			break;
> +		}
> +
> +		err = airoha_alloc_gdm_device(eth, port, be32_to_cpup(nbq_ptr),
> +					      node);
> +		if (err) {
> +			of_node_put(node);
> +			return err;
> +		}
> +	}
> +
> +	return !d ? airoha_alloc_gdm_device(eth, port, 0, np) : 0;
>  }

If nbq_ptr is missing, err = -EINVAL is set and the loop breaks. However,
since the child counter d was already incremented, the final statement
evaluates to 0 (success) because !d is false.

Does this swallow the -EINVAL error and cause a silent failure where the
driver continues probing without initializing the misconfigured GDM devices?

Additionally, the for_each_child_of_node() iterator automatically handles
taking and dropping the reference to node.

When node is passed to airoha_alloc_gdm_device(), it is directly assigned:
netdev->dev.of_node = np;

Since of_node_get() is not called, when the loop iteration finishes and drops
the reference, does this leave the net_device pointing to a freed device
tree node?
-- 
pw-bot: cr