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

[Jakub Kicinski]

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;