Re: [PATCH net-next v3 09/10] net: ethernet: mtk-star-emac: separate tx/rx handling with two NAPIs

Dear Jakub,
	Thanks for your comments~

On Thu, 2022-06-23 at 21:34 -0700, Jakub Kicinski wrote:

On Wed, 22 Jun 2022 17:05:44 +0800 Biao Huang wrote:

quoted

+	if (rx || tx) {
+		spin_lock_irqsave(&priv->lock, flags);
+		/* mask Rx and TX Complete interrupt */
+		mtk_star_disable_dma_irq(priv, rx, tx);
+		spin_unlock_irqrestore(&priv->lock, flags);

You do _irqsave / _irqrestore here

We should invoke spin_lock, no need save/store irq here.

quoted

+		if (rx)
+			__napi_schedule_irqoff(&priv->rx_napi);
+		if (tx)
+			__napi_schedule_irqoff(&priv->tx_napi);

Yet assume _irqoff here.

So can this be run from non-IRQ context or not?

seems __napi_schedule is more proper for our case, we'll modify it in
next send.

quoted

-	if (mtk_star_ring_full(ring))
+	if (unlikely(mtk_star_tx_ring_avail(ring) < MAX_SKB_FRAGS + 1))
 		netif_stop_queue(ndev);

Please look around other drivers (like ixgbe) and copy the way they
handle safe stopping of the queues. You need to add some barriers and
re-check after disabling.

Yes, we look drivers from other vendors, and will do similar thing in
next send.

quoted

-	spin_unlock_bh(&priv->lock);
-
 	mtk_star_dma_resume_tx(priv);
 
 	return NETDEV_TX_OK;

quoted

+	while ((entry != head) && (count < MTK_STAR_RING_NUM_DESCS -
1)) {
 

Parenthesis unnecessary, so is the empty line after the while ().

Yes, the empty line will be removed in next send.

quoted

 		ret = mtk_star_tx_complete_one(priv);
 		if (ret < 0)
 			break;
+
+		count++;
+		pkts_compl++;
+		bytes_compl += ret;
+		entry = ring->tail;
 	}
 
+	__netif_tx_lock_bh(netdev_get_tx_queue(priv->ndev, 0));
 	netdev_completed_queue(ndev, pkts_compl, bytes_compl);
+	__netif_tx_unlock_bh(netdev_get_tx_queue(priv->ndev, 0));

what are you taking this lock for?

In this version, we encounter some issue related to
__QUEUE_STATE_STACK_OFF, 
and if we add __netif_tx_lock_bh here, it disappears.

When recieve your comments, we survey netdev_completed_queue handles in
drivers from other vendors, we beleive the __QUEUE_STATE_STACK_OFF
issue may caused by unproper usage of __napi_schedule_irqoff in
previous lines, and we'll remove __netif_tx_lock_bh, and have another
try.

If our local stress test pass, corresponding modification will be added
in next send.

quoted

-	if (wake && netif_queue_stopped(ndev))
+	if (unlikely(netif_queue_stopped(ndev)) &&
+	    (mtk_star_tx_ring_avail(ring) > MTK_STAR_TX_THRESH))
 		netif_wake_queue(ndev);
 
-	spin_unlock(&priv->lock);
+	if (napi_complete(napi)) {
+		spin_lock_irqsave(&priv->lock, flags);
+		mtk_star_enable_dma_irq(priv, false, true);
+		spin_unlock_irqrestore(&priv->lock, flags);
+	}
+
+	return 0;
 }

@@ -1475,6 +1514,7 @@ static int mtk_star_set_timing(struct

mtk_star_priv *priv)
 
 	return regmap_write(priv->regs, MTK_STAR_REG_TEST0, delay_val);
 }
+
 static int mtk_star_probe(struct platform_device *pdev)
 {
 	struct device_node *of_node;

spurious whitespace change

Yes, will fix it in next send.

Best Regards!
Biao