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: <20230509080627.GF38143@unreal>
Date: Tue, 9 May 2023 11:06:27 +0300
From: Leon Romanovsky <leon@...nel.org>
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]

This is part of changelog which doesn't belong to commit message. The
examples which you can find in git log, for such format like you used,
are usually reserved to maintainers when they apply the patch.

Thanks

> Signed-off-by: Lukas Wunner <lukas@...ner.de>
> ---
>  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
> 
>