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

[Jose Abreu]

From: Jakub Kicinski <jakub.kicinski@xxxxxxxxxxxxx>
Date: Dec/14/2019, 20:36:23 (UTC+00:00)

> 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?

Yeah, agreed!

> 
> > +	}
> >  }
> >  
> >  /**
> 
> > @@ -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.

Well but this is a TX NAPI ... Shouldn't it be limited to prevent CPU 
starvation ?

---
Thanks,
Jose Miguel Abreu