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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 8019 bytes --]

> 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?

This issue is fixed in the following net patch (not merged yet):

https://lore.kernel.org/netdev/20260402-airoha_qdma_rx_process-mem-leak-fix-v1-1-b5706f402d3c@kernel.org/

> 
> [ ... ]
> > @@ -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?

ack, I will fix it in v3.

> 
> [ ... ]
> > @@ -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?

This does not seem strictly related to this series. I will look into it in a
dedicated patch.

> 
> [ ... ]
> > @@ -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.

ack, I will fix it in v3.

> 
> 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.

ack, I will fix it in v3.

> 
> [ ... ]
> > @@ -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?

ack, I will fix it in v3.

Regards,
Lorenzo

> -- 
> pw-bot: cr

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]