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

[Jakub Kicinski <kuba@kernel.org>]

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?

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

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

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

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