| Message ID | 342380d989ce26bc49f0e5d45fbb0416a5f7809f.1683606193.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 995585ecdf42c14fbd4d9d12ca73bf54358581c6 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: enc28j60: Use threaded interrupt instead of workqueue | expand |
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 > >
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 > >
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
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
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.
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
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
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.
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
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!
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",