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