[Intel-wired-lan] [PATCH iwl-next v1 0/2] Implement ixgbe PCI reset

[Intel-wired-lan] [PATCH iwl-next v1 0/2] Implement ixgbe PCI reset

[Sergey Temerkhanov]

This series adds an ability to properly perform PCI level reset
via sysfs to the ixgbe driver.

To achieve this, several operation handlers are refactored to
check whether the netdev is available during the invocation, and
the actual reset handlers are implemented which prepare the device
for the reset and re-initialize it afterwards.

Sergey Temerkhanov (2):
  ixgbe: Refactor device operations to check whether netdev is available
  ixgbe: Implement PCI reset handler

 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   5 +
 .../net/ethernet/intel/ixgbe/ixgbe_dcb_nl.c   |  13 +-
 .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c  |  34 ++++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c |  20 +++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 112 ++++++++++++++++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c  |   4 +-
 6 files changed, 159 insertions(+), 29 deletions(-)

-- 
2.53.0

[Intel-wired-lan] [PATCH iwl-next v1 1/2] ixgbe: Refactor device operations to check whether netdev is available

[Sergey Temerkhanov]

Refactor several ixgbe driver operations to check whether the
netdev they operate on is enabled. This will allow the system
to get synchronized, for example, during the PCI resets.

Signed-off-by: Sergey Temerkhanov <sergey.temerkhanov@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |  5 +++
 .../net/ethernet/intel/ixgbe/ixgbe_dcb_nl.c   | 13 +++++--
 .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c  | 34 ++++++++++++-------
 drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c | 20 ++++++++---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 22 +++++++-----
 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c  |  4 +--
 6 files changed, 69 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 594ccb28da20..e801433c5db8 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -955,6 +955,11 @@ extern char ixgbe_driver_name[];
 extern char ixgbe_default_device_descr[];
 #endif /* IXGBE_FCOE */
 
+static inline bool ixgbe_netif_running(struct net_device *netdev)
+{
+	return netif_running(netdev) && netif_device_present(netdev);
+}
+
 int ixgbe_open(struct net_device *netdev);
 int ixgbe_close(struct net_device *netdev);
 void ixgbe_up(struct ixgbe_adapter *adapter);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_dcb_nl.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_dcb_nl.c
