| Message ID | 20200912193629.1586-5-hauke@hauke-m.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/4] net: lantiq: Wake TX queue again | expand |
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.
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
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 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(-)