[PATCH net v2 1/2] net: ks8851: Inline ks8851_rx_skb()

[PATCH net v2 1/2] net: ks8851: Inline ks8851_rx_skb()

[Marek Vasut]

Both ks8851_rx_skb_par() and ks8851_rx_skb_spi() call netif_rx(skb),
inline the netif_rx(skb) call directly into ks8851_common.c and drop
the .rx_skb callback and ks8851_rx_skb() wrapper. This removes one
indirect call from the driver, no functional change otherwise.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Ratheesh Kannoth <rkannoth@marvell.com>
Cc: Ronald Wahl <ronald.wahl@raritan.com>
Cc: Simon Horman <horms@kernel.org>
Cc: netdev@vger.kernel.org
---
V2: - Fill in 'net' subject prefix
---
 drivers/net/ethernet/micrel/ks8851.h        |  3 ---
 drivers/net/ethernet/micrel/ks8851_common.c | 12 +-----------
 drivers/net/ethernet/micrel/ks8851_par.c    | 11 -----------
 drivers/net/ethernet/micrel/ks8851_spi.c    | 11 -----------
 4 files changed, 1 insertion(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.h b/drivers/net/ethernet/micrel/ks8851.h
index e5ec0a363aff8..31f75b4a67fd7 100644
--- a/drivers/net/ethernet/micrel/ks8851.h
+++ b/drivers/net/ethernet/micrel/ks8851.h
@@ -368,7 +368,6 @@ union ks8851_tx_hdr {
  * @rdfifo: FIFO read callback
  * @wrfifo: FIFO write callback
  * @start_xmit: start_xmit() implementation callback
- * @rx_skb: rx_skb() implementation callback
  * @flush_tx_work: flush_tx_work() implementation callback
  *
  * The @statelock is used to protect information in the structure which may
@@ -423,8 +422,6 @@ struct ks8851_net {
 					  struct sk_buff *txp, bool irq);
 	netdev_tx_t		(*start_xmit)(struct sk_buff *skb,
 					      struct net_device *dev);
-	void			(*rx_skb)(struct ks8851_net *ks,
-					  struct sk_buff *skb);
 	void			(*flush_tx_work)(struct ks8851_net *ks);
 };
 
diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c
index 0bf13b38b8f5b..896d43bb8883d 100644
--- a/drivers/net/ethernet/micrel/ks8851_common.c
+++ b/drivers/net/ethernet/micrel/ks8851_common.c
@@ -231,16 +231,6 @@ static void ks8851_dbg_dumpkkt(struct ks8851_net *ks, u8 *rxpkt)
 		   rxpkt[12], rxpkt[13], rxpkt[14], rxpkt[15]);
 }
 
-/**
- * ks8851_rx_skb - receive skbuff
- * @ks: The device state.
- * @skb: The skbuff
- */
-static void ks8851_rx_skb(struct ks8851_net *ks, struct sk_buff *skb)
-{
-	ks->rx_skb(ks, skb);
-}
-
 /**
  * ks8851_rx_pkts - receive packets from the host
  * @ks: The device information.
@@ -309,7 +299,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 					ks8851_dbg_dumpkkt(ks, rxpkt);
 
 				skb->protocol = eth_type_trans(skb, ks->netdev);
-				ks8851_rx_skb(ks, skb);
+				netif_rx(skb);
 
 				ks->netdev->stats.rx_packets++;
 				ks->netdev->stats.rx_bytes += rxlen;
diff --git a/drivers/net/ethernet/micrel/ks8851_par.c b/drivers/net/ethernet/micrel/ks8851_par.c
index 2a7f298542670..381b9cd285ebd 100644
--- a/drivers/net/ethernet/micrel/ks8851_par.c
+++ b/drivers/net/ethernet/micrel/ks8851_par.c
@@ -210,16 +210,6 @@ static void ks8851_wrfifo_par(struct ks8851_net *ks, struct sk_buff *txp,
 	iowrite16_rep(ksp->hw_addr, txp->data, len / 2);
 }
 
-/**
- * ks8851_rx_skb_par - receive skbuff
- * @ks: The device state.
- * @skb: The skbuff
- */
-static void ks8851_rx_skb_par(struct ks8851_net *ks, struct sk_buff *skb)
-{
-	netif_rx(skb);
-}
-
 static unsigned int ks8851_rdreg16_par_txqcr(struct ks8851_net *ks)
 {
 	return ks8851_rdreg16_par(ks, KS_TXQCR);
@@ -298,7 +288,6 @@ static int ks8851_probe_par(struct platform_device *pdev)
 	ks->rdfifo = ks8851_rdfifo_par;
 	ks->wrfifo = ks8851_wrfifo_par;
 	ks->start_xmit = ks8851_start_xmit_par;
-	ks->rx_skb = ks8851_rx_skb_par;
 
 #define STD_IRQ (IRQ_LCI |	/* Link Change */	\
 		 IRQ_RXI |	/* RX done */		\
diff --git a/drivers/net/ethernet/micrel/ks8851_spi.c b/drivers/net/ethernet/micrel/ks8851_spi.c
index 2f803377c9f9d..670c1de966db8 100644
--- a/drivers/net/ethernet/micrel/ks8851_spi.c
+++ b/drivers/net/ethernet/micrel/ks8851_spi.c
@@ -298,16 +298,6 @@ static unsigned int calc_txlen(unsigned int len)
 	return ALIGN(len + 4, 4);
 }
 
