| Message ID | 20250212163659.2287292-1-wintera@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0d0b752f2497471ddd2b32143d167d42e18a8f3c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] s390/qeth: move netif_napi_add_tx() and napi_enable() from under BH | expand |
On Wed, Feb 12, 2025 at 05:36:59PM +0100, Alexandra Winter wrote: > Like other drivers qeth is calling local_bh_enable() after napi_schedule() > to kick-start softirqs [0]. > Since netif_napi_add_tx() and napi_enable() now take the netdev_lock() > mutex [1], move them out from under the BH protection. Same solution as in > commit a60558644e20 ("wifi: mt76: move napi_enable() from under BH") > > Fixes: 1b23cdbd2bbc ("net: protect netdev->napi_list with netdev_lock()") Hm, I wonder if the fixes should be for commit 413f0271f396 ("net: protect NAPI enablement with netdev_lock()") instead ? > Link: https://lore.kernel.org/netdev/20240612181900.4d9d18d0@kernel.org/ [0] > Link: https://lore.kernel.org/netdev/20250115035319.559603-1-kuba@kernel.org/ [1] > Signed-off-by: Alexandra Winter <wintera@linux.ibm.com> > --- > drivers/s390/net/qeth_core_main.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) Other than the above, I briefly scanned the driver source and the change seems reasonable. I am not sure whether a different Fixes is needed or not (I'll leave that to the maintainers to decide), but whether this is fine as is or is re-posted with a new Fixes tag: Acked-by: Joe Damato <jdamato@fastly.com>
On 12.02.25 20:35, Joe Damato wrote: > On Wed, Feb 12, 2025 at 05:36:59PM +0100, Alexandra Winter wrote: >> Like other drivers qeth is calling local_bh_enable() after napi_schedule() >> to kick-start softirqs [0]. >> Since netif_napi_add_tx() and napi_enable() now take the netdev_lock() >> mutex [1], move them out from under the BH protection. Same solution as in >> commit a60558644e20 ("wifi: mt76: move napi_enable() from under BH") >> >> Fixes: 1b23cdbd2bbc ("net: protect netdev->napi_list with netdev_lock()") > Hm, I wonder if the fixes should be for commit 413f0271f396 ("net: > protect NAPI enablement with netdev_lock()") instead ? I was wondering about that too. netif_napi_add_tx() got the lock in 1b23cdbd2bbc and napi_enable() got the lock in 413f0271f396. I don't think I can have 2 Fixes tags, can I? (And I don't think it makes sense to split these few lines up into 2 commits) I chose 1b23cdbd2bbc because it is the earlier one.
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 12 Feb 2025 17:36:59 +0100 you wrote: > Like other drivers qeth is calling local_bh_enable() after napi_schedule() > to kick-start softirqs [0]. > Since netif_napi_add_tx() and napi_enable() now take the netdev_lock() > mutex [1], move them out from under the BH protection. Same solution as in > commit a60558644e20 ("wifi: mt76: move napi_enable() from under BH") > > Fixes: 1b23cdbd2bbc ("net: protect netdev->napi_list with netdev_lock()") > Link: https://lore.kernel.org/netdev/20240612181900.4d9d18d0@kernel.org/ [0] > Link: https://lore.kernel.org/netdev/20250115035319.559603-1-kuba@kernel.org/ [1] > Signed-off-by: Alexandra Winter <wintera@linux.ibm.com> > > [...] Here is the summary with links: - [net] s390/qeth: move netif_napi_add_tx() and napi_enable() from under BH https://git.kernel.org/netdev/net/c/0d0b752f2497 You are awesome, thank you!
diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c index a3adaec5504e..20328d695ef9 100644 --- a/drivers/s390/net/qeth_core_main.c +++ b/drivers/s390/net/qeth_core_main.c @@ -7050,14 +7050,16 @@ int qeth_open(struct net_device *dev) card->data.state = CH_STATE_UP; netif_tx_start_all_queues(dev); - local_bh_disable(); qeth_for_each_output_queue(card, queue, i) { netif_napi_add_tx(dev, &queue->napi, qeth_tx_poll); napi_enable(&queue->napi); - napi_schedule(&queue->napi); } - napi_enable(&card->napi); + + local_bh_disable(); + qeth_for_each_output_queue(card, queue, i) { + napi_schedule(&queue->napi); + } napi_schedule(&card->napi); /* kick-start the NAPI softirq: */ local_bh_enable();
Like other drivers qeth is calling local_bh_enable() after napi_schedule() to kick-start softirqs [0]. Since netif_napi_add_tx() and napi_enable() now take the netdev_lock() mutex [1], move them out from under the BH protection. Same solution as in commit a60558644e20 ("wifi: mt76: move napi_enable() from under BH") Fixes: 1b23cdbd2bbc ("net: protect netdev->napi_list with netdev_lock()") Link: https://lore.kernel.org/netdev/20240612181900.4d9d18d0@kernel.org/ [0] Link: https://lore.kernel.org/netdev/20250115035319.559603-1-kuba@kernel.org/ [1] Signed-off-by: Alexandra Winter <wintera@linux.ibm.com> --- drivers/s390/net/qeth_core_main.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)