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

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

[Lukas Wunner]

From: Philipp Rosenberger <p.rosenberger@kunbus.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@kunbus.com>
Signed-off-by: Zhi Han <hanzhi09@gmail.com>
[lukas: rewrite commit message, linewrap request_threaded_irq() call]
Signed-off-by: Lukas Wunner <lukas@wunner.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

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

[Leon Romanovsky]

On Tue, May 09, 2023 at 06:28:56AM +0200, Lukas Wunner wrote:
> From: Philipp Rosenberger <p.rosenberger@kunbus.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@kunbus.com>
> Signed-off-by: Zhi Han <hanzhi09@gmail.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@wunner.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
> 
> 

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

[Lukas Wunner]

On Tue, May 09, 2023 at 11:06:27AM +0300, Leon Romanovsky wrote:
> On Tue, May 09, 2023 at 06:28:56AM +0200, Lukas Wunner wrote:
> > From: Philipp Rosenberger <p.rosenberger@kunbus.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@kunbus.com>
> > Signed-off-by: Zhi Han <hanzhi09@gmail.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.

Is that a new rule?

Honestly I think it's important to mention changes applied to
someone else's patch, if only to let it be known who's to blame
for any mistakes.

I'm seeing plenty of recent precedent in the git history where
non-committers fixed up patches and made their changes known in
this way, e.g.:

commit 99669259f3361d759219811e670b7e0742668556
Author:     Maxime Bizon <mbizon@freebox.fr>
AuthorDate: Thu Mar 16 16:33:16 2023 -0700
Commit:     David S. Miller <davem@davemloft.net>
CommitDate: Sun Mar 19 10:48:35 2023 +0000

    [florian: fix kdoc, added Fixes tag]
    Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

commit 0c9ef08a4d0fd6c5e6000597b506235d71a85a61
Author:     Nathan Huckleberry <nhuck@google.com>
AuthorDate: Tue Nov 8 17:26:30 2022 -0700
Commit:     Paolo Abeni <pabeni@redhat.com>
CommitDate: Thu Nov 10 12:28:30 2022 +0100

    [nathan: Rebase on net-next and resolve conflicts
             Add note about new clang warning]
    Signed-off-by: Nathan Chancellor <nathan@kernel.org>
    Signed-off-by: Paolo Abeni <pabeni@redhat.com>

commit 91e87045a5ef6f7003e9a2cb7dfa435b9b002dbe
Author:     Steffen BÀtz <steffen@innosonix.de>
AuthorDate: Fri Oct 28 13:31:58 2022 -0300
Commit:     Jakub Kicinski <kuba@kernel.org>
CommitDate: Mon Oct 31 20:00:20 2022 -0700

    [fabio: Improved commit log and extended it to mv88e6321_ops]
    Signed-off-by: Fabio Estevam <festevam@denx.de>
    Signed-off-by: Jakub Kicinski <kuba@kernel.org>

commit ebe73a284f4de8c5d401adeccd9b8fe3183b6e95
Author:     David Ahern <dsahern@kernel.org>
AuthorDate: Tue Jul 12 21:52:30 2022 +0100
Commit:     Jakub Kicinski <kuba@kernel.org>
CommitDate: Tue Jul 19 14:20:54 2022 -0700

    [pavel: move callback into msghdr]
    Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
    Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Thanks,

Lukas

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

[Leon Romanovsky]

On Tue, May 09, 2023 at 03:36:20PM +0200, Lukas Wunner wrote:
> On Tue, May 09, 2023 at 11:06:27AM +0300, Leon Romanovsky wrote:
> > On Tue, May 09, 2023 at 06:28:56AM +0200, Lukas Wunner wrote:
> > > From: Philipp Rosenberger <p.rosenberger@kunbus.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@kunbus.com>
> > > Signed-off-by: Zhi Han <hanzhi09@gmail.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.
> 
> Is that a new rule?

No, this rule always existed, just some of the maintainers didn't care
about it.

