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

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

On Apr 09, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email says "For your consideration."
> ---
> net: airoha: Support multiple net_devices for a single FE GDM port
> 
> This commit introduces support for mapping multiple net_device interfaces
> to a single Frame Engine (FE) GDM port via an external hardware arbiter.
> It updates the rx/tx paths, port allocation, and registration logic to
> handle the new net_device array.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> [ ... ]
> > @@ -615,9 +617,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;
> 
> While looking at this code, I noticed an issue that predates this patch.
> Can the lack of a memory barrier after checking the descriptor ownership bit
> cause stale reads?
> 
> The driver checks QDMA_DESC_DONE_MASK to verify that the hardware has finished
> writing the descriptor. However, it reads desc->msg1 before this check and
> without a dma_rmb():
> 
> 		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);
> 
> 		if (!(desc_ctrl & QDMA_DESC_DONE_MASK))
> 			break;
> 
> Since weakly ordered architectures can reorder memory reads, could the CPU
> prefetch msg1 before the DMA engine writes the new descriptor values, leading
> to routing the packet to the wrong gdm_dev?

This issue has been already fixed with the following commit:

commit 4ae0604a0673e11e2075b178387151fcad5111b5
Author: Lorenzo Bianconi <lorenzo@kernel.org>
Date:   Tue Apr 7 08:48:04 2026 +0200

    net: airoha: Add dma_rmb() and READ_ONCE() in airoha_qdma_rx_process()
    
    Add missing dma_rmb() in airoha_qdma_rx_process routine to make sure the
    DMA read operations are completed when the NIC reports the processing on
    the current descriptor is done. Moreover, add missing READ_ONCE() in
    airoha_qdma_rx_process() for DMA descriptor control fields in order to
    avoid any compiler reordering.

> 
> [ ... ]
> > @@ -1764,9 +1765,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;
> 
> Can configuring the loopback for one device break connectivity for other
> devices sharing the same port?
> 
> When multiple net_devices (like PCIE0 and PCIE1 on GDM3) open and call
> airoha_set_gdm2_loopback(), they modify global hardware registers such as
> REG_FE_WAN_PORT.
> 
> Since WAN0_MASK is unconditionally updated to track the current device's
> src_port:
> 
> 	airoha_fe_rmw(eth, REG_FE_WAN_PORT,
> 		      WAN1_EN_MASK | WAN1_MASK | WAN0_MASK,
> 		      FIELD_PREP(WAN0_MASK, src_port));
> 
> won't the last configured device clobber the hardware state for the others?

This issue is fixed with the following patch in the series:

commit 008995bd8f39c5fdf37a933fa3c962e7ef52e8c6
Author: Lorenzo Bianconi <lorenzo@kernel.org>
Date:   Fri Apr 3 12:07:27 2026 +0200

    net: airoha: Introduce WAN device flag
    
    Introduce WAN flag to specify if a given device is used to transmit/receive
    WAN or LAN traffic. Current codebase supports specifying LAN/WAN device
    configuration in ndo_init() callback during device bootstrap.
    Please note it is possible to specify multiple LAN devices but just a
    single WAN one.

> 
> [ ... ]
> > @@ -1987,7 +1986,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];
> 
> Does sharing the hardware QDMA TX queues among multiple net_device instances
> introduce a permanent transmit stall?
> 
> In airoha_dev_xmit(), if a shared hardware queue is full, the driver stops the
> netdev_queue of the specific netdev making the call:
> 
> 	if (q->queued + nr_frags >= q->ndesc) {
> 		/* not enough space in the queue */
> 		netif_tx_stop_queue(txq);
> 		spin_unlock_bh(&q->lock);
> 		return NETDEV_TX_BUSY;
> 	}
> 
> However, the TX completion handler (airoha_tx_irq_process()) only wakes the
> queue of the netdev attached to the completed packet. If an interface's TX
> queue is stopped but it has no pending packets in the hardware queue (because
> another interface filled it), will it ever get woken up?

This issue is orthogonal to this series and it will be fixed with a dedicated
patch.

> 
> [ ... ]
> > @@ -2955,11 +2958,25 @@ static int airoha_alloc_gdm_device(struct airoha_eth *eth,
> >  			 netdev->dev_addr);
> >  	}
> >  
> > +	/* Allowed nbq for EN7581 on GDM3 port are 4 and 5 for PCIE0
> > +	 * and PCIE1 respectively.
> > +	 */
> > +	index = nbq;
> > +	if (index && 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;
> > +	}
> > +
> > +	netdev->dev.of_node = of_node_get(np);
> >  	dev = netdev_priv(netdev);
> 
> Does this code leak the of_node reference?
> 
> The node reference is explicitly incremented via of_node_get(np). The networking
> core does not automatically drop explicit of_node references on free_netdev().
> Without a matching of_node_put() in the error paths or the driver's teardown
> function (airoha_remove()), will this reference permanently leak when the device
> is destroyed?

ack, I will fix it in v4.

> 
> [ ... ]
> > @@ -3003,7 +3021,43 @@ 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;
> > +		int nbq;
> > +
> > +		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");
> > +			of_node_put(node);
> > +			return -EINVAL;
> > +		}
> > +
> > +		/* Verify the provided nbq parameter is valid */
> > +		nbq = be32_to_cpup(nbq_ptr);
> > +		err = eth->soc->ops.get_src_port_id(port, nbq);
> > +		if (err < 0) {
> > +			of_node_put(node);
> > +			return err;
> > +		}
> > +
> > +		err = airoha_alloc_gdm_device(eth, port, nbq, node);
> > +		if (err) {
> > +			of_node_put(node);
> > +			return err;
> > +		}
> > +	}
> > +
> > +	return !d ? airoha_alloc_gdm_device(eth, port, 0, np) : 0;
> >  }
> 
> Does passing 0 for nbq in the fallback path break older device trees on the
> EN7581 SoC?
> 
> Later, when the interface is brought up, airoha_set_gdm2_loopback() calls
> airoha_en7581_get_src_port_id(), which returns -EINVAL for GDM3 if nbq is
> not 4 or 5:
> 
> 	switch (port->id) {
> 	case AIROHA_GDM3_IDX:
> 		/* 7581 SoC supports PCIe serdes on GDM3 port */
> 		if (nbq == 4)
> 			return HSGMII_LAN_7581_PCIE0_SRCPORT;
> 		if (nbq == 5)
> 			return HSGMII_LAN_7581_PCIE1_SRCPORT;
> 		break;
> 
> If nbq is hardcoded to 0 for legacy DTs, won't this cause interface
> initialization to fail unconditionally for GDM3?

ack, I will fix it in v4.

Regards,
Lorenzo

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