netdev - Re: [PATCH 3/3] net: lantiq: Use napi_complete

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: <28b6bd3f-761c-9143-e90d-30af5d76e3ed@gmail.com>
Date:   Mon, 17 Aug 2020 17:33:52 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Hauke Mehrtens <hauke@...ke-m.de>,
        Eric Dumazet <eric.dumazet@...il.com>, davem@...emloft.net
Cc:     kuba@...nel.org, netdev@...r.kernel.org,
        martin.blumenstingl@...glemail.com
Subject: Re: [PATCH 3/3] net: lantiq: Use napi_complete_done()



On 8/17/20 2:17 PM, Hauke Mehrtens wrote:
> On 8/16/20 8:07 PM, Eric Dumazet wrote:
>>
>>
>> On 8/15/20 11:33 AM, Hauke Mehrtens wrote:
>>> Use napi_complete_done() and activate the interrupts when this function
>>> returns true. This way the generic NAPI code can take care of activating
>>> the interrupts.
>>>
>>> Signed-off-by: Hauke Mehrtens <hauke@...ke-m.de>
>>> ---
>>>  drivers/net/ethernet/lantiq_xrx200.c | 8 ++------
>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/lantiq_xrx200.c b/drivers/net/ethernet/lantiq_xrx200.c
>>> index f34e4dc8c661..674ffb2ecd9a 100644
>>> --- a/drivers/net/ethernet/lantiq_xrx200.c
>>> +++ b/drivers/net/ethernet/lantiq_xrx200.c
>>> @@ -229,10 +229,8 @@ static int xrx200_poll_rx(struct napi_struct *napi, int budget)
>>>  		}
>>>  	}
>>>  
>>> -	if (rx < budget) {
>>> -		napi_complete(&ch->napi);
>>> +	if (napi_complete_done(&ch->napi, rx))
>>>  		ltq_dma_enable_irq(&ch->dma);
>>> -	}
>>>  
>>>  	return rx;
>>>  }
>>> @@ -271,10 +269,8 @@ static int xrx200_tx_housekeeping(struct napi_struct *napi, int budget)
>>>  	if (netif_queue_stopped(net_dev))
>>>  		netif_wake_queue(net_dev);
>>>  
>>> -	if (pkts < budget) {
>>> -		napi_complete(&ch->napi);
>>> +	if (napi_complete_done(&ch->napi, pkts))
>>>  		ltq_dma_enable_irq(&ch->dma);
>>> -	}
>>>  
>>>  	return pkts;
>>>  }
>>>
>>
>>
>> This looks buggy to me.
> 
> Hi Eric,
> 
> Thanks for looking at the patch.
> 
> What exactly looks buggy to you?

You removed the " if (rx < budget) "

But you must not.

Drivers have to keep the test.

Something like that seems more correct :

diff --git a/drivers/net/ethernet/lantiq_xrx200.c b/drivers/net/ethernet/lantiq_xrx200.c
index 1645e4e7ebdbb3c7abff8fe4207273df20f123d4..e3d617d387ed0f5593c3ba81d1d531d463bb5a6e 100644
--- a/drivers/net/ethernet/lantiq_xrx200.c
+++ b/drivers/net/ethernet/lantiq_xrx200.c
@@ -230,8 +230,8 @@ static int xrx200_poll_rx(struct napi_struct *napi, int budget)
        }
 
        if (rx < budget) {
-               napi_complete(&ch->napi);
-               ltq_dma_enable_irq(&ch->dma);
+               if (napi_complete(&ch->napi, rx))
+                       ltq_dma_enable_irq(&ch->dma);
        }
 
        return rx;
@@ -269,8 +269,8 @@ static int xrx200_tx_housekeeping(struct napi_struct *napi, int budget)
        netdev_completed_queue(ch->priv->net_dev, pkts, bytes);
 
        if (pkts < budget) {
-               napi_complete(&ch->napi);
-               ltq_dma_enable_irq(&ch->dma);
+               if (napi_complete(&ch->napi, pkts))
+                       ltq_dma_enable_irq(&ch->dma);
        }
 
        return pkts;