* [PATCH 1/3] net: lantiq: Wake TX queue again
@ 2020-08-15 18:33 Hauke Mehrtens
2020-08-15 18:33 ` [PATCH 2/3] net: lantiq: use netif_tx_napi_add() for TX NAPI Hauke Mehrtens
2020-08-15 18:33 ` [PATCH 3/3] net: lantiq: Use napi_complete_done() Hauke Mehrtens
0 siblings, 2 replies; 8+ messages in thread
From: Hauke Mehrtens @ 2020-08-15 18:33 UTC (permalink / raw)
To: davem; +Cc: kuba, netdev, martin.blumenstingl, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/3] net: lantiq: use netif_tx_napi_add() for TX NAPI 2020-08-15 18:33 [PATCH 1/3] net: lantiq: Wake TX queue again Hauke Mehrtens @ 2020-08-15 18:33 ` Hauke Mehrtens 2020-08-16 18:05 ` Eric Dumazet 2020-08-15 18:33 ` [PATCH 3/3] net: lantiq: Use napi_complete_done() Hauke Mehrtens 1 sibling, 1 reply; 8+ messages in thread From: Hauke Mehrtens @ 2020-08-15 18:33 UTC (permalink / raw) To: davem; +Cc: kuba, netdev, martin.blumenstingl, Hauke Mehrtens netif_tx_napi_add() should be used for NAPI in the TX direction instead of the netif_napi_add() function. 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 | 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] net: lantiq: use netif_tx_napi_add() for TX NAPI 2020-08-15 18:33 ` [PATCH 2/3] net: lantiq: use netif_tx_napi_add() for TX NAPI Hauke Mehrtens @ 2020-08-16 18:05 ` Eric Dumazet 0 siblings, 0 replies; 8+ messages in thread From: Eric Dumazet @ 2020-08-16 18:05 UTC (permalink / raw) To: Hauke Mehrtens, davem; +Cc: kuba, netdev, martin.blumenstingl On 8/15/20 11:33 AM, Hauke Mehrtens wrote: > netif_tx_napi_add() should be used for NAPI in the TX direction instead > of the netif_napi_add() function. > > 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 | 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); > > This is not a must, simply a matter of (small) optimization in the very rare case of busy polling users. The Fixes: tag here is not necessary. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] net: lantiq: Use napi_complete_done() 2020-08-15 18:33 [PATCH 1/3] net: lantiq: Wake TX queue again Hauke Mehrtens 2020-08-15 18:33 ` [PATCH 2/3] net: lantiq: use netif_tx_napi_add() for TX NAPI Hauke Mehrtens @ 2020-08-15 18:33 ` Hauke Mehrtens 2020-08-16 18:07 ` Eric Dumazet 1 sibling, 1 reply; 8+ messages in thread From: Hauke Mehrtens @ 2020-08-15 18:33 UTC (permalink / raw) To: davem; +Cc: kuba, netdev, martin.blumenstingl, 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, 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; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] net: lantiq: Use napi_complete_done() 2020-08-15 18:33 ` [PATCH 3/3] net: lantiq: Use napi_complete_done() Hauke Mehrtens @ 2020-08-16 18:07 ` Eric Dumazet 2020-08-17 21:17 ` Hauke Mehrtens 0 siblings, 1 reply; 8+ messages in thread From: Eric Dumazet @ 2020-08-16 18:07 UTC (permalink / raw) To: Hauke Mehrtens, davem; +Cc: kuba, netdev, martin.blumenstingl 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@hauke-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. Please look again to other implementations for a correct usage. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] net: lantiq: Use napi_complete_done() 2020-08-16 18:07 ` Eric Dumazet @ 2020-08-17 21:17 ` Hauke Mehrtens 2020-08-18 0:33 ` Eric Dumazet 0 siblings, 1 reply; 8+ messages in thread From: Hauke Mehrtens @ 2020-08-17 21:17 UTC (permalink / raw) To: Eric Dumazet, davem; +Cc: kuba, netdev, martin.blumenstingl [-- Attachment #1.1: Type: text/plain, Size: 1956 bytes --] 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@hauke-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? We see with and also without this patch problems in the TX path, it looks like the netif tx queue is not started again. > Please look again to other implementations for a correct usage. I looked at multiple drivers and they look similar in this area. For example microchip/lan743x_main.c is looking similar to me. The hardware has two interrupts one for the RX and one for TX complete. Can you please suggest a driver which uses the NAPI in a good way and is not very complex. Hauke [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] net: lantiq: Use napi_complete_done() 2020-08-17 21:17 ` Hauke Mehrtens @ 2020-08-18 0:33 ` Eric Dumazet 2020-08-18 0:48 ` Eric Dumazet 0 siblings, 1 reply; 8+ messages in thread From: Eric Dumazet @ 2020-08-18 0:33 UTC (permalink / raw) To: Hauke Mehrtens, Eric Dumazet, davem; +Cc: kuba, netdev, martin.blumenstingl 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@hauke-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; ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] net: lantiq: Use napi_complete_done() 2020-08-18 0:33 ` Eric Dumazet @ 2020-08-18 0:48 ` Eric Dumazet 0 siblings, 0 replies; 8+ messages in thread From: Eric Dumazet @ 2020-08-18 0:48 UTC (permalink / raw) To: Eric Dumazet, Hauke Mehrtens, davem; +Cc: kuba, netdev, martin.blumenstingl On 8/17/20 5:33 PM, Eric Dumazet wrote: > > > 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@hauke-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)) Obviously : s/napi_complete/napi_complete_done/ > + 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)) Same : s/napi_complete/napi_complete_done/ > + ltq_dma_enable_irq(&ch->dma); > } > > return pkts; > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-08-18 0:48 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-15 18:33 [PATCH 1/3] net: lantiq: Wake TX queue again Hauke Mehrtens 2020-08-15 18:33 ` [PATCH 2/3] net: lantiq: use netif_tx_napi_add() for TX NAPI Hauke Mehrtens 2020-08-16 18:05 ` Eric Dumazet 2020-08-15 18:33 ` [PATCH 3/3] net: lantiq: Use napi_complete_done() Hauke Mehrtens 2020-08-16 18:07 ` Eric Dumazet 2020-08-17 21:17 ` Hauke Mehrtens 2020-08-18 0:33 ` Eric Dumazet 2020-08-18 0:48 ` Eric Dumazet
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.