> 
> Honestly I think it's important to mention changes applied to
> someone else's patch, if only to let it be known who's to blame
> for any mistakes.

Right, this is why maintainers use this notation when they apply
patches. In your case, you are submitter, patch is not applied yet
and all changes can be easily seen through lore web interface.

> 
> I'm seeing plenty of recent precedent in the git history where
> non-committers fixed up patches and made their changes known in
> this way, e.g.:

It doesn't make it correct.
Documentation/maintainer/modifying-patches.rst

Thanks

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

[Jakub Kicinski]

On Tue, 9 May 2023 16:56:13 +0300 Leon Romanovsky wrote:
> > > 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.  
> > 
> > Is that a new rule?  
> 
> No, this rule always existed, just some of the maintainers didn't care
> about it.
> 
> > 
> > Honestly I think it's important to mention changes applied to
> > someone else's patch, if only to let it be known who's to blame
> > for any mistakes.  
> 
> Right, this is why maintainers use this notation when they apply
> patches. In your case, you are submitter, patch is not applied yet
> and all changes can be easily seen through lore web interface.
> 
> > 
> > I'm seeing plenty of recent precedent in the git history where
> > non-committers fixed up patches and made their changes known in
> > this way, e.g.:  
> 
> It doesn't make it correct.
> Documentation/maintainer/modifying-patches.rst

TBH I'm not sure if this is the correct reading of this doc.
I don't see any problem with Lukas using the common notation.
It makes it quite obvious what he changed and the changes are
not invasive enough to warrant a major rewrite of the commit msg.

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

[Paolo Abeni]

On Wed, 2023-05-10 at 19:05 -0700, Jakub Kicinski wrote:
> On Tue, 9 May 2023 16:56:13 +0300 Leon Romanovsky wrote:
> > > > 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.  
> > > 
> > > Is that a new rule?  
> > 
> > No, this rule always existed, just some of the maintainers didn't care
> > about it.
> > 
> > > 
> > > Honestly I think it's important to mention changes applied to
> > > someone else's patch, if only to let it be known who's to blame
> > > for any mistakes.  
> > 
> > Right, this is why maintainers use this notation when they apply
> > patches. In your case, you are submitter, patch is not applied yet
> > and all changes can be easily seen through lore web interface.
> > 
> > > 
> > > I'm seeing plenty of recent precedent in the git history where
> > > non-committers fixed up patches and made their changes known in
> > > this way, e.g.:  
> > 
> > It doesn't make it correct.
> > Documentation/maintainer/modifying-patches.rst
> 
> TBH I'm not sure if this is the correct reading of this doc.
> I don't see any problem with Lukas using the common notation.
> It makes it quite obvious what he changed and the changes are
> not invasive enough to warrant a major rewrite of the commit msg.

My reading of such documentation is that (sub-)maintainers could be
(more frequently) called to this kind of editing, but such editing is
not restricted.

In this specific case I could not find quickly via lore references to
the originating patch, so I find the editing useful.

The rationale of the mentioned process documentation is avoiding - when
possible and sensible - unneeded back and forth: I think we could and
should accept the patch in its current form.

I'm leaving it on PW a little more, in case there are strong, different
opinions.

Cheers,

Paolo

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

[Leon Romanovsky]

On Thu, May 11, 2023 at 08:36:46AM +0200, Paolo Abeni wrote:
> On Wed, 2023-05-10 at 19:05 -0700, Jakub Kicinski wrote:
> > On Tue, 9 May 2023 16:56:13 +0300 Leon Romanovsky wrote:
> > > > > 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.  
> > > > 
> > > > Is that a new rule?  
> > > 
> > > No, this rule always existed, just some of the maintainers didn't care
> > > about it.
> > > 
> > > > 
> > > > Honestly I think it's important to mention changes applied to
> > > > someone else's patch, if only to let it be known who's to blame
> > > > for any mistakes.  
> > > 
> > > Right, this is why maintainers use this notation when they apply
> > > patches. In your case, you are submitter, patch is not applied yet
> > > and all changes can be easily seen through lore web interface.
> > > 
> > > > 
> > > > I'm seeing plenty of recent precedent in the git history where
> > > > non-committers fixed up patches and made their changes known in
> > > > this way, e.g.:  
> > > 
> > > It doesn't make it correct.
> > > Documentation/maintainer/modifying-patches.rst
> > 
> > TBH I'm not sure if this is the correct reading of this doc.
> > I don't see any problem with Lukas using the common notation.
> > It makes it quite obvious what he changed and the changes are
> > not invasive enough to warrant a major rewrite of the commit msg.
> 
> My reading of such documentation is that (sub-)maintainers could be
> (more frequently) called to this kind of editing, but such editing is
> not restricted.
> 
> In this specific case I could not find quickly via lore references to
> the originating patch.