-/**
- * ks8851_rx_skb_spi - receive skbuff
- * @ks: The device state
- * @skb: The skbuff
- */
-static void ks8851_rx_skb_spi(struct ks8851_net *ks, struct sk_buff *skb)
-{
-	netif_rx(skb);
-}
-
 /**
  * ks8851_tx_work - process tx packet(s)
  * @work: The work strucutre what was scheduled.
@@ -435,7 +425,6 @@ static int ks8851_probe_spi(struct spi_device *spi)
 	ks->rdfifo = ks8851_rdfifo_spi;
 	ks->wrfifo = ks8851_wrfifo_spi;
 	ks->start_xmit = ks8851_start_xmit_spi;
-	ks->rx_skb = ks8851_rx_skb_spi;
 	ks->flush_tx_work = ks8851_flush_tx_work_spi;
 
 #define STD_IRQ (IRQ_LCI |	/* Link Change */	\
-- 
2.43.0

[PATCH net v2 2/2] net: ks8851: Handle softirqs at the end of IRQ thread to fix hang

[Marek Vasut]

The ks8851_irq() thread may call ks8851_rx_pkts() in case there are
any packets in the MAC FIFO, which calls netif_rx(). This netif_rx()
implementation is guarded by local_bh_disable() and local_bh_enable().
The local_bh_enable() may call do_softirq() to run softirqs in case
any are pending. One of the softirqs is net_rx_action, which ultimately
reaches the driver .start_xmit callback. If that happens, the system
hangs. The entire call chain is below:

ks8851_start_xmit_par from netdev_start_xmit
netdev_start_xmit from dev_hard_start_xmit
dev_hard_start_xmit from sch_direct_xmit
sch_direct_xmit from __dev_queue_xmit
__dev_queue_xmit from __neigh_update
__neigh_update from neigh_update
neigh_update from arp_process.constprop.0
arp_process.constprop.0 from __netif_receive_skb_one_core
__netif_receive_skb_one_core from process_backlog
process_backlog from __napi_poll.constprop.0
__napi_poll.constprop.0 from net_rx_action
net_rx_action from __do_softirq
__do_softirq from call_with_stack
call_with_stack from do_softirq
do_softirq from __local_bh_enable_ip
__local_bh_enable_ip from netif_rx
netif_rx from ks8851_irq
ks8851_irq from irq_thread_fn
irq_thread_fn from irq_thread
irq_thread from kthread
kthread from ret_from_fork

The hang happens because ks8851_irq() first locks a spinlock in
ks8851_par.c ks8851_lock_par() spin_lock_irqsave(&ksp->lock, ...)
and with that spinlock locked, calls netif_rx(). Once the execution
reaches ks8851_start_xmit_par(), it calls ks8851_lock_par() again
which attempts to claim the already locked spinlock again, and the
hang happens.

Move the do_softirq() call outside of the spinlock protected section
of ks8851_irq() by disabling BHs around the entire spinlock protected
section of ks8851_irq() handler. Place local_bh_enable() outside of
the spinlock protected section, so that it can trigger do_softirq()
without the ks8851_par.c ks8851_lock_par() spinlock being held, and
safely call ks8851_start_xmit_par() without attempting to lock the
already locked spinlock.

Since ks8851_irq() is protected by local_bh_disable()/local_bh_enable()
now, replace netif_rx() with __netif_rx() which is not duplicating the
local_bh_disable()/local_bh_enable() calls.

Fixes: 797047f875b5 ("net: ks8851: Implement Parallel bus operations")
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Ratheesh Kannoth <rkannoth@marvell.com>
Cc: Ronald Wahl <ronald.wahl@raritan.com>
Cc: Simon Horman <horms@kernel.org>
Cc: netdev@vger.kernel.org
---
V2: - Drop the need_bh_off test, the test was always true
    - Fill in 'net' subject prefix
