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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 14 Dec 2019 12:36:23 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Jose Abreu <Jose.Abreu@...opsys.com>
Cc:     netdev@...r.kernel.org, Joao Pinto <Joao.Pinto@...opsys.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        "David S. Miller" <davem@...emloft.net>,
        Maxime Ripard <mripard@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        linux-arm-kernel@...ts.infradead.org,
        linux-stm32@...md-mailman.stormreply.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 3/4] net: stmmac: Let TX and RX interrupts be
 independently enabled/disabled

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;