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]

Date:   Wed, 6 Nov 2019 12:47:29 +0800
From:   Zhu Yanjun <yanjun.zhu@...cle.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.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 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.

Any way, it is a code style problem. It is trivial.
>   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!
Thanks a lot for code review.

Zhu Yanjun
>