netdev - Re: [PATCH net-next] net: enc28j60: Use threaded interrupt instead of workqueue

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]

Message-ID: <ZFoQ5EN+XPXjzQ2e@nimitz>
Date: Tue, 9 May 2023 11:22:44 +0200
From: Piotr Raczynski <piotr.raczynski@...el.com>
To: Lukas Wunner <lukas@...ner.de>
CC: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, <netdev@...r.kernel.org>, Philipp Rosenberger
	<p.rosenberger@...bus.com>, Zhi Han <hanzhi09@...il.com>
Subject: Re: [PATCH net-next] net: enc28j60: Use threaded interrupt instead
 of workqueue

On Tue, May 09, 2023 at 06:28:56AM +0200, Lukas Wunner wrote:
> From: Philipp Rosenberger <p.rosenberger@...bus.com>
> 
> The Microchip ENC28J60 SPI Ethernet driver schedules a work item from
> the interrupt handler because accesses to the SPI bus may sleep.
> 
> On PREEMPT_RT (which forces interrupt handling into threads) this
> old-fashioned approach unnecessarily increases latency because an
> interrupt results in first waking the interrupt thread, then scheduling
> the work item.  So, a double indirection to handle an interrupt.
> 
> Avoid by converting the driver to modern threaded interrupt handling.
> 
> Signed-off-by: Philipp Rosenberger <p.rosenberger@...bus.com>
> Signed-off-by: Zhi Han <hanzhi09@...il.com>
> [lukas: rewrite commit message, linewrap request_threaded_irq() call]
> Signed-off-by: Lukas Wunner <lukas@...ner.de>

Other than commit message commented by Leon, LGTM.
Reviewed-by: Piotr Raczynski <piotr.raczynski@...el.com>
> ---
>  drivers/net/ethernet/microchip/enc28j60.c | 28 +++++------------------
>  1 file changed, 6 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/enc28j60.c b/drivers/net/ethernet/microchip/enc28j60.c
> index 176efbeae127..d6c9491537e4 100644
> --- a/drivers/net/ethernet/microchip/enc28j60.c
> +++ b/drivers/net/ethernet/microchip/enc28j60.c
> @@ -58,7 +58,6 @@ struct enc28j60_net {
>  	struct mutex lock;
>  	struct sk_buff *tx_skb;
>  	struct work_struct tx_work;
> -	struct work_struct irq_work;
>  	struct work_struct setrx_work;
>  	struct work_struct restart_work;
>  	u8 bank;		/* current register bank selected */
> @@ -1118,10 +1117,9 @@ static int enc28j60_rx_interrupt(struct net_device *ndev)
>  	return ret;
>  }
>  
> -static void enc28j60_irq_work_handler(struct work_struct *work)
> +static irqreturn_t enc28j60_irq(int irq, void *dev_id)
>  {
> -	struct enc28j60_net *priv =
> -		container_of(work, struct enc28j60_net, irq_work);
> +	struct enc28j60_net *priv = dev_id;
>  	struct net_device *ndev = priv->netdev;
>  	int intflags, loop;
>  
> @@ -1225,6 +1223,8 @@ static void enc28j60_irq_work_handler(struct work_struct *work)
>  
>  	/* re-enable interrupts */
>  	locked_reg_bfset(priv, EIE, EIE_INTIE);
> +
> +	return IRQ_HANDLED;
>  }
>  
>  /*
> @@ -1309,22 +1309,6 @@ static void enc28j60_tx_work_handler(struct work_struct *work)
>  	enc28j60_hw_tx(priv);
>  }
>  
> -static irqreturn_t enc28j60_irq(int irq, void *dev_id)
> -{
> -	struct enc28j60_net *priv = dev_id;
> -
> -	/*
> -	 * Can't do anything in interrupt context because we need to
> -	 * block (spi_sync() is blocking) so fire of the interrupt
> -	 * handling workqueue.
> -	 * Remember that we access enc28j60 registers through SPI bus
> -	 * via spi_sync() call.
> -	 */
> -	schedule_work(&priv->irq_work);
> -
> -	return IRQ_HANDLED;
> -}
> -
>  static void enc28j60_tx_timeout(struct net_device *ndev, unsigned int txqueue)
>  {
>  	struct enc28j60_net *priv = netdev_priv(ndev);
> @@ -1559,7 +1543,6 @@ static int enc28j60_probe(struct spi_device *spi)
>  	mutex_init(&priv->lock);
>  	INIT_WORK(&priv->tx_work, enc28j60_tx_work_handler);
>  	INIT_WORK(&priv->setrx_work, enc28j60_setrx_work_handler);
> -	INIT_WORK(&priv->irq_work, enc28j60_irq_work_handler);
>  	INIT_WORK(&priv->restart_work, enc28j60_restart_work_handler);
>  	spi_set_drvdata(spi, priv);	/* spi to priv reference */
>  	SET_NETDEV_DEV(dev, &spi->dev);
> @@ -1578,7 +1561,8 @@ static int enc28j60_probe(struct spi_device *spi)
>  	/* Board setup must set the relevant edge trigger type;
>  	 * level triggers won't currently work.
>  	 */
> -	ret = request_irq(spi->irq, enc28j60_irq, 0, DRV_NAME, priv);
> +	ret = request_threaded_irq(spi->irq, NULL, enc28j60_irq, IRQF_ONESHOT,
> +				   DRV_NAME, priv);
>  	if (ret < 0) {
>  		if (netif_msg_probe(priv))
>  			dev_err(&spi->dev, "request irq %d failed (ret = %d)\n",
> -- 
> 2.39.2
> 
>