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

From: Jakub Kicinski <kuba@kernel.org>
To: Biao Huang <biao.huang@mediatek.com>
Cc: David Miller <davem@davemloft.net>,
	Rob Herring <robh+dt@kernel.org>,
	Bartosz Golaszewski <brgl@bgdev.pl>,
	Fabien Parent <fparent@baylibre.com>,
	Felix Fietkau <nbd@nbd.name>, "John Crispin" <john@phrozen.org>,
	Sean Wang <sean.wang@mediatek.com>,
	Mark Lee <Mark-MC.Lee@mediatek.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	<netdev@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-mediatek@lists.infradead.org>,
	Yinghua Pan <ot_yinghua.pan@mediatek.com>,
	<srv_heupstream@mediatek.com>,
	Macpaul Lin <macpaul.lin@mediatek.com>
Subject: Re: [PATCH net-next v3 09/10] net: ethernet: mtk-star-emac: separate tx/rx handling with two NAPIs
Date: Thu, 23 Jun 2022 21:34:31 -0700	[thread overview]
Message-ID: <20220623213431.23528544@kernel.org> (raw)
In-Reply-To: <20220622090545.23612-10-biao.huang@mediatek.com>

On Wed, 22 Jun 2022 17:05:44 +0800 Biao Huang wrote:
> +	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

> +		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?

> -	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.

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

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

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

>  		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?

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