| Message ID | 20250206225638.1387810-3-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3e7efc3f4f03bca0ea630c302e7c79cf807476bb |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: improve core queue API handling while device is down | expand |
On Thu, Feb 6, 2025 at 2:56 PM Jakub Kicinski <kuba@kernel.org> wrote: > > We seem to be missing a netif_running() check from the devmem > installation path. Starting a queue on a stopped device makes > no sense. We still want to be able to allocate the memory, just > to test that the device is indeed setting up the page pools > in a memory provider compatible way. > Gah, I think I made a suggestion on v1 that would have broken this check. We do indeed need to make the driver calls, if only for the check that the driver the memory provider enabled pool. > This is not a bug fix, because existing drivers check if > the interface is down as part of the ops. But new drivers > shouldn't have to do this, as long as they can correctly > alloc/free while down. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> Reviewed-by: Mina Almasry <almasrymina@google.com>
diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h index b02bb9f109d5..73d3401261a6 100644 --- a/include/net/netdev_queues.h +++ b/include/net/netdev_queues.h @@ -117,6 +117,10 @@ struct netdev_stat_ops { * * @ndo_queue_stop: Stop the RX queue at the specified index. The stopped * queue's memory is written at the specified address. + * + * Note that @ndo_queue_mem_alloc and @ndo_queue_mem_free may be called while + * the interface is closed. @ndo_queue_start and @ndo_queue_stop will only + * be called for an interface which is open. */ struct netdev_queue_mgmt_ops { size_t ndo_queue_mem_size; diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c index a5813d50e058..5352e0c1f37e 100644 --- a/net/core/netdev_rx_queue.c +++ b/net/core/netdev_rx_queue.c @@ -37,13 +37,17 @@ int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx) if (err) goto err_free_new_queue_mem; - err = qops->ndo_queue_stop(dev, old_mem, rxq_idx); - if (err) - goto err_free_new_queue_mem; + if (netif_running(dev)) { + err = qops->ndo_queue_stop(dev, old_mem, rxq_idx); + if (err) + goto err_free_new_queue_mem; - err = qops->ndo_queue_start(dev, new_mem, rxq_idx); - if (err) - goto err_start_queue; + err = qops->ndo_queue_start(dev, new_mem, rxq_idx); + if (err) + goto err_start_queue; + } else { + swap(new_mem, old_mem); + } qops->ndo_queue_mem_free(dev, old_mem);
We seem to be missing a netif_running() check from the devmem installation path. Starting a queue on a stopped device makes no sense. We still want to be able to allocate the memory, just to test that the device is indeed setting up the page pools in a memory provider compatible way. This is not a bug fix, because existing drivers check if the interface is down as part of the ops. But new drivers shouldn't have to do this, as long as they can correctly alloc/free while down. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- include/net/netdev_queues.h | 4 ++++ net/core/netdev_rx_queue.c | 16 ++++++++++------ 2 files changed, 14 insertions(+), 6 deletions(-)