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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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

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