[PATCH v2 0/4] net: lantiq: Fix bugs in NAPI handling

[PATCH v2 0/4] net: lantiq: Fix bugs in NAPI handling

[Hauke Mehrtens]

This fixes multiple bugs in the NAPI handling.

Changes since:
v1:
 - removed stable tag from "net: lantiq: use netif_tx_napi_add() for TX NAPI"
 - Check the NAPI budged in "net: lantiq: Use napi_complete_done()"
 - Add extra fix "net: lantiq: Disable IRQs only if NAPI gets scheduled"

Hauke Mehrtens (4):
  net: lantiq: Wake TX queue again
  net: lantiq: use netif_tx_napi_add() for TX NAPI
  net: lantiq: Use napi_complete_done()
  net: lantiq: Disable IRQs only if NAPI gets scheduled

 drivers/net/ethernet/lantiq_xrx200.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH v2 1/4] net: lantiq: Wake TX queue again

[Hauke Mehrtens]

The call to netif_wake_queue() when the TX descriptors were freed was
missing. When there are no TX buffers available the TX queue will be
stopped, but it was not started again when they are available again,
this is fixed in this patch.

Fixes: fe1a56420cf2 ("net: lantiq: Add Lantiq / Intel VRX200 Ethernet driver")
Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/net/ethernet/lantiq_xrx200.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/lantiq_xrx200.c b/drivers/net/ethernet/lantiq_xrx200.c
index 1645e4e7ebdb..1feb9fc710e0 100644
--- a/drivers/net/ethernet/lantiq_xrx200.c
+++ b/drivers/net/ethernet/lantiq_xrx200.c
@@ -268,6 +268,9 @@ static int xrx200_tx_housekeeping(struct napi_struct *napi, int budget)
 	net_dev->stats.tx_bytes += bytes;
 	netdev_completed_queue(ch->priv->net_dev, pkts, bytes);
 
+	if (netif_queue_stopped(net_dev))
+		netif_wake_queue(net_dev);
+
 	if (pkts < budget) {
 		napi_complete(&ch->napi);
 		ltq_dma_enable_irq(&ch->dma);
-- 
2.20.1

[PATCH v2 2/4] net: lantiq: use netif_tx_napi_add() for TX NAPI

[Hauke Mehrtens]

netif_tx_napi_add() should be used for NAPI in the TX direction instead
of the netif_napi_add() function.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/net/ethernet/lantiq_xrx200.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/lantiq_xrx200.c b/drivers/net/ethernet/lantiq_xrx200.c
index 1feb9fc710e0..f34e4dc8c661 100644
--- a/drivers/net/ethernet/lantiq_xrx200.c
+++ b/drivers/net/ethernet/lantiq_xrx200.c
@@ -502,7 +502,7 @@ static int xrx200_probe(struct platform_device *pdev)
 
 	/* setup NAPI */
 	netif_napi_add(net_dev, &priv->chan_rx.napi, xrx200_poll_rx, 32);
-	netif_napi_add(net_dev, &priv->chan_tx.napi, xrx200_tx_housekeeping, 32);
+	netif_tx_napi_add(net_dev, &priv->chan_tx.napi, xrx200_tx_housekeeping, 32);
 
 	platform_set_drvdata(pdev, priv);
 
-- 
2.20.1

[PATCH v2 3/4] net: lantiq: Use napi_complete_done()

[Hauke Mehrtens]

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@hauke-m.de>
---
 drivers/net/ethernet/lantiq_xrx200.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/lantiq_xrx200.c b/drivers/net/ethernet/lantiq_xrx200.c
index f34e4dc8c661..abee7d61074c 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_done(&ch->napi, rx))
+			ltq_dma_enable_irq(&ch->dma);
 	}
 
 	return rx;
@@ -272,8 +272,8 @@ static int xrx200_tx_housekeeping(struct napi_struct *napi, int budget)
 		netif_wake_queue(net_dev);
 
 	if (pkts < budget) {
-		napi_complete(&ch->napi);
-		ltq_dma_enable_irq(&ch->dma);
+		if (napi_complete_done(&ch->napi, pkts))
+			ltq_dma_enable_irq(&ch->dma);
 	}
 
 	return pkts;
-- 
2.20.1

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

[Hauke Mehrtens]

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@hauke-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;
 }
-- 
2.20.1

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

[Jakub Kicinski]

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@hauke-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.

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

[Hauke Mehrtens]

[-- Attachment #1.1: Type: text/plain, Size: 1998 bytes --]

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@hauke-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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/4] net: lantiq: Fix bugs in NAPI handling

[Martin Blumenstingl]

Hi Hauke,

On Sat, Sep 12, 2020 at 9:36 PM Hauke Mehrtens <hauke@hauke-m.de> wrote:
>
> This fixes multiple bugs in the NAPI handling.
many thanks!
These fix the TX hang that I could reproduce by simply starting iperf3
on a lantiq board.

Is the plan to have these patches applied to net or net-next?
Having them in the net tree would be great so these can be backported
to 5.4 (which is what OpenWrt uses)

> Hauke Mehrtens (4):
>   net: lantiq: Wake TX queue again
>   net: lantiq: use netif_tx_napi_add() for TX NAPI
>   net: lantiq: Use napi_complete_done()
>   net: lantiq: Disable IRQs only if NAPI gets scheduled
for all four:
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>


Thank you!
Martin

Re: [PATCH v2 0/4] net: lantiq: Fix bugs in NAPI handling

[David Miller]

From: Hauke Mehrtens <hauke@hauke-m.de>
Date: Sat, 12 Sep 2020 21:36:25 +0200

> This fixes multiple bugs in the NAPI handling.
> 
> Changes since:
> v1:
>  - removed stable tag from "net: lantiq: use netif_tx_napi_add() for TX NAPI"
>  - Check the NAPI budged in "net: lantiq: Use napi_complete_done()"
>  - Add extra fix "net: lantiq: Disable IRQs only if NAPI gets scheduled"

Series applied and queued up for -stable.

Please answer Jakub's question about patch #4.

Thanks.