---
 drivers/net/ethernet/micrel/ks8851_common.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c
index 896d43bb8883d..d4cdf3d4f5525 100644
--- a/drivers/net/ethernet/micrel/ks8851_common.c
+++ b/drivers/net/ethernet/micrel/ks8851_common.c
@@ -299,7 +299,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 					ks8851_dbg_dumpkkt(ks, rxpkt);
 
 				skb->protocol = eth_type_trans(skb, ks->netdev);
-				netif_rx(skb);
+				__netif_rx(skb);
 
 				ks->netdev->stats.rx_packets++;
 				ks->netdev->stats.rx_bytes += rxlen;
@@ -330,6 +330,8 @@ static irqreturn_t ks8851_irq(int irq, void *_ks)
 	unsigned long flags;
 	unsigned int status;
 
+	local_bh_disable();
+
 	ks8851_lock(ks, &flags);
 
 	status = ks8851_rdreg16(ks, KS_ISR);
@@ -406,6 +408,8 @@ static irqreturn_t ks8851_irq(int irq, void *_ks)
 	if (status & IRQ_LCI)
 		mii_check_link(&ks->mii);
 
+	local_bh_enable();
+
 	return IRQ_HANDLED;
 }
 
-- 
2.43.0

Re: [PATCH net v2 2/2] net: ks8851: Handle softirqs at the end of IRQ thread to fix hang

[Andy Shevchenko]

On Fri, Apr 05, 2024 at 10:30:40PM +0200, Marek Vasut wrote:
> The ks8851_irq() thread may call ks8851_rx_pkts() in case there are
> any packets in the MAC FIFO, which calls netif_rx(). This netif_rx()
> implementation is guarded by local_bh_disable() and local_bh_enable().
> The local_bh_enable() may call do_softirq() to run softirqs in case
> any are pending. One of the softirqs is net_rx_action, which ultimately
> reaches the driver .start_xmit callback. If that happens, the system
> hangs. The entire call chain is below:
> 
> ks8851_start_xmit_par from netdev_start_xmit
> netdev_start_xmit from dev_hard_start_xmit
> dev_hard_start_xmit from sch_direct_xmit
> sch_direct_xmit from __dev_queue_xmit
> __dev_queue_xmit from __neigh_update
> __neigh_update from neigh_update
> neigh_update from arp_process.constprop.0
> arp_process.constprop.0 from __netif_receive_skb_one_core
> __netif_receive_skb_one_core from process_backlog
> process_backlog from __napi_poll.constprop.0
> __napi_poll.constprop.0 from net_rx_action
> net_rx_action from __do_softirq
> __do_softirq from call_with_stack
> call_with_stack from do_softirq
> do_softirq from __local_bh_enable_ip
> __local_bh_enable_ip from netif_rx
> netif_rx from ks8851_irq
> ks8851_irq from irq_thread_fn

> irq_thread_fn from irq_thread
> irq_thread from kthread
> kthread from ret_from_fork

These lines are unneeded (in case you need a new version, you can drop them).

> The hang happens because ks8851_irq() first locks a spinlock in
> ks8851_par.c ks8851_lock_par() spin_lock_irqsave(&ksp->lock, ...)
> and with that spinlock locked, calls netif_rx(). Once the execution
> reaches ks8851_start_xmit_par(), it calls ks8851_lock_par() again
> which attempts to claim the already locked spinlock again, and the
> hang happens.
> 
> Move the do_softirq() call outside of the spinlock protected section
> of ks8851_irq() by disabling BHs around the entire spinlock protected
> section of ks8851_irq() handler. Place local_bh_enable() outside of
> the spinlock protected section, so that it can trigger do_softirq()
> without the ks8851_par.c ks8851_lock_par() spinlock being held, and
> safely call ks8851_start_xmit_par() without attempting to lock the
> already locked spinlock.
> 
> Since ks8851_irq() is protected by local_bh_disable()/local_bh_enable()
> now, replace netif_rx() with __netif_rx() which is not duplicating the
> local_bh_disable()/local_bh_enable() calls.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net v2 1/2] net: ks8851: Inline ks8851_rx_skb()

[patchwork-bot+netdevbpf]

Hello:

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

