Message ID | 20201112185949.11315-1-TheSven73@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [net,v1] lan743x: fix issue causing intermittent kernel log warnings | expand |
On Thu, 12 Nov 2020 13:59:49 -0500 Sven Van Asbroeck wrote: > From: Sven Van Asbroeck <thesven73@gmail.com> > > When running this chip on arm imx6, we intermittently observe > the following kernel warning in the log, especially when the > system is under high load: > The driver is calling dev_kfree_skb() from code inside a spinlock, > where h/w interrupts are disabled. This is forbidden, as documented > in include/linux/netdevice.h. The correct function to use > dev_kfree_skb_irq(), or dev_kfree_skb_any(). > > Fix by using the correct dev_kfree_skb_xxx() functions: > > in lan743x_tx_release_desc(): > called by lan743x_tx_release_completed_descriptors() > called by in lan743x_tx_napi_poll() > which holds a spinlock > called by lan743x_tx_release_all_descriptors() > called by lan743x_tx_close() > which can-sleep > conclusion: use dev_kfree_skb_any() > > in lan743x_tx_xmit_frame(): > which holds a spinlock > conclusion: use dev_kfree_skb_irq() > > in lan743x_tx_close(): > which can-sleep > conclusion: use dev_kfree_skb() > > in lan743x_rx_release_ring_element(): > called by lan743x_rx_close() > which can-sleep > called by lan743x_rx_open() > which can-sleep > conclusion: use dev_kfree_skb() > > Fixes: 23f0703c125b ("lan743x: Add main source files for new lan743x driver") > Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com> Applied, thanks. The _irq() cases look a little strange, are you planning a refactor in net-next? Seems like the freeing can be moved outside the lock. Also the driver could stop the queue when there is less than MAX_SKB_FRAGS + 2 descriptors left, so it doesn't need the "overflow_skb" thing.
On Sat, Nov 14, 2020 at 6:19 PM Jakub Kicinski <kuba@kernel.org> wrote: > > The _irq() cases look a little strange, are you planning a refactor in > net-next? I'd like to, but I don't understand skbs/queues well enough (yet).
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c index 9de970ec2056..a9fda2e6e715 100644 --- a/drivers/net/ethernet/microchip/lan743x_main.c +++ b/drivers/net/ethernet/microchip/lan743x_main.c @@ -1291,13 +1291,13 @@ static void lan743x_tx_release_desc(struct lan743x_tx *tx, goto clear_active; if (!(buffer_info->flags & TX_BUFFER_INFO_FLAG_TIMESTAMP_REQUESTED)) { - dev_kfree_skb(buffer_info->skb); + dev_kfree_skb_any(buffer_info->skb); goto clear_skb; } if (cleanup) { lan743x_ptp_unrequest_tx_timestamp(tx->adapter); - dev_kfree_skb(buffer_info->skb); + dev_kfree_skb_any(buffer_info->skb); } else { ignore_sync = (buffer_info->flags & TX_BUFFER_INFO_FLAG_IGNORE_SYNC) != 0; @@ -1607,7 +1607,7 @@ static netdev_tx_t lan743x_tx_xmit_frame(struct lan743x_tx *tx, if (required_number_of_descriptors > lan743x_tx_get_avail_desc(tx)) { if (required_number_of_descriptors > (tx->ring_size - 1)) { - dev_kfree_skb(skb); + dev_kfree_skb_irq(skb); } else { /* save to overflow buffer */ tx->overflow_skb = skb; @@ -1640,7 +1640,7 @@ static netdev_tx_t lan743x_tx_xmit_frame(struct lan743x_tx *tx, start_frame_length, do_timestamp, skb->ip_summed == CHECKSUM_PARTIAL)) { - dev_kfree_skb(skb); + dev_kfree_skb_irq(skb); goto unlock; } @@ -1659,7 +1659,7 @@ static netdev_tx_t lan743x_tx_xmit_frame(struct lan743x_tx *tx, * frame assembler clean up was performed inside * lan743x_tx_frame_add_fragment */ - dev_kfree_skb(skb); + dev_kfree_skb_irq(skb); goto unlock; } }