netdev - Re: [PATCH v2 4/4] net: lantiq: Disable IRQs only if NAPI gets scheduled

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] [day] [month] [year] [list]

Message-ID: <b3c182e4-8c68-d142-38c4-b1b99d343e9d@hauke-m.de>
Date:   Tue, 15 Sep 2020 01:06:38 +0200
From:   Hauke Mehrtens <hauke@...ke-m.de>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        martin.blumenstingl@...glemail.com, eric.dumazet@...il.com
Subject: Re: [PATCH v2 4/4] net: lantiq: Disable IRQs only if NAPI gets
 scheduled

On 9/14/20 10:54 PM, Jakub Kicinski wrote:
> On Sat, 12 Sep 2020 21:36:29 +0200 Hauke Mehrtens wrote:
>> The napi_schedule() call will only schedule the NAPI if it is not
>> already running. To make sure that we do not deactivate interrupts
>> without scheduling NAPI only deactivate the interrupts in case NAPI also
>> gets scheduled.
>>
>> Signed-off-by: Hauke Mehrtens <hauke@...ke-m.de>
>> ---
>>  drivers/net/ethernet/lantiq_xrx200.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/lantiq_xrx200.c b/drivers/net/ethernet/lantiq_xrx200.c
>> index abee7d61074c..635ff3a5dcfb 100644
>> --- a/drivers/net/ethernet/lantiq_xrx200.c
>> +++ b/drivers/net/ethernet/lantiq_xrx200.c
>> @@ -345,10 +345,12 @@ static irqreturn_t xrx200_dma_irq(int irq, void *ptr)
>>  {
>>  	struct xrx200_chan *ch = ptr;
>>  
>> -	ltq_dma_disable_irq(&ch->dma);
>> -	ltq_dma_ack_irq(&ch->dma);
>> +	if (napi_schedule_prep(&ch->napi)) {
>> +		__napi_schedule(&ch->napi);
>> +		ltq_dma_disable_irq(&ch->dma);
>> +	}
>>  
>> -	napi_schedule(&ch->napi);
>> +	ltq_dma_ack_irq(&ch->dma);
>>  
>>  	return IRQ_HANDLED;
>>  }
> 
> The patches look good to me, but I wonder why you don't want to always
> disable the IRQ here? You're guaranteed that NAPI will get called, or it
> was disabled. In both cases seems like disabling the IRQ can only help.
> 

This was more or less copied from the mtk_handle_irq_rx() function of
this driver:
drivers/net/ethernet/mediatek/mtk_eth_soc.c

My assumption was that napi_schedule_prep() could return false and then
NAPI would not be triggered and the IRQ would be deactivated. In such a
case the network would hang.

I observed such hangs of the TX, which were mostly fixed by the first
patch in this series, but I still saw some of them even with that first
patch. With this last patch I did not see them any more, but I am not
100% if all possible cases are eliminated.

Hauke



Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)