And this is mainly the issue here. Lukas changes are not different from
what many of us doing when we submit internal patches. We change/update/rewrite
patches which make them different from internal variant.

Once the patches are public, they will have relevant changelog section.

I don't see how modifying-patches.rst can be seen differently.

BTW, Regarding know-to-blame reasoning, everyone who added his
Signed-off-by to the patch is immediately suspicious.

Thanks

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

[Jakub Kicinski]

On Thu, 11 May 2023 09:59:17 +0300 Leon Romanovsky wrote:
> And this is mainly the issue here. Lukas changes are not different from
> what many of us doing when we submit internal patches. We change/update/rewrite
> patches which make them different from internal variant.
> 
> Once the patches are public, they will have relevant changelog section.
> 
> I don't see how modifying-patches.rst can be seen differently.
> 
> BTW, Regarding know-to-blame reasoning, everyone who added his
> Signed-off-by to the patch is immediately suspicious.

Right, modifying-patches.rst does not apply to corpo patches.

Maybe the analogy from US law would be helpful to show how I think
about it -- corporation (especially working on its own product)
is one "legal person", Philipp and Lukas are separate "human persons".

IOW patch circulation and attribution within a corporation is naturally
different than between community members.

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

[Leon Romanovsky]

On Thu, May 11, 2023 at 08:51:28AM -0700, Jakub Kicinski wrote:
> On Thu, 11 May 2023 09:59:17 +0300 Leon Romanovsky wrote:
> > And this is mainly the issue here. Lukas changes are not different from
> > what many of us doing when we submit internal patches. We change/update/rewrite
> > patches which make them different from internal variant.
> > 
> > Once the patches are public, they will have relevant changelog section.
> > 
> > I don't see how modifying-patches.rst can be seen differently.
> > 
> > BTW, Regarding know-to-blame reasoning, everyone who added his
> > Signed-off-by to the patch is immediately suspicious.
> 
> Right, modifying-patches.rst does not apply to corpo patches.
> 
> Maybe the analogy from US law would be helpful to show how I think
> about it -- corporation (especially working on its own product)
> is one "legal person", Philipp and Lukas are separate "human persons".

You have no reliable way to know the relation between person A and person B.

> 
> IOW patch circulation and attribution within a corporation is naturally
> different than between community members.

Yes and no, it is very dependant on corporation.

Thanks

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

[Piotr Raczynski]

On Tue, May 09, 2023 at 06:28:56AM +0200, Lukas Wunner wrote:
> From: Philipp Rosenberger <p.rosenberger@kunbus.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@kunbus.com>
> Signed-off-by: Zhi Han <hanzhi09@gmail.com>
> [lukas: rewrite commit message, linewrap request_threaded_irq() call]
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Other than commit message commented by Leon, LGTM.
Reviewed-by: Piotr Raczynski <piotr.raczynski@intel.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
> 
> 

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 9 May 2023 06:28:56 +0200 you wrote:
> From: Philipp Rosenberger <p.rosenberger@kunbus.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.
> 
> [...]

Here is the summary with links:
  - [net-next] net: enc28j60: Use threaded interrupt instead of workqueue
    https://git.kernel.org/netdev/net-next/c/995585ecdf42

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html