Re: [PATCH net-next 3/4] net: stmmac: Let TX and RX interrupts be independently enabled/disabled

From: Jakub Kicinski
Date: Sat Dec 14 2019 - 15:36:30 EST


On Tue, 10 Dec 2019 20:54:43 +0100, Jose Abreu wrote:
> @@ -2278,10 +2286,14 @@ static void stmmac_tx_timer(struct timer_list *t)
> * If NAPI is already running we can miss some events. Let's rearm
> * the timer and try again.
> */
> - if (likely(napi_schedule_prep(&ch->tx_napi)))
> + if (likely(napi_schedule_prep(&ch->tx_napi))) {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ch->lock, flags);
> + stmmac_disable_dma_irq(priv, priv->ioaddr, ch->index, 0, 1);
> + spin_unlock_irqrestore(&ch->lock, flags);
> __napi_schedule(&ch->tx_napi);
> - else
> - mod_timer(&tx_q->txtimer, STMMAC_COAL_TIMER(10));

You should also remove the comment above the if statement if it's
really okay to no longer re-arm the timer. No?

> + }
> }
>
> /**

> @@ -3759,24 +3777,18 @@ static int stmmac_napi_poll_tx(struct napi_struct *napi, int budget)
> struct stmmac_channel *ch =
> container_of(napi, struct stmmac_channel, tx_napi);
> struct stmmac_priv *priv = ch->priv_data;
> - struct stmmac_tx_queue *tx_q;
> u32 chan = ch->index;
> int work_done;
>
> priv->xstats.napi_poll++;
>
> - work_done = stmmac_tx_clean(priv, DMA_TX_SIZE, chan);
> - work_done = min(work_done, budget);
> -
> - if (work_done < budget)
> - napi_complete_done(napi, work_done);
> + work_done = stmmac_tx_clean(priv, budget, chan);
> + if (work_done < budget && napi_complete_done(napi, work_done)) {

Not really related to this patch, but this looks a little suspicious.
I think the TX completions should all be processed regardless of the
budget. The budget is for RX.

> + unsigned long flags;
>
> - /* Force transmission restart */
> - tx_q = &priv->tx_queue[chan];
> - if (tx_q->cur_tx != tx_q->dirty_tx) {
> - stmmac_enable_dma_transmission(priv, priv->ioaddr);
> - stmmac_set_tx_tail_ptr(priv, priv->ioaddr, tx_q->tx_tail_addr,
> - chan);
> + spin_lock_irqsave(&ch->lock, flags);
> + stmmac_enable_dma_irq(priv, priv->ioaddr, chan, 0, 1);
> + spin_unlock_irqrestore(&ch->lock, flags);
> }
>
> return work_done;