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

[Zhu Yanjun <yanjun.zhu@oracle.com>]

On 2019/11/6 12:48, Jakub Kicinski wrote:
> 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);
> 	}

Thanks a lot. Will send a new patch with the above changes.

Zhu Yanjun

>