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

Lorenzo Bianconi
2026-04-03T10:18:18Z

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