index 382d097e4b11..7c7408d32742 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_dcb_nl.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_dcb_nl.c
@@ -277,17 +277,23 @@ static void ixgbe_dcbnl_get_pfc_cfg(struct net_device *netdev, int priority,
 static void ixgbe_dcbnl_devreset(struct net_device *dev)
 {
 	struct ixgbe_adapter *adapter = ixgbe_from_netdev(dev);
+	bool running;
+
+	if (!netif_device_present(dev))
+		return;
+
+	running = ixgbe_netif_running(dev);
 
 	while (test_and_set_bit(__IXGBE_RESETTING, &adapter->state))
 		usleep_range(1000, 2000);
 
-	if (netif_running(dev))
+	if (running)
 		dev->netdev_ops->ndo_stop(dev);
 
 	ixgbe_clear_interrupt_scheme(adapter);
 	ixgbe_init_interrupt_scheme(adapter);
 
-	if (netif_running(dev))
+	if (running)
 		dev->netdev_ops->ndo_open(dev);
 
 	clear_bit(__IXGBE_RESETTING, &adapter->state);
@@ -515,6 +521,9 @@ static int ixgbe_dcbnl_ieee_setets(struct net_device *dev,
 	if (!(adapter->dcbx_cap & DCB_CAP_DCBX_VER_IEEE))
 		return -EINVAL;
 
+	if (!netif_device_present(dev))
+		return -ENETDOWN;
+
 	if (!adapter->ixgbe_ieee_ets) {
 		adapter->ixgbe_ieee_ets = kmalloc_obj(struct ieee_ets);
 		if (!adapter->ixgbe_ieee_ets)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 36e43b5e88d1..02aff411426d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -466,6 +466,9 @@ static int ixgbe_set_link_ksettings(struct net_device *netdev,
 	u32 advertised, old;
 	int err = 0;
 
+	if (!netif_device_present(netdev))
+		return -ENETDOWN;
+
 	if ((hw->phy.media_type == ixgbe_media_type_copper) ||
 	    (hw->phy.multispeed_fiber)) {
 		/*
@@ -576,9 +579,9 @@ static void ixgbe_set_pauseparam_finalize(struct net_device *netdev,
 	/* If the thing changed then we'll update and use new autoneg. */
 	if (memcmp(fc, &hw->fc, sizeof(*fc))) {
 		hw->fc = *fc;
-		if (netif_running(netdev))
+		if (ixgbe_netif_running(netdev))
 			ixgbe_reinit_locked(adapter);
-		else
+		else if (netif_device_present(netdev))
 			ixgbe_reset(adapter);
 	}
 }
@@ -1266,7 +1269,7 @@ static int ixgbe_set_ringparam(struct net_device *netdev,
 	while (test_and_set_bit(__IXGBE_RESETTING, &adapter->state))
 		usleep_range(1000, 2000);
 
-	if (!netif_running(adapter->netdev)) {
+	if (!ixgbe_netif_running(adapter->netdev)) {
 		for (i = 0; i < adapter->num_tx_queues; i++)
 			adapter->tx_ring[i]->count = new_tx_count;
 		for (i = 0; i < adapter->num_xdp_queues; i++)
@@ -2249,10 +2252,11 @@ static void ixgbe_diag_test(struct net_device *netdev,
 			    struct ethtool_test *eth_test, u64 *data)
 {
 	struct ixgbe_adapter *adapter = ixgbe_from_netdev(netdev);
-	bool if_running = netif_running(netdev);
+	bool if_running = ixgbe_netif_running(netdev);
 
-	if (ixgbe_removed(adapter->hw.hw_addr)) {
-		e_err(hw, "Adapter removed - test blocked\n");
+	if (ixgbe_removed(adapter->hw.hw_addr) ||
+	    !netif_device_present(netdev)) {
+		e_err(hw, "Adapter removed or detached - test blocked\n");
 		data[0] = 1;
 		data[1] = 1;
 		data[2] = 1;
@@ -2466,7 +2470,7 @@ static int ixgbe_nway_reset(struct net_device *netdev)
 {
 	struct ixgbe_adapter *adapter = ixgbe_from_netdev(netdev);
 
-	if (netif_running(netdev))
+	if (ixgbe_netif_running(netdev))
 		ixgbe_reinit_locked(adapter);
 
 	return 0;
@@ -2650,7 +2654,8 @@ static int ixgbe_set_coalesce(struct net_device *netdev,
 		else
 			/* rx only or mixed */
 			q_vector->itr = rx_itr_param;
-		ixgbe_write_eitr(q_vector);
+		if (netif_device_present(netdev))
+			ixgbe_write_eitr(q_vector);
 	}
 
 	/*
@@ -3694,6 +3699,9 @@ static int ixgbe_set_eee_e610(struct net_device *netdev,
 	    kedata->eee_enabled)
 		return -EOPNOTSUPP;
 
+	if (!netif_device_present(netdev))
+		return -ENETDOWN;
+
 	hw->phy.eee_speeds_advertised = kedata->eee_enabled ?
 					hw->phy.eee_speeds_supported : 0;
 
@@ -3709,9 +3717,9 @@ static int ixgbe_set_eee_e610(struct net_device *netdev,
 	else
 		adapter->flags2 &= ~IXGBE_FLAG2_EEE_ENABLED;
 
-	if (netif_running(netdev))
+	if (ixgbe_netif_running(netdev))
 		ixgbe_reinit_locked(adapter);
-	else
+	else if (netif_device_present(netdev))
 		ixgbe_reset(adapter);
 
 	return 0;
@@ -3793,9 +3801,9 @@ static int ixgbe_set_eee(struct net_device *netdev, struct ethtool_keee *edata)
 	}
 
 	/* reset link */
-	if (netif_running(netdev))
+	if (ixgbe_netif_running(netdev))
 		ixgbe_reinit_locked(adapter);
-	else
+	else if (netif_device_present(netdev))
 		ixgbe_reset(adapter);
 
 	return 0;
@@ -3851,7 +3859,7 @@ static int ixgbe_set_priv_flags(struct net_device *netdev, u32 priv_flags)
 		adapter->flags2 = flags2;
 
 		/* reset interface to repopulate queues */
-		if (netif_running(netdev))
+		if (ixgbe_netif_running(netdev))
 			ixgbe_reinit_locked(adapter);
 	}
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
index e338ff0e6522..a22c20c73cdc 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
@@ -853,6 +853,10 @@ int ixgbe_fcoe_enable(struct net_device *netdev)
 {
 	struct ixgbe_adapter *adapter = ixgbe_from_netdev(netdev);
 	struct ixgbe_fcoe *fcoe = &adapter->fcoe;
+	bool running;
+
+	if (!netif_device_present(netdev))
+		return -ENETDOWN;
 
 	atomic_inc(&fcoe->refcnt);
 
@@ -862,12 +866,14 @@ int ixgbe_fcoe_enable(struct net_device *netdev)
 	if (adapter->flags & IXGBE_FLAG_FCOE_ENABLED)
 		return -EINVAL;
 
+	running = ixgbe_netif_running(netdev);
+
 	e_info(drv, "Enabling FCoE offload features.\n");
 
 	if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
 		e_warn(probe, "Enabling FCoE on PF will disable legacy VFs\n");
 
-	if (netif_running(netdev))
+	if (running)
 		netdev->netdev_ops->ndo_stop(netdev);
 
 	/* Allocate per CPU memory to track DDP pools */
@@ -882,7 +888,7 @@ int ixgbe_fcoe_enable(struct net_device *netdev)
 	ixgbe_clear_interrupt_scheme(adapter);
 	ixgbe_init_interrupt_scheme(adapter);
 
-	if (netif_running(netdev))
+	if (running)
 		netdev->netdev_ops->ndo_open(netdev);
 
 	return 0;
@@ -899,6 +905,10 @@ int ixgbe_fcoe_enable(struct net_device *netdev)
 int ixgbe_fcoe_disable(struct net_device *netdev)
 {
 	struct ixgbe_adapter *adapter = ixgbe_from_netdev(netdev);
+	bool running;
+
+	if (!netif_device_present(netdev))
+		return -ENETDOWN;
 
 	if (!atomic_dec_and_test(&adapter->fcoe.refcnt))
 		return -EINVAL;
@@ -906,8 +916,10 @@ int ixgbe_fcoe_disable(struct net_device *netdev)
 	if (!(adapter->flags & IXGBE_FLAG_FCOE_ENABLED))
 		return -EINVAL;
 
+	running = ixgbe_netif_running(netdev);
+
 	e_info(drv, "Disabling FCoE offload features.\n");
-	if (netif_running(netdev))
+	if (running)
 		netdev->netdev_ops->ndo_stop(netdev);
 
 	/* Free per CPU memory to track DDP pools */
@@ -923,7 +935,7 @@ int ixgbe_fcoe_disable(struct net_device *netdev)
 	ixgbe_clear_interrupt_scheme(adapter);
 	ixgbe_init_interrupt_scheme(adapter);
 
-	if (netif_running(netdev))
+	if (running)
 		netdev->netdev_ops->ndo_open(netdev);
 
 	return 0;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 2ac274c73d61..42dac766c907 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7420,7 +7420,7 @@ static int ixgbe_change_mtu(struct net_device *netdev, int new_mtu)
 	/* must set new MTU before calling down or up */
 	WRITE_ONCE(netdev->mtu, new_mtu);
 
-	if (netif_running(netdev))
+	if (ixgbe_netif_running(netdev))
 		ixgbe_reinit_locked(adapter);
 
 	return 0;
@@ -9917,6 +9917,7 @@ int ixgbe_setup_tc(struct net_device *dev, u8 tc)
 {
 	struct ixgbe_adapter *adapter = ixgbe_from_netdev(dev);
 	struct ixgbe_hw *hw = &adapter->hw;
+	bool running = ixgbe_netif_running(dev);
 
 	/* Hardware supports up to 8 traffic classes */
 	if (tc > adapter->dcb_cfg.num_tcs.pg_tcs)
@@ -9929,7 +9930,10 @@ int ixgbe_setup_tc(struct net_device *dev, u8 tc)
 	 * match packet buffer alignment. Unfortunately, the
 	 * hardware is not flexible enough to do this dynamically.
 	 */
-	if (netif_running(dev))
+	if (!netif_device_present(dev))
+		return -ENETDOWN;
+
+	if (running)
 		ixgbe_close(dev);
 	else
 		ixgbe_reset(adapter);
@@ -9942,7 +9946,7 @@ int ixgbe_setup_tc(struct net_device *dev, u8 tc)
 			e_warn(probe, "DCB is not supported with XDP\n");
 
 			ixgbe_init_interrupt_scheme(adapter);
-			if (netif_running(dev))
+			if (running)
 				ixgbe_open(dev);
 			return -EINVAL;
 		}
@@ -9977,7 +9981,7 @@ int ixgbe_setup_tc(struct net_device *dev, u8 tc)
 
 	ixgbe_defrag_macvlan_pools(dev);
 
-	if (netif_running(dev))
+	if (running)
 		return ixgbe_open(dev);
 
 	return 0;
@@ -10503,9 +10507,9 @@ void ixgbe_do_reset(struct net_device *netdev)
 {
 	struct ixgbe_adapter *adapter = ixgbe_from_netdev(netdev);
 
-	if (netif_running(netdev))
+	if (ixgbe_netif_running(netdev))
 		ixgbe_reinit_locked(adapter);
-	else
+	else if (netif_device_present(netdev))
 		ixgbe_reset(adapter);
 }
 
@@ -10837,7 +10841,7 @@ static void *ixgbe_fwd_add(struct net_device *pdev, struct net_device *vdev)
 	accel->pool = pool;
 	accel->netdev = vdev;
 
-	if (!netif_running(pdev))
+	if (!ixgbe_netif_running(pdev))
 		return accel;
 
 	err = ixgbe_fwd_ring_up(adapter, accel);
@@ -10968,8 +10972,10 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
 			synchronize_rcu();
 		err = ixgbe_setup_tc(dev, adapter->hw_tcs);
 
-		if (err)
+		if (err) {
+			xchg(&adapter->xdp_prog, old_prog);
 			return -EINVAL;
+		}
 		if (!prog)
 			xdp_features_clear_redirect_target(dev);
 	} else {
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
index 89f96c463f02..02820982b202 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
@@ -39,7 +39,7 @@ static int ixgbe_xsk_pool_enable(struct ixgbe_adapter *adapter,
 	if (err)
 		return err;
 
-	if_running = netif_running(adapter->netdev) &&
+	if_running = ixgbe_netif_running(adapter->netdev) &&
 		     ixgbe_enabled_xdp_adapter(adapter);
 
 	if (if_running)
@@ -71,7 +71,7 @@ static int ixgbe_xsk_pool_disable(struct ixgbe_adapter *adapter, u16 qid)
 	if (!pool)
 		return -EINVAL;
 
-	if_running = netif_running(adapter->netdev) &&
+	if_running = ixgbe_netif_running(adapter->netdev) &&
 		     ixgbe_enabled_xdp_adapter(adapter);
 
 	if (if_running)
-- 
2.53.0

[Intel-wired-lan] [PATCH iwl-next v1 2/2] ixgbe: Implement PCI reset handler

[Sergey Temerkhanov]

Implement PCI device reset handler to allow the network device to
get re-initialized and function after a PCI-level reset.

This is necessary for the adapter to avoid TX queue timeouts
occurring after the PCI reset is performed via sysfs during
its operation.

The reset codepath may trigger a number of dependencies in the
reset of the driver, so that it is necessary to check if
the netdev is present and running there.

Signed-off-by: Sergey Temerkhanov <sergey.temerkhanov@intel.com>
Reviewed-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 90 +++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 42dac766c907..1865b604ace7 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -12358,6 +12358,94 @@ static pci_ers_result_t ixgbe_io_slot_reset(struct pci_dev *pdev)
 	return result;
 }
 
+/**
+ * ixgbe_pci_reset_prepare - called before the pci bus is reset.
+ * @pdev: Pointer to PCI device
+ *
+ * Quiesce the driver in preparation for a PCI function reset. Called from
+ * pci_dev_save_and_disable() before the core saves config state and writes
+ * PCI_COMMAND_INTX_DISABLE to clear bus mastering and MMIO decode, so MMIO
+ * access to the device is still valid here.
+ */
+static void ixgbe_pci_reset_prepare(struct pci_dev *pdev)
+{
+	struct ixgbe_adapter *adapter = pci_get_drvdata(pdev);
+	struct net_device *netdev;
+
+	if (!adapter)
+		return;
+
+	netdev = adapter->netdev;
+
+	rtnl_lock();
+	netif_device_detach(netdev);
+	if (netif_running(netdev))
+		ixgbe_close_suspend(adapter);
+	rtnl_unlock();
+
+	/* __IXGBE_RESETTING is intentionally not set here: it is spun on
+	 * while holding rtnl by ixgbe_reinit_locked(), ixgbe_dcbnl_devreset()
+	 * and the ethtool reset paths, so holding it across the rtnl drop
+	 * would deadlock those callers against ixgbe_pci_reset_done(), which
+	 * needs to re-acquire rtnl.  During the reset window concurrent
+	 * rtnl-holding paths must treat the netdev as detached, while teardown
+	 * paths also observe __IXGBE_DOWN set by ixgbe_down() via
+	 * ixgbe_close_suspend(), matching the existing ixgbe_io_error_detected()
+	 * flow.
+	 */
+
+	if (test_bit(__IXGBE_SERVICE_INITED, &adapter->state)) {
+		/* The service timer was already stopped by ixgbe_down() via
+		 * ixgbe_close_suspend(); if the netdev was not running, the
+		 * timer is not armed.  Only the currently queued service task
+		 * (if any) still needs to be flushed here.
+		 */
+		cancel_work_sync(&adapter->service_task);
+		clear_bit(__IXGBE_SERVICE_SCHED, &adapter->state);
+	}
+}
+
+/**
+ * ixgbe_pci_reset_done - called after the pci bus has been reset.
+ * @pdev: Pointer to PCI device
+ *
+ * Re-initialize the device after a PCI function reset. The PCI core has
+ * already called pci_restore_state() before invoking this callback, so the
+ * saved Command register (including bus mastering) is back in place.
+ */
+static void ixgbe_pci_reset_done(struct pci_dev *pdev)
+{
+	struct ixgbe_adapter *adapter = pci_get_drvdata(pdev);
+	struct net_device *netdev;
+	bool running;
+	int err = 0;
+
+	if (!adapter)
+		return;
+
+	netdev = adapter->netdev;
+
+	rtnl_lock();
+	adapter->hw.hw_addr = adapter->io_addr;
+	ixgbe_reset(adapter);
+	IXGBE_WRITE_REG(&adapter->hw, IXGBE_WUS, ~0);
+	running = netif_running(netdev);
+	if (running) {
+		err = ixgbe_open(netdev);
+		if (err) {
+			e_dev_err("Cannot re-open netdev after PCI reset: %d. A new reset is needed.\n",
+				  err);
+			dev_close(netdev);
+		}
+	}
+	/* Restore presence so userspace can retry later. If ixgbe_open() failed,
+	 * dev_close() cleared IFF_UP first so netif_device_attach() will not wake
+	 * Tx queues without a successful open.
+	 */
+	netif_device_attach(netdev);
+	rtnl_unlock();
+}
+
 /**
  * ixgbe_io_resume - called when traffic can start flowing again.
  * @pdev: Pointer to PCI device
@@ -12390,6 +12478,8 @@ static const struct pci_error_handlers ixgbe_err_handler = {
 	.error_detected = ixgbe_io_error_detected,
 	.slot_reset = ixgbe_io_slot_reset,
 	.resume = ixgbe_io_resume,
+	.reset_prepare = ixgbe_pci_reset_prepare,
+	.reset_done = ixgbe_pci_reset_done,
 };
 
 static DEFINE_SIMPLE_DEV_PM_OPS(ixgbe_pm_ops, ixgbe_suspend, ixgbe_resume);
-- 
2.53.0

Re: [Intel-wired-lan] [PATCH iwl-next v1 1/2] ixgbe: Refactor device operations to check whether netdev is available

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://netdev-ai.bots.linux.dev/sashiko/
---
ixgbe: Refactor device operations to check whether netdev is available

This commit refactors several ixgbe driver operations to check whether
the netdev they operate on is enabled by introducing an ixgbe_netif_running()
helper and adding netif_device_present() guards across the ethtool, DCB,
FCoE and XDP code paths.

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 8873a8cc4a18..12361ed58a0f 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

[ ... ]

> @@ -10954,8 +10958,10 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
>  			synchronize_rcu();
>  		err = ixgbe_setup_tc(dev, adapter->hw_tcs);
>  
> -		if (err)
> +		if (err) {
> +			xchg(&adapter->xdp_prog, old_prog);
>  			return -EINVAL;
> +		}
>  		if (!prog)
>  			xdp_features_clear_redirect_target(dev);
>  	} else {

[Medium]
The xchg(&adapter->xdp_prog, old_prog) rollback on the ixgbe_setup_tc()
error path looks like a semantic fix rather than a refactor.

Before this patch the error path left adapter->xdp_prog pointing at prog:

    old_prog = xchg(&adapter->xdp_prog, prog);
    ...
    err = ixgbe_setup_tc(dev, adapter->hw_tcs);
    if (err)
        return -EINVAL;

while the ndo_bpf caller dev_xdp_install() drops its reference on prog
when the driver returns an error:

    if (err) {
        if (prog)
            bpf_prog_put(prog);
        return err;
    }

Can this leave adapter->xdp_prog as a dangling pointer to a freed
bpf_prog on the pre-patch code, which is what the new xchg() restores?

The commit message describes only a refactor and does not mention this
change.

Should the xdp_prog rollback be split into its own patch with a Fixes:
tag pointing at the commit that introduced the missing rollback, per
Documentation/process/submitting-patches.rst which asks for one logical
change per patch?

The new behavior also skips the subsequent if (old_prog) bpf_prog_put(old_prog)
in the failure path, which is a separate semantic worth calling out in the
commit log.

Re: [Intel-wired-lan] [PATCH iwl-next v1 1/2] ixgbe: Refactor device operations to check whether netdev is available

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
--- Patch [1]: [PATCH iwl-next v1 1/2] ixgbe: Refactor device operations to check whether netdev is available ---
commit f7037ec91fe4185b79d89bd64ec8042f9f9b1904
Author: Sergey Temerkhanov <sergey.temerkhanov@intel.com>

ixgbe: Refactor device operations to check whether netdev is available

Refactor several ixgbe driver operations to check whether the
netdev they operate on is enabled. This will allow the system
to get synchronized, for example, during the PCI resets.

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
> index 011fda9c61939..e4cd4673f21c2 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
> @@ -836,6 +836,10 @@ int ixgbe_fcoe_enable(struct net_device *netdev)
>  {
>  	struct ixgbe_adapter *adapter = ixgbe_from_netdev(netdev);
>  	struct ixgbe_fcoe *fcoe = &adapter->fcoe;
> +	bool running;
> +
> +	if (!netif_device_present(netdev))
> +		return -ENETDOWN;
>  
>  	atomic_inc(&fcoe->refcnt);
>  
> @@ -845,12 +849,14 @@ int ixgbe_fcoe_enable(struct net_device *netdev)
>  	if (adapter->flags & IXGBE_FLAG_FCOE_ENABLED)
>  		return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does this leak the FCoE reference count?

If ixgbe_fcoe_enable() is called when FCoE is already enabled or the adapter
is not FCoE capable, the function returns early without decrementing the
freshly incremented fcoe->refcnt.

Later, when ixgbe_fcoe_disable() is called, atomic_dec_and_test() will not
reach zero, causing it to return -EINVAL without freeing the FCoE DDP pool or
clearing the FCoE flags.

[ ... ]
> @@ -882,6 +888,10 @@ int ixgbe_fcoe_enable(struct net_device *netdev)
>  int ixgbe_fcoe_disable(struct net_device *netdev)
>  {
>  	struct ixgbe_adapter *adapter = ixgbe_from_netdev(netdev);
> +	bool running;
> +
> +	if (!netif_device_present(netdev))
> +		return -ENETDOWN;

[Severity: High]
Does this early return cause a memory leak of the FCoE DDP pool?

During a hardware error or device hot-unplug, ixgbe_io_error_detected() calls
netif_device_detach(), which clears the device present state. When the
driver is subsequently removed, the FCoE stack catches the unregister event
and calls ndo_fcoe_disable().

Because the device is already detached, this new early return triggers,
causing the function to skip calling ixgbe_fcoe_ddp_disable(). Will this
abandon the per-CPU DDP pool so it is never freed?

>  
>  	if (!atomic_dec_and_test(&adapter->fcoe.refcnt))
>  		return -EINVAL;
[ ... ]
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 8873a8cc4a185..12361ed58a0ff 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
[ ... ]
> @@ -10954,8 +10958,10 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
>  			synchronize_rcu();
>  		err = ixgbe_setup_tc(dev, adapter->hw_tcs);
>  
> -		if (err)
> +		if (err) {
> +			xchg(&adapter->xdp_prog, old_prog);
>  			return -EINVAL;
> +		}

[Severity: Critical]
This is a pre-existing issue, but does this error path leave a freed BPF
program installed in the active Rx rings?

If DCB is enabled on the interface, ixgbe_setup_tc() returns -EINVAL.
However, before returning, it calls ixgbe_open() to restart the interface:

ixgbe_setup_tc() {
        ...
        if (running)
                return ixgbe_open(dev);
}

During this restart, the Rx rings are initialized and assigned the new BPF
program currently stored in adapter->xdp_prog.

When ixgbe_xdp_setup() sees the error, it restores old_prog and returns
-EINVAL to the network stack, which then frees the new program. The
rx_ring[i]->xdp_prog pointers still reference the freed program. Can this
lead to a use-after-free?

Re: [Intel-wired-lan] [PATCH iwl-next v1 1/2] ixgbe: Refactor device operations to check whether netdev is available

[Temerkhanov, Sergey]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Thursday, July 16, 2026 3:03 PM
> To: Temerkhanov, Sergey <sergey.temerkhanov@intel.com>
> Cc: Simon Horman <horms@kernel.org>; intel-wired-lan@lists.osuosl.org;
> netdev@vger.kernel.org
> Subject: Re: [PATCH iwl-next v1 1/2] ixgbe: Refactor device operations to
> check whether netdev is available
> 
> This is an AI-generated review of your patch. The human sending this email
> has considered the AI review valid, or at least plausible.
> https://netdev-ai.bots.linux.dev/sashiko/
> ---
> ixgbe: Refactor device operations to check whether netdev is available
> 
> This commit refactors several ixgbe driver operations to check whether the
> netdev they operate on is enabled by introducing an ixgbe_netif_running()
> helper and adding netif_device_present() guards across the ethtool, DCB,
> FCoE and XDP code paths.
> 
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > index 8873a8cc4a18..12361ed58a0f 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> 
> [ ... ]
> 
> > @@ -10954,8 +10958,10 @@ static int ixgbe_xdp_setup(struct net_device
> *dev, struct bpf_prog *prog)
> >  			synchronize_rcu();
> >  		err = ixgbe_setup_tc(dev, adapter->hw_tcs);
> >
> > -		if (err)
> > +		if (err) {
> > +			xchg(&adapter->xdp_prog, old_prog);
> >  			return -EINVAL;
> > +		}
> >  		if (!prog)
> >  			xdp_features_clear_redirect_target(dev);
> >  	} else {
> 
> [Medium]
> The xchg(&adapter->xdp_prog, old_prog) rollback on the ixgbe_setup_tc()
> error path looks like a semantic fix rather than a refactor.
> 
> Before this patch the error path left adapter->xdp_prog pointing at prog:
> 
>     old_prog = xchg(&adapter->xdp_prog, prog);
>     ...
>     err = ixgbe_setup_tc(dev, adapter->hw_tcs);
>     if (err)
>         return -EINVAL;
> 
> while the ndo_bpf caller dev_xdp_install() drops its reference on prog when
> the driver returns an error:
> 
>     if (err) {
>         if (prog)
>             bpf_prog_put(prog);
>         return err;
>     }
> 
> Can this leave adapter->xdp_prog as a dangling pointer to a freed bpf_prog on
> the pre-patch code, which is what the new xchg() restores?
> 
> The commit message describes only a refactor and does not mention this
> change.
> 
> Should the xdp_prog rollback be split into its own patch with a Fixes:
> tag pointing at the commit that introduced the missing rollback, per
> Documentation/process/submitting-patches.rst which asks for one logical
> change per patch?
> 
> The new behavior also skips the subsequent if (old_prog)
> bpf_prog_put(old_prog) in the failure path, which is a separate semantic
> worth calling out in the commit log.

This part is going to a separate patch in v2

Regards,
Sergey