On Fri,  5 Apr 2024 22:30:39 +0200 you wrote:
> Both ks8851_rx_skb_par() and ks8851_rx_skb_spi() call netif_rx(skb),
> inline the netif_rx(skb) call directly into ks8851_common.c and drop
> the .rx_skb callback and ks8851_rx_skb() wrapper. This removes one
> indirect call from the driver, no functional change otherwise.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> [...]

Here is the summary with links:
  - [net,v2,1/2] net: ks8851: Inline ks8851_rx_skb()
    https://git.kernel.org/netdev/net/c/f96f700449b6
  - [net,v2,2/2] net: ks8851: Handle softirqs at the end of IRQ thread to fix hang
    https://git.kernel.org/netdev/net/c/be0384bf599c

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

Re: [PATCH net v2 2/2] net: ks8851: Handle softirqs at the end of IRQ thread to fix hang

[Marek Vasut]

On 4/8/24 4:52 PM, Andy Shevchenko wrote:
> On Fri, Apr 05, 2024 at 10:30:40PM +0200, Marek Vasut wrote:
>> The ks8851_irq() thread may call ks8851_rx_pkts() in case there are
>> any packets in the MAC FIFO, which calls netif_rx(). This netif_rx()
>> implementation is guarded by local_bh_disable() and local_bh_enable().
>> The local_bh_enable() may call do_softirq() to run softirqs in case
>> any are pending. One of the softirqs is net_rx_action, which ultimately
>> reaches the driver .start_xmit callback. If that happens, the system
>> hangs. The entire call chain is below:
>>
>> ks8851_start_xmit_par from netdev_start_xmit
>> netdev_start_xmit from dev_hard_start_xmit
>> dev_hard_start_xmit from sch_direct_xmit
>> sch_direct_xmit from __dev_queue_xmit
>> __dev_queue_xmit from __neigh_update
>> __neigh_update from neigh_update
>> neigh_update from arp_process.constprop.0
>> arp_process.constprop.0 from __netif_receive_skb_one_core
>> __netif_receive_skb_one_core from process_backlog
>> process_backlog from __napi_poll.constprop.0
>> __napi_poll.constprop.0 from net_rx_action
>> net_rx_action from __do_softirq
>> __do_softirq from call_with_stack
>> call_with_stack from do_softirq
>> do_softirq from __local_bh_enable_ip
>> __local_bh_enable_ip from netif_rx
>> netif_rx from ks8851_irq
>> ks8851_irq from irq_thread_fn
> 
>> irq_thread_fn from irq_thread
>> irq_thread from kthread
>> kthread from ret_from_fork
> 
> These lines are unneeded (in case you need a new version, you can drop them).

I just got back and going through a mountain of email, I see Jakub 
already picked the V2, so, noted for next time. Thank you !

Re: [PATCH net v2 2/2] net: ks8851: Handle softirqs at the end of IRQ thread to fix hang

[Jakub Kicinski]

On Fri, 12 Apr 2024 13:29:04 +0200 Marek Vasut wrote:
> >> irq_thread_fn from irq_thread
> >> irq_thread from kthread
> >> kthread from ret_from_fork  
> > 
> > These lines are unneeded (in case you need a new version, you can drop them).  
> 
> I just got back and going through a mountain of email, I see Jakub 
> already picked the V2, so, noted for next time. Thank you !

Whether the stack trace is for a hard IRQ or threaded IRQ is the first
thing I looked for when reviewing. Change is about the calling context. 
So I figured while not strictly necessary, in this particular case,
these lines may be helpful for people eyeballing the change...
In general, yes, trimming the bottom of the stack is good hygiene.

Re: [PATCH net v2 2/2] net: ks8851: Handle softirqs at the end of IRQ thread to fix hang

[Jakub Kicinski]

On Fri, 12 Apr 2024 07:52:20 -0700 Jakub Kicinski wrote:
> In general, yes, trimming the bottom of the stack is good hygiene.

That sentence puts the words "bottom" and "hygiene" uncomfortably close
together. Oh, well, it's Friday..

Re: [PATCH net v2 2/2] net: ks8851: Handle softirqs at the end of IRQ thread to fix hang

[Andy Shevchenko]

On Fri, Apr 12, 2024 at 07:54:14AM -0700, Jakub Kicinski wrote:
> On Fri, 12 Apr 2024 07:52:20 -0700 Jakub Kicinski wrote:
> > In general, yes, trimming the bottom of the stack is good hygiene.
> 
> That sentence puts the words "bottom" and "hygiene" uncomfortably close
> together. Oh, well, it's Friday..

Oh, nice! Have a good one, and take your hygiene seriously even on Fridays!

-- 
With Best Regards,
Andy Shevchenko