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

On Fri, 28 Jan 2022 15:05:27 +0800 Biao Huang wrote:

quoted

quoted

+ * Description : this is the driver interrupt service routine.
+ * it mainly handles:
+ *  1. tx complete interrupt for frame transmission.
+ *  2. rx complete interrupt for frame reception.
+ *  3. MAC Management Counter interrupt to avoid counter overflow.
  */
 static irqreturn_t mtk_star_handle_irq(int irq, void *data)
 {
-	struct mtk_star_priv *priv;
-	struct net_device *ndev;
+	struct net_device *ndev = data;
+	struct mtk_star_priv *priv = netdev_priv(ndev);
+	unsigned int intr_status = mtk_star_intr_ack_all(priv);
+	unsigned long flags = 0;
+
+	if (intr_status & MTK_STAR_BIT_INT_STS_FNRC) {
+		if (napi_schedule_prep(&priv->rx_napi)) {
+			spin_lock_irqsave(&priv->lock, flags);
+			/* mask Rx Complete interrupt */
+			mtk_star_disable_dma_irq(priv, true, false);
+			spin_unlock_irqrestore(&priv->lock, flags);
+			__napi_schedule_irqoff(&priv->rx_napi);
+		}
+	}
 
-	ndev = data;
-	priv = netdev_priv(ndev);
+	if (intr_status & MTK_STAR_BIT_INT_STS_TNTC) {
+		if (napi_schedule_prep(&priv->tx_napi)) {
+			spin_lock_irqsave(&priv->lock, flags);
+			/* mask Tx Complete interrupt */
+			mtk_star_disable_dma_irq(priv, false, true);
+			spin_unlock_irqrestore(&priv->lock, flags);
+			__napi_schedule_irqoff(&priv->tx_napi);
+		}
+	}  

Seems a little wasteful to retake the same lock twice if two IRQ
sources fire at the same time.  

The TX/RX irq control bits are in the same register,
but they are triggered independently.
So it seems necessary to protect the register
access with a spin lock.

This is what I meant:

rx = (status & RX) && napi_schedule_prep(rx_napi);
tx = (status & TX) && napi_schedule_prep(tx_napi);

if (rx || tx) {
	spin_lock()
	disable_irq(priv, rx, tx);	
	spin_unlock();
	if (rx)
		__napi_schedule_irqoff(rx_napi)
	if (tx)
		__napi_schedule_irqoff(tx_napi)
}

quoted

quoted

 	desc_data.dma_addr = mtk_star_dma_map_tx(priv, skb);
 	if (dma_mapping_error(dev, desc_data.dma_addr))

@@ -1050,18 +1103,10 @@ static int

mtk_star_netdev_start_xmit(struct sk_buff *skb,
 
 	desc_data.skb = skb;
 	desc_data.len = skb->len;
-
-	spin_lock_bh(&priv->lock);

 	mtk_star_ring_push_head_tx(ring, &desc_data);
 
 	netdev_sent_queue(ndev, skb->len);
 
-	if (mtk_star_ring_full(ring))
-		netif_stop_queue(ndev);  

Are you stopping the queue in advance somewhere else now? Did you
only
test this with BQL enabled? Only place that stops the ring also
prints
a loud warning now AFAICS..  

No.

We modify the ring full condition, and will not invoke netif_stop_queue
if queue is already stopped.

I don't understand what you're saying.

Test pass no matter whether BQL is enabled or disabled.

It's much safer to judge queue is full or not at the beginning of
start_xmit() to avoid invalid setting.

Drivers are expected to stop their queues at the end of xmit routine if
the ring can't accommodate another frame. It's more efficient to stop
the queues early than have to put skbs already dequeued from the qdisc
layer back into the qdiscs.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel