netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc)
@ 2025-08-19 22:19 Tony Nguyen
  2025-08-19 22:19 ` [PATCH net v2 1/5] ice: fix NULL pointer dereference in ice_unplug_aux_dev() on reset Tony Nguyen
                   ` (6 more replies)
  0 siblings, 7 replies; 13+ messages in thread
From: Tony Nguyen @ 2025-08-19 22:19 UTC (permalink / raw)
  To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev; +Cc: Tony Nguyen

For ice:
Emil adds a check to ensure auxiliary device was created before
tear down to prevent NULL a pointer dereference and adds an unroll error
path on auxiliary device creation to stop a possible memory leak.

For ixgbe:
Jason Xing corrects a condition in which improper decrement can cause
improper budget value.

Maciej extends down states in which XDP cannot transmit and excludes XDP
rings from Tx hang checks.

For igc:
VladikSS moves setting of hardware device information to allow for proper
check of device ID.
---
v2:
- Drop patch 'ice: fix Rx page leak on multi-buffer frames'

v1: https://lore.kernel.org/netdev/20250815204205.1407768-1-anthony.l.nguyen@intel.com/

The following are changes since commit 01792bc3e5bdafa171dd83c7073f00e7de93a653:
  net: ti: icssg-prueth: Fix HSR and switch offload Enablement during firwmare reload.
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 100GbE

Emil Tantilov (2):
  ice: fix NULL pointer dereference in ice_unplug_aux_dev() on reset
  ice: fix possible leak in ice_plug_aux_dev() error path

Jason Xing (1):
  ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc

Maciej Fijalkowski (1):
  ixgbe: fix ndo_xdp_xmit() workloads

ValdikSS (1):
  igc: fix disabling L1.2 PCI-E link substate on I226 on init

 drivers/net/ethernet/intel/ice/ice.h          |  1 +
 drivers/net/ethernet/intel/ice/ice_idc.c      | 29 +++++++++-------
 drivers/net/ethernet/intel/igc/igc_main.c     | 14 ++++----
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 34 ++++++-------------
 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c  |  4 ++-
 5 files changed, 39 insertions(+), 43 deletions(-)

-- 
2.47.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH net v2 1/5] ice: fix NULL pointer dereference in ice_unplug_aux_dev() on reset
  2025-08-19 22:19 [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc) Tony Nguyen
@ 2025-08-19 22:19 ` Tony Nguyen
  2025-08-19 22:19 ` [PATCH net v2 2/5] ice: fix possible leak in ice_plug_aux_dev() error path Tony Nguyen
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Tony Nguyen @ 2025-08-19 22:19 UTC (permalink / raw)
  To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
  Cc: Emil Tantilov, anthony.l.nguyen, david.m.ertman,
	tatyana.e.nikolova, Przemek Kitszel

From: Emil Tantilov <emil.s.tantilov@intel.com>

Issuing a reset when the driver is loaded without RDMA support, will
results in a crash as it attempts to remove RDMA's non-existent auxbus
device:
echo 1 > /sys/class/net/<if>/device/reset

BUG: kernel NULL pointer dereference, address: 0000000000000008
...
RIP: 0010:ice_unplug_aux_dev+0x29/0x70 [ice]
...
Call Trace:
<TASK>
ice_prepare_for_reset+0x77/0x260 [ice]
pci_dev_save_and_disable+0x2c/0x70
pci_reset_function+0x88/0x130
reset_store+0x5a/0xa0
kernfs_fop_write_iter+0x15e/0x210
vfs_write+0x273/0x520
ksys_write+0x6b/0xe0
do_syscall_64+0x79/0x3b0
entry_SYSCALL_64_after_hwframe+0x76/0x7e

ice_unplug_aux_dev() checks pf->cdev_info->adev for NULL pointer, but
pf->cdev_info will also be NULL, leading to the deref in the trace above.

Introduce a flag to be set when the creation of the auxbus device is
successful, to avoid multiple NULL pointer checks in ice_unplug_aux_dev().

Fixes: c24a65b6a27c7 ("iidc/ice/irdma: Update IDC to support multiple consumers")
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h     |  1 +
 drivers/net/ethernet/intel/ice/ice_idc.c | 10 ++++++----
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 2098f00b3cd3..8a8a01a4bb40 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -510,6 +510,7 @@ enum ice_pf_flags {
 	ICE_FLAG_LINK_LENIENT_MODE_ENA,
 	ICE_FLAG_PLUG_AUX_DEV,
 	ICE_FLAG_UNPLUG_AUX_DEV,
+	ICE_FLAG_AUX_DEV_CREATED,
 	ICE_FLAG_MTU_CHANGED,
 	ICE_FLAG_GNSS,			/* GNSS successfully initialized */
 	ICE_FLAG_DPLL,			/* SyncE/PTP dplls initialized */
diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c b/drivers/net/ethernet/intel/ice/ice_idc.c
index 6ab53e430f91..420d45c2558b 100644
--- a/drivers/net/ethernet/intel/ice/ice_idc.c
+++ b/drivers/net/ethernet/intel/ice/ice_idc.c
@@ -336,6 +336,7 @@ int ice_plug_aux_dev(struct ice_pf *pf)
 	mutex_lock(&pf->adev_mutex);
 	cdev->adev = adev;
 	mutex_unlock(&pf->adev_mutex);
+	set_bit(ICE_FLAG_AUX_DEV_CREATED, pf->flags);
 
 	return 0;
 }
@@ -347,15 +348,16 @@ void ice_unplug_aux_dev(struct ice_pf *pf)
 {
 	struct auxiliary_device *adev;
 
+	if (!test_and_clear_bit(ICE_FLAG_AUX_DEV_CREATED, pf->flags))
+		return;
+
 	mutex_lock(&pf->adev_mutex);
 	adev = pf->cdev_info->adev;
 	pf->cdev_info->adev = NULL;
 	mutex_unlock(&pf->adev_mutex);
 
-	if (adev) {
-		auxiliary_device_delete(adev);
-		auxiliary_device_uninit(adev);
-	}
+	auxiliary_device_delete(adev);
+	auxiliary_device_uninit(adev);
 }
 
 /**
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH net v2 2/5] ice: fix possible leak in ice_plug_aux_dev() error path
  2025-08-19 22:19 [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc) Tony Nguyen
  2025-08-19 22:19 ` [PATCH net v2 1/5] ice: fix NULL pointer dereference in ice_unplug_aux_dev() on reset Tony Nguyen
@ 2025-08-19 22:19 ` Tony Nguyen
  2025-08-21  1:45   ` Jakub Kicinski
  2025-08-19 22:19 ` [PATCH net v2 3/5] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc Tony Nguyen
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 13+ messages in thread
From: Tony Nguyen @ 2025-08-19 22:19 UTC (permalink / raw)
  To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
  Cc: Emil Tantilov, anthony.l.nguyen, david.m.ertman,
	tatyana.e.nikolova, Przemek Kitszel, Aleksandr Loktionov

From: Emil Tantilov <emil.s.tantilov@intel.com>

Fix a memory leak in the error path where kfree(iadev) was not called
following an error in auxiliary_device_add().

Fixes: f9f5301e7e2d ("ice: Register auxiliary device to provide RDMA")
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_idc.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c b/drivers/net/ethernet/intel/ice/ice_idc.c
index 420d45c2558b..8c4a3dc22a7c 100644
--- a/drivers/net/ethernet/intel/ice/ice_idc.c
+++ b/drivers/net/ethernet/intel/ice/ice_idc.c
@@ -322,16 +322,12 @@ int ice_plug_aux_dev(struct ice_pf *pf)
 		"roce" : "iwarp";
 
 	ret = auxiliary_device_init(adev);
-	if (ret) {
-		kfree(iadev);
-		return ret;
-	}
+	if (ret)
+		goto free_iadev;
 
 	ret = auxiliary_device_add(adev);
-	if (ret) {
-		auxiliary_device_uninit(adev);
-		return ret;
-	}
+	if (ret)
+		goto aux_dev_uninit;
 
 	mutex_lock(&pf->adev_mutex);
 	cdev->adev = adev;
@@ -339,6 +335,13 @@ int ice_plug_aux_dev(struct ice_pf *pf)
 	set_bit(ICE_FLAG_AUX_DEV_CREATED, pf->flags);
 
 	return 0;
+
+aux_dev_uninit:
+	auxiliary_device_uninit(adev);
+free_iadev:
+	kfree(iadev);
+
+	return ret;
 }
 
 /* ice_unplug_aux_dev - unregister and free AUX device
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH net v2 3/5] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc
  2025-08-19 22:19 [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc) Tony Nguyen
  2025-08-19 22:19 ` [PATCH net v2 1/5] ice: fix NULL pointer dereference in ice_unplug_aux_dev() on reset Tony Nguyen
  2025-08-19 22:19 ` [PATCH net v2 2/5] ice: fix possible leak in ice_plug_aux_dev() error path Tony Nguyen
@ 2025-08-19 22:19 ` Tony Nguyen
  2025-08-19 22:19 ` [PATCH net v2 4/5] ixgbe: fix ndo_xdp_xmit() workloads Tony Nguyen
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Tony Nguyen @ 2025-08-19 22:19 UTC (permalink / raw)
  To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
  Cc: Jason Xing, anthony.l.nguyen, maciej.fijalkowski, magnus.karlsson,
	ast, daniel, hawk, john.fastabend, sdf, bpf, bjorn,
	przemyslaw.kitszel, larysa.zaremba, Paul Menzel,
	Aleksandr Loktionov, Priya Singh

From: Jason Xing <kernelxing@tencent.com>

Resolve the budget negative overflow which leads to returning true in
ixgbe_xmit_zc even when the budget of descs are thoroughly consumed.

Before this patch, when the budget is decreased to zero and finishes
sending the last allowed desc in ixgbe_xmit_zc, it will always turn back
and enter into the while() statement to see if it should keep processing
packets, but in the meantime it unexpectedly decreases the value again to
'unsigned int (0--)', namely, UINT_MAX. Finally, the ixgbe_xmit_zc returns
true, showing 'we complete cleaning the budget'. That also means
'clean_complete = true' in ixgbe_poll.

The true theory behind this is if that budget number of descs are consumed,
it implies that we might have more descs to be done. So we should return
false in ixgbe_xmit_zc to tell napi poll to find another chance to start
polling to handle the rest of descs. On the contrary, returning true here
means job done and we know we finish all the possible descs this time and
we don't intend to start a new napi poll.

It is apparently against our expectations. Please also see how
ixgbe_clean_tx_irq() handles the problem: it uses do..while() statement
to make sure the budget can be decreased to zero at most and the negative
overflow never happens.

The patch adds 'likely' because we rarely would not hit the loop condition
since the standard budget is 256.

Fixes: 8221c5eba8c1 ("ixgbe: add AF_XDP zero-copy Tx support")
Signed-off-by: Jason Xing <kernelxing@tencent.com>
Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Priya Singh <priyax.singh@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
index ac58964b2f08..7b941505a9d0 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
@@ -398,7 +398,7 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
 	dma_addr_t dma;
 	u32 cmd_type;
 
-	while (budget-- > 0) {
+	while (likely(budget)) {
 		if (unlikely(!ixgbe_desc_unused(xdp_ring))) {
 			work_done = false;
 			break;
@@ -433,6 +433,8 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget)
 		xdp_ring->next_to_use++;
 		if (xdp_ring->next_to_use == xdp_ring->count)
 			xdp_ring->next_to_use = 0;
+
+		budget--;
 	}
 
 	if (tx_desc) {
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH net v2 4/5] ixgbe: fix ndo_xdp_xmit() workloads
  2025-08-19 22:19 [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc) Tony Nguyen
                   ` (2 preceding siblings ...)
  2025-08-19 22:19 ` [PATCH net v2 3/5] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc Tony Nguyen
@ 2025-08-19 22:19 ` Tony Nguyen
  2025-08-19 22:19 ` [PATCH net v2 5/5] igc: fix disabling L1.2 PCI-E link substate on I226 on init Tony Nguyen
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Tony Nguyen @ 2025-08-19 22:19 UTC (permalink / raw)
  To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
  Cc: Maciej Fijalkowski, anthony.l.nguyen, magnus.karlsson, ast,
	daniel, hawk, john.fastabend, sdf, bpf, Tobias Böhm,
	Marcus Wichelmann, Aleksandr Loktionov

From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

Currently ixgbe driver checks periodically in its watchdog subtask if
there is anything to be transmitted (considering both Tx and XDP rings)
under state of carrier not being 'ok'. Such event is interpreted as Tx
hang and therefore results in interface reset.

This is currently problematic for ndo_xdp_xmit() as it is allowed to
produce descriptors when interface is going through reset or its carrier
is turned off.

Furthermore, XDP rings should not really be objects of Tx hang
detection. This mechanism is rather a matter of ndo_tx_timeout() being
called from dev_watchdog against Tx rings exposed to networking stack.

Taking into account issues described above, let us have a two fold fix -
do not respect XDP rings in local ixgbe watchdog and do not produce Tx
descriptors in ndo_xdp_xmit callback when there is some problem with
carrier currently. For now, keep the Tx hang checks in clean Tx irq
routine, but adjust it to not execute for XDP rings.

Cc: Tobias Böhm <tobias.boehm@hetzner-cloud.de>
Reported-by: Marcus Wichelmann <marcus.wichelmann@hetzner-cloud.de>
Closes: https://lore.kernel.org/netdev/eca1880f-253a-4955-afe6-732d7c6926ee@hetzner-cloud.de/
Fixes: 6453073987ba ("ixgbe: add initial support for xdp redirect")
Fixes: 33fdc82f0883 ("ixgbe: add support for XDP_TX action")
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Marcus Wichelmann <marcus.wichelmann@hetzner-cloud.de>
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 34 ++++++-------------
 1 file changed, 11 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 6122a0abb41f..80e6a2ef1350 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -968,10 +968,6 @@ static void ixgbe_update_xoff_rx_lfc(struct ixgbe_adapter *adapter)
 	for (i = 0; i < adapter->num_tx_queues; i++)
 		clear_bit(__IXGBE_HANG_CHECK_ARMED,
 			  &adapter->tx_ring[i]->state);
-
-	for (i = 0; i < adapter->num_xdp_queues; i++)
-		clear_bit(__IXGBE_HANG_CHECK_ARMED,
-			  &adapter->xdp_ring[i]->state);
 }
 
 static void ixgbe_update_xoff_received(struct ixgbe_adapter *adapter)
@@ -1214,7 +1210,7 @@ static void ixgbe_pf_handle_tx_hang(struct ixgbe_ring *tx_ring,
 	struct ixgbe_adapter *adapter = netdev_priv(tx_ring->netdev);
 	struct ixgbe_hw *hw = &adapter->hw;
 
-	e_err(drv, "Detected Tx Unit Hang%s\n"
+	e_err(drv, "Detected Tx Unit Hang\n"
 		   "  Tx Queue             <%d>\n"
 		   "  TDH, TDT             <%x>, <%x>\n"
 		   "  next_to_use          <%x>\n"
@@ -1222,16 +1218,14 @@ static void ixgbe_pf_handle_tx_hang(struct ixgbe_ring *tx_ring,
 		   "tx_buffer_info[next_to_clean]\n"
 		   "  time_stamp           <%lx>\n"
 		   "  jiffies              <%lx>\n",
-	      ring_is_xdp(tx_ring) ? " (XDP)" : "",
 	      tx_ring->queue_index,
 	      IXGBE_READ_REG(hw, IXGBE_TDH(tx_ring->reg_idx)),
 	      IXGBE_READ_REG(hw, IXGBE_TDT(tx_ring->reg_idx)),
 	      tx_ring->next_to_use, next,
 	      tx_ring->tx_buffer_info[next].time_stamp, jiffies);
 
-	if (!ring_is_xdp(tx_ring))
-		netif_stop_subqueue(tx_ring->netdev,
-				    tx_ring->queue_index);
+	netif_stop_subqueue(tx_ring->netdev,
+			    tx_ring->queue_index);
 }
 
 /**
@@ -1451,6 +1445,9 @@ static bool ixgbe_clean_tx_irq(struct ixgbe_q_vector *q_vector,
 				   total_bytes);
 	adapter->tx_ipsec += total_ipsec;
 
+	if (ring_is_xdp(tx_ring))
+		return !!budget;
+
 	if (check_for_tx_hang(tx_ring) && ixgbe_check_tx_hang(tx_ring)) {
 		if (adapter->hw.mac.type == ixgbe_mac_e610)
 			ixgbe_handle_mdd_event(adapter, tx_ring);
@@ -1468,9 +1465,6 @@ static bool ixgbe_clean_tx_irq(struct ixgbe_q_vector *q_vector,
 		return true;
 	}
 
-	if (ring_is_xdp(tx_ring))
-		return !!budget;
-
 #define TX_WAKE_THRESHOLD (DESC_NEEDED * 2)
 	txq = netdev_get_tx_queue(tx_ring->netdev, tx_ring->queue_index);
 	if (!__netif_txq_completed_wake(txq, total_packets, total_bytes,
@@ -7974,12 +7968,9 @@ static void ixgbe_check_hang_subtask(struct ixgbe_adapter *adapter)
 		return;
 
 	/* Force detection of hung controller */
-	if (netif_carrier_ok(adapter->netdev)) {
+	if (netif_carrier_ok(adapter->netdev))
 		for (i = 0; i < adapter->num_tx_queues; i++)
 			set_check_for_tx_hang(adapter->tx_ring[i]);
-		for (i = 0; i < adapter->num_xdp_queues; i++)
-			set_check_for_tx_hang(adapter->xdp_ring[i]);
-	}
 
 	if (!(adapter->flags & IXGBE_FLAG_MSIX_ENABLED)) {
 		/*
@@ -8199,13 +8190,6 @@ static bool ixgbe_ring_tx_pending(struct ixgbe_adapter *adapter)
 			return true;
 	}
 
-	for (i = 0; i < adapter->num_xdp_queues; i++) {
-		struct ixgbe_ring *ring = adapter->xdp_ring[i];
-
-		if (ring->next_to_use != ring->next_to_clean)
-			return true;
-	}
-
 	return false;
 }
 
@@ -11005,6 +10989,10 @@ static int ixgbe_xdp_xmit(struct net_device *dev, int n,
 	if (unlikely(test_bit(__IXGBE_DOWN, &adapter->state)))
 		return -ENETDOWN;
 
+	if (!netif_carrier_ok(adapter->netdev) ||
+	    !netif_running(adapter->netdev))
+		return -ENETDOWN;
+
 	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
 		return -EINVAL;
 
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH net v2 5/5] igc: fix disabling L1.2 PCI-E link substate on I226 on init
  2025-08-19 22:19 [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc) Tony Nguyen
                   ` (3 preceding siblings ...)
  2025-08-19 22:19 ` [PATCH net v2 4/5] ixgbe: fix ndo_xdp_xmit() workloads Tony Nguyen
@ 2025-08-19 22:19 ` Tony Nguyen
  2025-08-21  1:45 ` [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc) Jakub Kicinski
  2025-08-21  2:30 ` patchwork-bot+netdevbpf
  6 siblings, 0 replies; 13+ messages in thread
From: Tony Nguyen @ 2025-08-19 22:19 UTC (permalink / raw)
  To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
  Cc: ValdikSS, anthony.l.nguyen, vitaly.lifshits, dima.ruinskiy,
	Paul Menzel

From: ValdikSS <iam@valdikss.org.ru>

Device ID comparison in igc_is_device_id_i226 is performed before
the ID is set, resulting in always failing check on init.

Before the patch:
* L1.2 is not disabled on init
* L1.2 is properly disabled after suspend-resume cycle

With the patch:
* L1.2 is properly disabled both on init and after suspend-resume

How to test:
Connect to the 1G link with 300+ mbit/s Internet speed, and run
the download speed test, such as:

    curl -o /dev/null http://speedtest.selectel.ru/1GB

Without L1.2 disabled, the speed would be no more than ~200 mbit/s.
With L1.2 disabled, the speed would reach 1 gbit/s.
Note: it's required that the latency between your host and the remote
be around 3-5 ms, the test inside LAN (<1 ms latency) won't trigger the
issue.

Link: https://lore.kernel.org/intel-wired-lan/15248b4f-3271-42dd-8e35-02bfc92b25e1@intel.com
Fixes: 0325143b59c6 ("igc: disable L1.2 PCI-E link substate to avoid performance issue")
Signed-off-by: ValdikSS <iam@valdikss.org.ru>
Reviewed-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 458e5eaa92e5..e79b14d50b24 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -7149,6 +7149,13 @@ static int igc_probe(struct pci_dev *pdev,
 	adapter->port_num = hw->bus.func;
 	adapter->msg_enable = netif_msg_init(debug, DEFAULT_MSG_ENABLE);
 
+	/* PCI config space info */
+	hw->vendor_id = pdev->vendor;
+	hw->device_id = pdev->device;
+	hw->revision_id = pdev->revision;
+	hw->subsystem_vendor_id = pdev->subsystem_vendor;
+	hw->subsystem_device_id = pdev->subsystem_device;
+
 	/* Disable ASPM L1.2 on I226 devices to avoid packet loss */
 	if (igc_is_device_id_i226(hw))
 		pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
@@ -7175,13 +7182,6 @@ static int igc_probe(struct pci_dev *pdev,
 	netdev->mem_start = pci_resource_start(pdev, 0);
 	netdev->mem_end = pci_resource_end(pdev, 0);
 
-	/* PCI config space info */
-	hw->vendor_id = pdev->vendor;
-	hw->device_id = pdev->device;
-	hw->revision_id = pdev->revision;
-	hw->subsystem_vendor_id = pdev->subsystem_vendor;
-	hw->subsystem_device_id = pdev->subsystem_device;
-
 	/* Copy the default MAC and PHY function pointers */
 	memcpy(&hw->mac.ops, ei->mac_ops, sizeof(hw->mac.ops));
 	memcpy(&hw->phy.ops, ei->phy_ops, sizeof(hw->phy.ops));
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH net v2 2/5] ice: fix possible leak in ice_plug_aux_dev() error path
  2025-08-19 22:19 ` [PATCH net v2 2/5] ice: fix possible leak in ice_plug_aux_dev() error path Tony Nguyen
@ 2025-08-21  1:45   ` Jakub Kicinski
  2025-08-21  8:46     ` Przemek Kitszel
  0 siblings, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2025-08-21  1:45 UTC (permalink / raw)
  To: Tony Nguyen
  Cc: davem, pabeni, edumazet, andrew+netdev, netdev, Emil Tantilov,
	david.m.ertman, tatyana.e.nikolova, Przemek Kitszel,
	Aleksandr Loktionov

On Tue, 19 Aug 2025 15:19:56 -0700 Tony Nguyen wrote:
>  	ret = auxiliary_device_init(adev);
> -	if (ret) {
> -		kfree(iadev);
> -		return ret;
> -	}
> +	if (ret)
> +		goto free_iadev;
>  
>  	ret = auxiliary_device_add(adev);
> -	if (ret) {
> -		auxiliary_device_uninit(adev);
> -		return ret;

I think the code is correct as is. Once auxiliary_device_init()
returns the device is refcounted, auxiliary_device_uninit()
will call release, which is ice_adev_release(), which in turn
frees iadev.

> -	}
> +	if (ret)
> +		goto aux_dev_uninit;
>  
>  	mutex_lock(&pf->adev_mutex);
>  	cdev->adev = adev;
> @@ -339,6 +335,13 @@ int ice_plug_aux_dev(struct ice_pf *pf)
>  	set_bit(ICE_FLAG_AUX_DEV_CREATED, pf->flags);
>  
>  	return 0;
> +
> +aux_dev_uninit:
> +	auxiliary_device_uninit(adev);
> +free_iadev:
> +	kfree(iadev);
> +
> +	return ret;

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc)
  2025-08-19 22:19 [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc) Tony Nguyen
                   ` (4 preceding siblings ...)
  2025-08-19 22:19 ` [PATCH net v2 5/5] igc: fix disabling L1.2 PCI-E link substate on I226 on init Tony Nguyen
@ 2025-08-21  1:45 ` Jakub Kicinski
  2025-08-21  8:31   ` Przemek Kitszel
  2025-08-21  2:30 ` patchwork-bot+netdevbpf
  6 siblings, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2025-08-21  1:45 UTC (permalink / raw)
  To: Tony Nguyen; +Cc: davem, pabeni, edumazet, andrew+netdev, netdev

On Tue, 19 Aug 2025 15:19:54 -0700 Tony Nguyen wrote:
> For ice:
> Emil adds a check to ensure auxiliary device was created before
> tear down to prevent NULL a pointer dereference and adds an unroll error
> path on auxiliary device creation to stop a possible memory leak.

I'll apply the non-ice patches from the list, hope that's okay.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc)
  2025-08-19 22:19 [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc) Tony Nguyen
                   ` (5 preceding siblings ...)
  2025-08-21  1:45 ` [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc) Jakub Kicinski
@ 2025-08-21  2:30 ` patchwork-bot+netdevbpf
  6 siblings, 0 replies; 13+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-08-21  2:30 UTC (permalink / raw)
  To: Tony Nguyen; +Cc: davem, kuba, pabeni, edumazet, andrew+netdev, netdev

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 19 Aug 2025 15:19:54 -0700 you wrote:
> For ice:
> Emil adds a check to ensure auxiliary device was created before
> tear down to prevent NULL a pointer dereference and adds an unroll error
> path on auxiliary device creation to stop a possible memory leak.
> 
> For ixgbe:
> Jason Xing corrects a condition in which improper decrement can cause
> improper budget value.
> 
> [...]

Here is the summary with links:
  - [net,v2,1/5] ice: fix NULL pointer dereference in ice_unplug_aux_dev() on reset
    (no matching commit)
  - [net,v2,2/5] ice: fix possible leak in ice_plug_aux_dev() error path
    (no matching commit)
  - [net,v2,3/5] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc
    https://git.kernel.org/netdev/net/c/4d4d9ef9dfee
  - [net,v2,4/5] ixgbe: fix ndo_xdp_xmit() workloads
    https://git.kernel.org/netdev/net/c/f3d9f7fa7f5d
  - [net,v2,5/5] igc: fix disabling L1.2 PCI-E link substate on I226 on init
    https://git.kernel.org/netdev/net/c/1468c1f97cf3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc)
  2025-08-21  1:45 ` [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc) Jakub Kicinski
@ 2025-08-21  8:31   ` Przemek Kitszel
  2025-08-21 14:22     ` Jakub Kicinski
  0 siblings, 1 reply; 13+ messages in thread
From: Przemek Kitszel @ 2025-08-21  8:31 UTC (permalink / raw)
  To: Jakub Kicinski, Tony Nguyen
  Cc: davem, pabeni, edumazet, andrew+netdev, netdev

On 8/21/25 03:45, Jakub Kicinski wrote:
> On Tue, 19 Aug 2025 15:19:54 -0700 Tony Nguyen wrote:
>> For ice:
>> Emil adds a check to ensure auxiliary device was created before
>> tear down to prevent NULL a pointer dereference and adds an unroll error
>> path on auxiliary device creation to stop a possible memory leak.
> 
> I'll apply the non-ice patches from the list, hope that's okay.
> 

the first ice one fixes real problem, reproducible by various reset
scenarios

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net v2 2/5] ice: fix possible leak in ice_plug_aux_dev() error path
  2025-08-21  1:45   ` Jakub Kicinski
@ 2025-08-21  8:46     ` Przemek Kitszel
  0 siblings, 0 replies; 13+ messages in thread
From: Przemek Kitszel @ 2025-08-21  8:46 UTC (permalink / raw)
  To: Jakub Kicinski, Tony Nguyen, Jerome Brunet, Emil Tantilov
  Cc: davem, pabeni, edumazet, andrew+netdev, netdev, david.m.ertman,
	tatyana.e.nikolova, Aleksandr Loktionov

