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

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Wed, 6 Nov 2019 12:47:29 +0800, Zhu Yanjun wrote:
> On 2019/11/6 1:48, Jakub Kicinski wrote:
> > 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()?  
> As your suggestion, the change is like this:
> 
> @@ -2374,7 +2374,9 @@ static netdev_tx_t nv_start_xmit(struct sk_buff 
> *skb, struct net_device *dev)
>          spin_unlock_irqrestore(&np->lock, flags);
> 
>   txkick:
> -       if (netif_queue_stopped(dev) || !netdev_xmit_more()) {
> +       if (netif_queue_stopped(dev) || !netdev_xmit_more())
> +dma_error:
> +       {
>                  u32 txrxctl_kick = NVREG_TXRXCTL_KICK | np->txrxctl_bits;
> 
>                  writel(txrxctl_kick, get_hwbase(dev) + NvRegTxRxControl);
> 
> The opening brace on the first of the line. It conflicts with the following:
> 
> Documentation/process/coding-style.rst:
> "
>    98 3) Placing Braces and Spaces
>    99 ----------------------------
>   100
>   101 The other issue that always comes up in C styling is the placement of
>   102 braces.  Unlike the indent size, there are few technical reasons to
>   103 choose one placement strategy over the other, but the preferred 
> way, as
>   104 shown to us by the prophets Kernighan and Ritchie, is to put the 
> opening
>   105 brace last on the line, and put the closing brace first, thusly:
> "
> So I prefer to the current code style.
> 
> Thanks for your suggestions.

	if (netif_queue_stopped(dev) || !netdev_xmit_more()) {
		u32 txrxctl_kick;

txkick:
		txrxctl_kick = NVREG_TXRXCTL_KICK | np->txrxctl_bits;
		writel(txrxctl_kick, get_hwbase(dev) + NvRegTxRxControl);
	}