* [net,PATCH v2] net: ks8851: Queue RX packets in IRQ handler instead of disabling BHs
@ 2024-04-30 19:43 Marek Vasut
2024-05-01 2:10 ` Jakub Kicinski
0 siblings, 1 reply; 4+ messages in thread
From: Marek Vasut @ 2024-04-30 19:43 UTC (permalink / raw
To: netdev
Cc: Marek Vasut, Ronald Wahl, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman
Currently the driver uses local_bh_disable()/local_bh_enable() in its
IRQ handler to avoid triggering net_rx_action() softirq on exit from
netif_rx(). The net_rx_action() could trigger this driver .start_xmit
callback, which is protected by the same lock as the IRQ handler, so
calling the .start_xmit from netif_rx() from the IRQ handler critical
section protected by the lock could lead to an attempt to claim the
already claimed lock, and a hang.
The local_bh_disable()/local_bh_enable() approach works only in case
the IRQ handler is protected by a spinlock, but does not work if the
IRQ handler is protected by mutex, i.e. this works for KS8851 with
Parallel bus interface, but not for KS8851 with SPI bus interface.
Remove the BH manipulation and instead of calling netif_rx() inside
the IRQ handler code protected by the lock, queue all the received
SKBs in the IRQ handler into a queue first, and once the IRQ handler
exits the critical section protected by the lock, dequeue all the
queued SKBs and push them all into netif_rx(). At this point, it is
safe to trigger the net_rx_action() softirq, since the netif_rx()
call is outside of the lock that protects the IRQ handler.
Fixes: be0384bf599c ("net: ks8851: Handle softirqs at the end of IRQ thread to fix hang")
Tested-by: Ronald Wahl <ronald.wahl@raritan.com> # KS8851 SPI
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Ronald Wahl <ronald.wahl@raritan.com>
Cc: Simon Horman <horms@kernel.org>
Cc: netdev@vger.kernel.org
---
V2: - Add TB from Ronald
- Operate private skb queue without locking as suggested by Eric
---
Note: This is basically what Jakub originally suggested in
https://patchwork.kernel.org/project/netdevbpf/patch/20240331142353.93792-2-marex@denx.de/#25785606
---
drivers/net/ethernet/micrel/ks8851.h | 1 +
drivers/net/ethernet/micrel/ks8851_common.c | 8 ++++----
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/micrel/ks8851.h b/drivers/net/ethernet/micrel/ks8851.h
index 31f75b4a67fd7..f311074ea13bc 100644
--- a/drivers/net/ethernet/micrel/ks8851.h
+++ b/drivers/net/ethernet/micrel/ks8851.h
@@ -399,6 +399,7 @@ struct ks8851_net {
struct work_struct rxctrl_work;
+ struct sk_buff_head rxq;
struct sk_buff_head txq;
unsigned int queued_len;
diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c
index d4cdf3d4f5525..813a41193e3ce 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);
+ __skb_queue_tail(&ks->rxq, skb);
ks->netdev->stats.rx_packets++;
ks->netdev->stats.rx_bytes += rxlen;
@@ -330,8 +330,6 @@ 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);
@@ -408,7 +406,8 @@ static irqreturn_t ks8851_irq(int irq, void *_ks)
if (status & IRQ_LCI)
mii_check_link(&ks->mii);
- local_bh_enable();
+ while (!skb_queue_empty(&ks->rxq))
+ netif_rx(__skb_dequeue(&ks->rxq));
return IRQ_HANDLED;
}
@@ -1189,6 +1188,7 @@ int ks8851_probe_common(struct net_device *netdev, struct device *dev,
NETIF_MSG_PROBE |
NETIF_MSG_LINK);
+ __skb_queue_head_init(&ks->rxq);
skb_queue_head_init(&ks->txq);
netdev->ethtool_ops = &ks8851_ethtool_ops;
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [net,PATCH v2] net: ks8851: Queue RX packets in IRQ handler instead of disabling BHs
2024-04-30 19:43 [net,PATCH v2] net: ks8851: Queue RX packets in IRQ handler instead of disabling BHs Marek Vasut
@ 2024-05-01 2:10 ` Jakub Kicinski
2024-05-01 2:39 ` Eric Dumazet
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Kicinski @ 2024-05-01 2:10 UTC (permalink / raw
To: Marek Vasut
Cc: netdev, Ronald Wahl, David S. Miller, Eric Dumazet, Paolo Abeni,
Simon Horman
On Tue, 30 Apr 2024 21:43:34 +0200 Marek Vasut wrote:
> diff --git a/drivers/net/ethernet/micrel/ks8851.h b/drivers/net/ethernet/micrel/ks8851.h
> index 31f75b4a67fd7..f311074ea13bc 100644
> --- a/drivers/net/ethernet/micrel/ks8851.h
> +++ b/drivers/net/ethernet/micrel/ks8851.h
> @@ -399,6 +399,7 @@ struct ks8851_net {
>
> struct work_struct rxctrl_work;
>
> + struct sk_buff_head rxq;
> struct sk_buff_head txq;
> unsigned int queued_len;
One more round, sorry, this structure has a kdoc, please fill in
the description for the new member.
> @@ -408,7 +406,8 @@ static irqreturn_t ks8851_irq(int irq, void *_ks)
> if (status & IRQ_LCI)
> mii_check_link(&ks->mii);
>
> - local_bh_enable();
> + while (!skb_queue_empty(&ks->rxq))
> + netif_rx(__skb_dequeue(&ks->rxq));
Personal preference and probably not worth retesting but FWIW I'd write
this as:
while ((skb = __skb_dequeue(&ks->rxq)))
netif_rx(skb);
--
pw-bot: cr
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [net,PATCH v2] net: ks8851: Queue RX packets in IRQ handler instead of disabling BHs
2024-05-01 2:10 ` Jakub Kicinski
@ 2024-05-01 2:39 ` Eric Dumazet
2024-05-03 0:47 ` Marek Vasut
0 siblings, 1 reply; 4+ messages in thread
From: Eric Dumazet @ 2024-05-01 2:39 UTC (permalink / raw
To: Jakub Kicinski
Cc: Marek Vasut, netdev, Ronald Wahl, David S. Miller, Paolo Abeni,
Simon Horman
On Wed, May 1, 2024 at 4:10 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 30 Apr 2024 21:43:34 +0200 Marek Vasut wrote:
> > diff --git a/drivers/net/ethernet/micrel/ks8851.h b/drivers/net/ethernet/micrel/ks8851.h
> > index 31f75b4a67fd7..f311074ea13bc 100644
> > --- a/drivers/net/ethernet/micrel/ks8851.h
> > +++ b/drivers/net/ethernet/micrel/ks8851.h
> > @@ -399,6 +399,7 @@ struct ks8851_net {
> >
> > struct work_struct rxctrl_work;
> >
> > + struct sk_buff_head rxq;
> > struct sk_buff_head txq;
> > unsigned int queued_len;
>
> One more round, sorry, this structure has a kdoc, please fill in
> the description for the new member.
Alternative is to use a local (automatic variable on the stack) struct
sk_buff_head,
no need for permanent storage in struct ks8851_net
>
>
> > @@ -408,7 +406,8 @@ static irqreturn_t ks8851_irq(int irq, void *_ks)
> > if (status & IRQ_LCI)
> > mii_check_link(&ks->mii);
> >
> > - local_bh_enable();
> > + while (!skb_queue_empty(&ks->rxq))
> > + netif_rx(__skb_dequeue(&ks->rxq));
>
> Personal preference and probably not worth retesting but FWIW I'd write
> this as:
>
> while ((skb = __skb_dequeue(&ks->rxq)))
> netif_rx(skb);
> --
> pw-bot: cr
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [net,PATCH v2] net: ks8851: Queue RX packets in IRQ handler instead of disabling BHs
2024-05-01 2:39 ` Eric Dumazet
@ 2024-05-03 0:47 ` Marek Vasut
0 siblings, 0 replies; 4+ messages in thread
From: Marek Vasut @ 2024-05-03 0:47 UTC (permalink / raw
To: Eric Dumazet, Jakub Kicinski
Cc: netdev, Ronald Wahl, David S. Miller, Paolo Abeni, Simon Horman
On 5/1/24 4:39 AM, Eric Dumazet wrote:
> On Wed, May 1, 2024 at 4:10 AM Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> On Tue, 30 Apr 2024 21:43:34 +0200 Marek Vasut wrote:
>>> diff --git a/drivers/net/ethernet/micrel/ks8851.h b/drivers/net/ethernet/micrel/ks8851.h
>>> index 31f75b4a67fd7..f311074ea13bc 100644
>>> --- a/drivers/net/ethernet/micrel/ks8851.h
>>> +++ b/drivers/net/ethernet/micrel/ks8851.h
>>> @@ -399,6 +399,7 @@ struct ks8851_net {
>>>
>>> struct work_struct rxctrl_work;
>>>
>>> + struct sk_buff_head rxq;
>>> struct sk_buff_head txq;
>>> unsigned int queued_len;
>>
>> One more round, sorry, this structure has a kdoc, please fill in
>> the description for the new member.
>
> Alternative is to use a local (automatic variable on the stack) struct
> sk_buff_head,
> no need for permanent storage in struct ks8851_net
I think that's even better, done in V3 + addressed feedback from Jakub.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-05-03 1:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-30 19:43 [net,PATCH v2] net: ks8851: Queue RX packets in IRQ handler instead of disabling BHs Marek Vasut
2024-05-01 2:10 ` Jakub Kicinski
2024-05-01 2:39 ` Eric Dumazet
2024-05-03 0:47 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.