On 8/21/25 03:45, Jakub Kicinski wrote:
> On Tue, 19 Aug 2025 15:19:56 -0700 Tony Nguyen wrote:
>>   	ret = auxiliary_device_init(adev);
>> -	if (ret) {
>> -		kfree(iadev);
>> -		return ret;
>> -	}
>> +	if (ret)
>> +		goto free_iadev;
>>   
>>   	ret = auxiliary_device_add(adev);
>> -	if (ret) {
>> -		auxiliary_device_uninit(adev);
>> -		return ret;
> 
> I think the code is correct as is. Once auxiliary_device_init()
> returns the device is refcounted, auxiliary_device_uninit()
> will call release, which is ice_adev_release(), which in turn
> frees iadev.

you are right

It's nice, that a recent wrapper [1] added notes that exact bit of
wisdom as comment (what only proves such wrapper is a great abstraction,
thanks @Jerome Brunet):

drivers/base/auxiliary.c:
444│         ret = __auxiliary_device_add(auxdev, modname);
445│         if (ret) {
446│                 /*
447│                  * It may look odd but auxdev should not be freed here.
448│                  * auxiliary_device_uninit() calls device_put() 
which call
449│                  * the device release function, freeing auxdev.
450│                  */
451│                 auxiliary_device_uninit(auxdev);
452│                 return NULL;
453│         }

[1] eaa0d30216c1 ("driver core: auxiliary bus: add device creation helpers")

> 
>> -	}
>> +	if (ret)
>> +		goto aux_dev_uninit;
>>   
>>   	mutex_lock(&pf->adev_mutex);
>>   	cdev->adev = adev;
>> @@ -339,6 +335,13 @@ int ice_plug_aux_dev(struct ice_pf *pf)
>>   	set_bit(ICE_FLAG_AUX_DEV_CREATED, pf->flags);
>>   
>>   	return 0;
>> +
>> +aux_dev_uninit:
>> +	auxiliary_device_uninit(adev);
>> +free_iadev:
>> +	kfree(iadev);
>> +
>> +	return ret;


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc)
  2025-08-21  8:31   ` Przemek Kitszel
@ 2025-08-21 14:22     ` Jakub Kicinski
  2025-08-21 16:01       ` Tony Nguyen
  0 siblings, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2025-08-21 14:22 UTC (permalink / raw)
  To: Przemek Kitszel
  Cc: Tony Nguyen, davem, pabeni, edumazet, andrew+netdev, netdev

On Thu, 21 Aug 2025 10:31:59 +0200 Przemek Kitszel wrote:
> On 8/21/25 03:45, Jakub Kicinski wrote:
> > On Tue, 19 Aug 2025 15:19:54 -0700 Tony Nguyen wrote:  
> >> For ice:
> >> Emil adds a check to ensure auxiliary device was created before
> >> tear down to prevent NULL a pointer dereference and adds an unroll error
> >> path on auxiliary device creation to stop a possible memory leak.  
> > 
> > I'll apply the non-ice patches from the list, hope that's okay.
> 
> the first ice one fixes real problem, reproducible by various reset
> scenarios

Ack, just felt cleaner cutting the PR up by driver ;)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc)
  2025-08-21 14:22     ` Jakub Kicinski
