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
