netdev - Re: [PATCHv4 1/1] net: forcedeth: add xmit

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: <20191105094841.623b498e@cakuba.netronome.com>
Date:   Tue, 5 Nov 2019 09:48:41 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Zhu Yanjun <yanjun.zhu@...cle.com>
Cc:     rain.1986.08.12@...il.com, davem@...emloft.net,
        netdev@...r.kernel.org
Subject: Re: [PATCHv4 1/1] net: forcedeth: add xmit_more support

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!