@ 2025-08-21 16:01       ` Tony Nguyen
  0 siblings, 0 replies; 13+ messages in thread
From: Tony Nguyen @ 2025-08-21 16:01 UTC (permalink / raw)
  To: Jakub Kicinski, Przemek Kitszel
  Cc: davem, pabeni, edumazet, andrew+netdev, netdev



On 8/21/2025 7:22 AM, Jakub Kicinski wrote:
> On Thu, 21 Aug 2025 10:31:59 +0200 Przemek Kitszel wrote:
>> On 8/21/25 03:45, Jakub Kicinski wrote:
>>> On Tue, 19 Aug 2025 15:19:54 -0700 Tony Nguyen wrote:
>>>> For ice:
>>>> Emil adds a check to ensure auxiliary device was created before
>>>> tear down to prevent NULL a pointer dereference and adds an unroll error
>>>> path on auxiliary device creation to stop a possible memory leak.
>>>
>>> I'll apply the non-ice patches from the list, hope that's okay.
>>
>> the first ice one fixes real problem, reproducible by various reset
>> scenarios
> 
> Ack, just felt cleaner cutting the PR up by driver ;)

I'll include it in the next misc net send.

Thanks,
Tony

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2025-08-21 16:01 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-19 22:19 [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc) Tony Nguyen
2025-08-19 22:19 ` [PATCH net v2 1/5] ice: fix NULL pointer dereference in ice_unplug_aux_dev() on reset Tony Nguyen
2025-08-19 22:19 ` [PATCH net v2 2/5] ice: fix possible leak in ice_plug_aux_dev() error path Tony Nguyen
2025-08-21  1:45   ` Jakub Kicinski
2025-08-21  8:46     ` Przemek Kitszel
2025-08-19 22:19 ` [PATCH net v2 3/5] ixgbe: xsk: resolve the negative overflow of budget in ixgbe_xmit_zc Tony Nguyen
2025-08-19 22:19 ` [PATCH net v2 4/5] ixgbe: fix ndo_xdp_xmit() workloads Tony Nguyen
2025-08-19 22:19 ` [PATCH net v2 5/5] igc: fix disabling L1.2 PCI-E link substate on I226 on init Tony Nguyen
2025-08-21  1:45 ` [PATCH net v2 0/5][pull request] Intel Wired LAN Driver Updates 2025-08-15 (ice, ixgbe, igc) Jakub Kicinski
2025-08-21  8:31   ` Przemek Kitszel
2025-08-21 14:22     ` Jakub Kicinski
2025-08-21 16:01       ` Tony Nguyen
2025-08-21  2:30 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).