Re: [PATCHv4 1/1] net: forcedeth: add xmit_more support

From: Jakub Kicinski <jakub.kicinski@netronome.com>
To: Zhu Yanjun <yanjun.zhu@oracle.com>
Cc: rain.1986.08.12@gmail.com, davem@davemloft.net, netdev@vger.kernel.org
Subject: Re: [PATCHv4 1/1] net: forcedeth: add xmit_more support
Date: Tue, 5 Nov 2019 09:48:41 -0800	[thread overview]
Message-ID: <20191105094841.623b498e@cakuba.netronome.com> (raw)
In-Reply-To: <1572928001-6915-1-git-send-email-yanjun.zhu@oracle.com>

On Mon,  4 Nov 2019 23:26:41 -0500, Zhu Yanjun wrote:
> diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
> index 05d2b47..0d21ddd 100644
> --- a/drivers/net/ethernet/nvidia/forcedeth.c
> +++ b/drivers/net/ethernet/nvidia/forcedeth.c
> @@ -2259,7 +2265,12 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  			u64_stats_update_begin(&np->swstats_tx_syncp);
>  			nv_txrx_stats_inc(stat_tx_dropped);
>  			u64_stats_update_end(&np->swstats_tx_syncp);
> -			return NETDEV_TX_OK;
> +
> +			writel(NVREG_TXRXCTL_KICK | np->txrxctl_bits,
> +			       get_hwbase(dev) + NvRegTxRxControl);
> +			ret = NETDEV_TX_OK;
> +
> +			goto dma_error;

You could goto the middle of the txkick if statement here, instead of
duplicating the writel()? Actually the txkick label could be in the
middle of the if statement to begin with, TXBUSY case above stops the
queue so it will always go into the if.

>  		}
>  		np->put_tx_ctx->dma_len = bcnt;
>  		np->put_tx_ctx->dma_single = 1;
> @@ -2305,7 +2316,12 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  				u64_stats_update_begin(&np->swstats_tx_syncp);
>  				nv_txrx_stats_inc(stat_tx_dropped);
>  				u64_stats_update_end(&np->swstats_tx_syncp);
> -				return NETDEV_TX_OK;
> +
> +				writel(NVREG_TXRXCTL_KICK | np->txrxctl_bits,
> +				       get_hwbase(dev) + NvRegTxRxControl);
> +				ret = NETDEV_TX_OK;
> +
> +				goto dma_error;

And here.

>  			}
>  
>  			np->put_tx_ctx->dma_len = bcnt;
> @@ -2357,8 +2373,15 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	spin_unlock_irqrestore(&np->lock, flags);
>  
> -	writel(NVREG_TXRXCTL_KICK|np->txrxctl_bits, get_hwbase(dev) + NvRegTxRxControl);
> -	return NETDEV_TX_OK;
> +txkick:
> +	if (netif_queue_stopped(dev) || !netdev_xmit_more()) {
> +		u32 txrxctl_kick = NVREG_TXRXCTL_KICK | np->txrxctl_bits;
> +
> +		writel(txrxctl_kick, get_hwbase(dev) + NvRegTxRxControl);
> +	}
> +
> +dma_error:
> +	return ret;
>  }

But otherwise looks correct to me now, thanks!