| Message ID | 20200815183314.404-3-hauke@hauke-m.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/3] net: lantiq: Wake TX queue again | expand |
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.
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
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;
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; >
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; }
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(-)