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

Lorenzo Bianconi
2026-04-10T10:19:13Z

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