| Message ID | 20240801205619.987396-1-pkaligineedi@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fba917b169bea5f8f2ee300e19d5f7a6341a5251 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] gve: Fix use of netif_carrier_ok() | expand |
On Thu, Aug 01, 2024 at 01:56:19PM -0700, Praveen Kaligineedi wrote: > GVE driver wrongly relies on netif_carrier_ok() to check the > interface administrative state when resources are being > allocated/deallocated for queue(s). netif_carrier_ok() needs > to be replaced with netif_running() for all such cases. > > Administrative state is the result of "ip link set dev <dev> > up/down". It reflects whether the administrator wants to use > the device for traffic and the corresponding resources have > been allocated. > > Fixes: 5f08cd3d6423 ("gve: Alloc before freeing when adjusting queues") > Signed-off-by: Praveen Kaligineedi <pkaligineedi@google.com> > Reviewed-by: Shailend Chand <shailend@google.com> > Reviewed-by: Willem de Bruijn <willemb@google.com> ... > diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c ... > @@ -1847,7 +1847,7 @@ int gve_adjust_queues(struct gve_priv *priv, > rx_alloc_cfg.qcfg = &new_rx_config; > tx_alloc_cfg.num_rings = new_tx_config.num_queues; > > - if (netif_carrier_ok(priv->dev)) { > + if (netif_running(priv->dev)) { > err = gve_adjust_config(priv, &tx_alloc_cfg, &rx_alloc_cfg); > return err; > } Hi Praveen, Not for this patch, but I am curious to know if this check is needed at all, because gve_adjust_queues only seems to be called from gve_set_channels if netif_running() (previously netif_carrier_ok()) is true ...
> > - if (netif_carrier_ok(priv->dev)) { > > + if (netif_running(priv->dev)) { > > err = gve_adjust_config(priv, &tx_alloc_cfg, &rx_alloc_cfg); > > return err; > > } > > Hi Praveen, > > Not for this patch, but I am curious to know if this check is needed at > all, because gve_adjust_queues only seems to be called from > gve_set_channels if netif_running() (previously netif_carrier_ok()) is true > > ... > Thanks Simon. I am aware of it. I am planning to send a follow-up patch to net-next to remove the redundant check.
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Thu, 1 Aug 2024 13:56:19 -0700 you wrote: > GVE driver wrongly relies on netif_carrier_ok() to check the > interface administrative state when resources are being > allocated/deallocated for queue(s). netif_carrier_ok() needs > to be replaced with netif_running() for all such cases. > > Administrative state is the result of "ip link set dev <dev> > up/down". It reflects whether the administrator wants to use > the device for traffic and the corresponding resources have > been allocated. > > [...] Here is the summary with links: - [net] gve: Fix use of netif_carrier_ok() https://git.kernel.org/netdev/net/c/fba917b169be You are awesome, thank you!
diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c index 3480ff5c7ed6..5a8b490ab3ad 100644 --- a/drivers/net/ethernet/google/gve/gve_ethtool.c +++ b/drivers/net/ethernet/google/gve/gve_ethtool.c @@ -495,7 +495,7 @@ static int gve_set_channels(struct net_device *netdev, return -EINVAL; } - if (!netif_carrier_ok(netdev)) { + if (!netif_running(netdev)) { priv->tx_cfg.num_queues = new_tx; priv->rx_cfg.num_queues = new_rx; return 0; diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c index 9744b426940e..661566db68c8 100644 --- a/drivers/net/ethernet/google/gve/gve_main.c +++ b/drivers/net/ethernet/google/gve/gve_main.c @@ -1566,7 +1566,7 @@ static int gve_set_xdp(struct gve_priv *priv, struct bpf_prog *prog, u32 status; old_prog = READ_ONCE(priv->xdp_prog); - if (!netif_carrier_ok(priv->dev)) { + if (!netif_running(priv->dev)) { WRITE_ONCE(priv->xdp_prog, prog); if (old_prog) bpf_prog_put(old_prog); @@ -1847,7 +1847,7 @@ int gve_adjust_queues(struct gve_priv *priv, rx_alloc_cfg.qcfg = &new_rx_config; tx_alloc_cfg.num_rings = new_tx_config.num_queues; - if (netif_carrier_ok(priv->dev)) { + if (netif_running(priv->dev)) { err = gve_adjust_config(priv, &tx_alloc_cfg, &rx_alloc_cfg); return err; } @@ -2064,7 +2064,7 @@ static int gve_set_features(struct net_device *netdev, if ((netdev->features & NETIF_F_LRO) != (features & NETIF_F_LRO)) { netdev->features ^= NETIF_F_LRO; - if (netif_carrier_ok(netdev)) { + if (netif_running(netdev)) { err = gve_adjust_config(priv, &tx_alloc_cfg, &rx_alloc_cfg); if (err) goto revert_features; @@ -2359,7 +2359,7 @@ static int gve_reset_recovery(struct gve_priv *priv, bool was_up) int gve_reset(struct gve_priv *priv, bool attempt_teardown) { - bool was_up = netif_carrier_ok(priv->dev); + bool was_up = netif_running(priv->dev); int err; dev_info(&priv->pdev->dev, "Performing reset\n"); @@ -2700,7 +2700,7 @@ static void gve_shutdown(struct pci_dev *pdev) { struct net_device *netdev = pci_get_drvdata(pdev); struct gve_priv *priv = netdev_priv(netdev); - bool was_up = netif_carrier_ok(priv->dev); + bool was_up = netif_running(priv->dev); rtnl_lock(); if (was_up && gve_close(priv->dev)) { @@ -2718,7 +2718,7 @@ static int gve_suspend(struct pci_dev *pdev, pm_message_t state) { struct net_device *netdev = pci_get_drvdata(pdev); struct gve_priv *priv = netdev_priv(netdev); - bool was_up = netif_carrier_ok(priv->dev); + bool was_up = netif_running(priv->dev); priv->suspend_cnt++; rtnl_lock();
GVE driver wrongly relies on netif_carrier_ok() to check the interface administrative state when resources are being allocated/deallocated for queue(s). netif_carrier_ok() needs to be replaced with netif_running() for all such cases. Administrative state is the result of "ip link set dev <dev> up/down". It reflects whether the administrator wants to use the device for traffic and the corresponding resources have been allocated. Fixes: 5f08cd3d6423 ("gve: Alloc before freeing when adjusting queues") Signed-off-by: Praveen Kaligineedi <pkaligineedi@google.com> Reviewed-by: Shailend Chand <shailend@google.com> Reviewed-by: Willem de Bruijn <willemb@google.com> --- drivers/net/ethernet/google/gve/gve_ethtool.c | 2 +- drivers/net/ethernet/google/gve/gve_main.c | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-)