[PATCH net-next v1 0/3] wangxun: improve service task synchronization

Netdev List
 help / color / mirror / Atom feed

* [PATCH net-next v1 0/3] wangxun: improve service task synchronization
@ 2026-05-19  8:00 Jiawen Wu
  2026-05-19  8:00 ` [PATCH net-next v1 1/3] net: wangxun: introduce WX_STATE_DOWN to serialize device shutdown state Jiawen Wu
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Jiawen Wu @ 2026-05-19  8:00 UTC (permalink / raw)
  To: netdev
  Cc: Mengyuan Lou, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman, Kees Cook,
	Larysa Zaremba, Jacob Keller, Jiawen Wu

This series improves synchronization between asynchronous service work,
device teardown, and module event handling in the Wangxun drivers.

Jiawen Wu (3):
  net: wangxun: introduce WX_STATE_DOWN to serialize device shutdown
    state
  net: wangxun: avoid statistics updates during device teardown
  net: txgbe: rework service event handling

 drivers/net/ethernet/wangxun/libwx/wx_hw.c    |  3 ++-
 drivers/net/ethernet/wangxun/libwx/wx_lib.c   |  9 ++++---
 drivers/net/ethernet/wangxun/libwx/wx_sriov.c |  2 +-
 drivers/net/ethernet/wangxun/libwx/wx_type.h  |  1 +
 .../net/ethernet/wangxun/libwx/wx_vf_common.c |  8 ++++--
 drivers/net/ethernet/wangxun/ngbe/ngbe_main.c | 14 ++++++-----
 .../net/ethernet/wangxun/txgbe/txgbe_aml.c    | 10 +++-----
 .../net/ethernet/wangxun/txgbe/txgbe_aml.h    |  2 +-
 .../net/ethernet/wangxun/txgbe/txgbe_irq.c    |  2 +-
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   | 25 ++++++++-----------
 10 files changed, 39 insertions(+), 37 deletions(-)

-- 
2.51.0


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

* [PATCH net-next v1 1/3] net: wangxun: introduce WX_STATE_DOWN to serialize device shutdown state
  2026-05-19  8:00 [PATCH net-next v1 0/3] wangxun: improve service task synchronization Jiawen Wu
@ 2026-05-19  8:00 ` Jiawen Wu
  2026-05-22  7:34   ` Simon Horman
  2026-05-19  8:00 ` [PATCH net-next v1 2/3] net: wangxun: avoid statistics updates during device teardown Jiawen Wu
  2026-05-19  8:00 ` [PATCH net-next v1 3/3] net: txgbe: rework service event handling Jiawen Wu
  2 siblings, 1 reply; 6+ messages in thread
From: Jiawen Wu @ 2026-05-19  8:00 UTC (permalink / raw)
  To: netdev
  Cc: Mengyuan Lou, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman, Kees Cook,
	Larysa Zaremba, Jacob Keller, Jiawen Wu

Replace various netif_running() checks with an explicit WX_STATE_DOWN
state bit to track whether the device datapath and interrupt handling
are operational.

The previous logic relied on netif_running() to gate interrupt
reenablement, queue wakeups, statistics updates, and service task
execution. However, netif_running() only reflects the administrative
state of the netdevice and does not fully serialize against teardown
and reset paths. During device shutdown and reset flows, asynchronous
contexts such as interrupt handlers, NAPI poll, and service work could
still observe netif_running() as true while device resources were
already being disabled or freed.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/libwx/wx_hw.c        |  3 ++-
 drivers/net/ethernet/wangxun/libwx/wx_lib.c       |  9 ++++++---
 drivers/net/ethernet/wangxun/libwx/wx_sriov.c     |  2 +-
 drivers/net/ethernet/wangxun/libwx/wx_type.h      |  1 +
 drivers/net/ethernet/wangxun/libwx/wx_vf_common.c |  8 ++++++--
 drivers/net/ethernet/wangxun/ngbe/ngbe_main.c     | 12 ++++++++----
 drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c    |  2 +-
 drivers/net/ethernet/wangxun/txgbe/txgbe_main.c   |  4 ++++
 8 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
index 2451f6b20b11..260e14d5d541 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
@@ -2520,6 +2520,7 @@ int wx_sw_init(struct wx *wx)
 	mutex_init(&wx->reset_lock);
 	bitmap_zero(wx->state, WX_STATE_NBITS);
 	bitmap_zero(wx->flags, WX_PF_FLAGS_NBITS);
+	set_bit(WX_STATE_DOWN, wx->state);
 	wx->misc_irq_domain = false;
 
 	return 0;
@@ -2875,7 +2876,7 @@ void wx_update_stats(struct wx *wx)
 	u64 restart_queue = 0, tx_busy = 0;
 	u32 i;
 
-	if (!netif_running(wx->netdev) ||
+	if (test_bit(WX_STATE_DOWN, wx->state) ||
 	    test_bit(WX_STATE_RESETTING, wx->state))
 		return;
 
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
index 746623fa59b4..69fe19737679 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
@@ -876,7 +876,7 @@ static bool wx_clean_tx_irq(struct wx_q_vector *q_vector,
 
 		if (__netif_subqueue_stopped(tx_ring->netdev,
 					     tx_ring->queue_index) &&
-		    netif_running(tx_ring->netdev)) {
+		    !test_bit(WX_STATE_DOWN, wx->state)) {
 			netif_wake_subqueue(tx_ring->netdev,
 					    tx_ring->queue_index);
 			++tx_ring->tx_stats.restart_queue;
@@ -964,7 +964,7 @@ static int wx_poll(struct napi_struct *napi, int budget)
 	if (likely(napi_complete_done(napi, work_done))) {
 		if (wx->adaptive_itr)
 			wx_update_dim_sample(q_vector);
-		if (netif_running(wx->netdev))
+		if (!test_bit(WX_STATE_DOWN, wx->state))
 			wx_intr_enable(wx, WX_INTR_Q(q_vector->v_idx));
 	}
 
@@ -2341,6 +2341,8 @@ int wx_init_interrupt_scheme(struct wx *wx)
 
 	wx_cache_ring_rss(wx);
 
+	set_bit(WX_STATE_DOWN, wx->state);
+
 	return 0;
 }
 EXPORT_SYMBOL(wx_init_interrupt_scheme);
@@ -3314,7 +3316,8 @@ EXPORT_SYMBOL(wx_set_ring);
 
 void wx_service_event_schedule(struct wx *wx)
 {
-	if (!test_and_set_bit(WX_STATE_SERVICE_SCHED, wx->state))
+	if (!test_and_set_bit(WX_STATE_SERVICE_SCHED, wx->state) &&
+	    !test_bit(WX_STATE_DOWN, wx->state))
 		queue_work(system_power_efficient_wq, &wx->service_task);
 }
 EXPORT_SYMBOL(wx_service_event_schedule);
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_sriov.c b/drivers/net/ethernet/wangxun/libwx/wx_sriov.c
index a360b06a086a..0152004a2dd3 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_sriov.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_sriov.c
@@ -898,7 +898,7 @@ static void wx_set_vf_link_state(struct wx *wx, int vf, int state)
 	wx->vfinfo[vf].link_state = state;
 	switch (state) {
 	case IFLA_VF_LINK_STATE_AUTO:
-		if (netif_running(wx->netdev))
+		if (!test_bit(WX_STATE_DOWN, wx->state))
 			wx->vfinfo[vf].link_enable = true;
 		else
 			wx->vfinfo[vf].link_enable = false;
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
index 0da5565ee4ff..c7befe4cdfe9 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
@@ -1202,6 +1202,7 @@ struct wx_last_stats {
 };
 
 enum wx_state {
+	WX_STATE_DOWN,
 	WX_STATE_RESETTING,
 	WX_STATE_SWFW_BUSY,
 	WX_STATE_PTP_RUNNING,
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
index 94ff8f5f0b4c..0d2db8d38cd5 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
@@ -68,7 +68,7 @@ static irqreturn_t wx_msix_misc_vf(int __always_unused irq, void *data)
 
 	set_bit(WX_FLAG_NEED_UPDATE_LINK, wx->flags);
 	/* Clear the interrupt */
-	if (netif_running(wx->netdev))
+	if (!test_bit(WX_STATE_DOWN, wx->state))
 		wr32(wx, WX_VXIMC, wx->eims_other);
 
 	return IRQ_HANDLED;
@@ -278,6 +278,7 @@ static void wxvf_up_complete(struct wx *wx)
 
 	wx_configure_msix_vf(wx);
 	smp_mb__before_atomic();
+	clear_bit(WX_STATE_DOWN, wx->state);
 	wx_napi_enable_all(wx);
 
 	/* clear any pending interrupts, may auto mask */
@@ -327,6 +328,9 @@ static void wxvf_down(struct wx *wx)
 {
 	struct net_device *netdev = wx->netdev;
 
+	if (test_and_set_bit(WX_STATE_DOWN, wx->state))
+		return;
+
 	timer_delete_sync(&wx->service_timer);
 	netif_tx_stop_all_queues(netdev);
 	netif_tx_disable(netdev);
@@ -360,7 +364,7 @@ static void wxvf_reset_subtask(struct wx *wx)
 
 	rtnl_lock();
 	if (test_bit(WX_STATE_RESETTING, wx->state) ||
-	    !(netif_running(wx->netdev))) {
+	    test_bit(WX_STATE_DOWN, wx->state)) {
 		rtnl_unlock();
 		return;
 	}
diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
index d51d8db95a76..f4a2dd6fa493 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
@@ -204,7 +204,7 @@ static irqreturn_t ngbe_intr(int __always_unused irq, void *data)
 		/* shared interrupt alert!
 		 * the interrupt that we masked before the EICR read.
 		 */
-		if (netif_running(wx->netdev))
+		if (!test_bit(WX_STATE_DOWN, wx->state))
 			ngbe_irq_enable(wx, true);
 		return IRQ_NONE;        /* Not our interrupt */
 	}
@@ -220,7 +220,7 @@ static irqreturn_t ngbe_intr(int __always_unused irq, void *data)
 	/* would disable interrupts here but it is auto disabled */
 	napi_schedule_irqoff(&q_vector->napi);
 
-	if (netif_running(wx->netdev))
+	if (!test_bit(WX_STATE_DOWN, wx->state))
 		ngbe_irq_enable(wx, false);
 
 	return IRQ_HANDLED;
@@ -235,7 +235,7 @@ static irqreturn_t __ngbe_msix_misc(struct wx *wx, u32 eicr)
 		wx_ptp_check_pps_event(wx);
 
 	/* re-enable the original interrupt state, no lsc, no queues */
-	if (netif_running(wx->netdev))
+	if (!test_bit(WX_STATE_DOWN, wx->state))
 		ngbe_irq_enable(wx, false);
 
 	return IRQ_HANDLED;
@@ -262,7 +262,7 @@ static irqreturn_t ngbe_misc_and_queue(int __always_unused irq, void *data)
 		/* queue */
 		q_vector = wx->q_vector[0];
 		napi_schedule_irqoff(&q_vector->napi);
-		if (netif_running(wx->netdev))
+		if (!test_bit(WX_STATE_DOWN, wx->state))
 			ngbe_irq_enable(wx, true);
 		return IRQ_HANDLED;
 	}
@@ -363,6 +363,9 @@ static void ngbe_disable_device(struct wx *wx)
 	struct net_device *netdev = wx->netdev;
 	u32 i;
 
+	if (test_and_set_bit(WX_STATE_DOWN, wx->state))
+		return;
+
 	if (wx->num_vfs) {
 		/* Clear EITR Select mapping */
 		wr32(wx, WX_PX_ITRSEL, 0);
@@ -428,6 +431,7 @@ void ngbe_up(struct wx *wx)
 
 	/* make sure to complete pre-operations */
 	smp_mb__before_atomic();
+	clear_bit(WX_STATE_DOWN, wx->state);
 	wx_napi_enable_all(wx);
 	/* enable transmits */
 	netif_tx_start_all_queues(wx->netdev);
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c
index aa14958d439a..8746318ad3bc 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c
@@ -141,7 +141,7 @@ static irqreturn_t txgbe_misc_irq_handle(int irq, void *data)
 		/* shared interrupt alert!
 		 * the interrupt that we masked before the ICR read.
 		 */
-		if (netif_running(wx->netdev))
+		if (!test_bit(WX_STATE_DOWN, wx->state))
 			txgbe_irq_enable(wx, true);
 		return IRQ_NONE;        /* Not our interrupt */
 	}
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index 4c549c2644ab..f9cd1caaf0a4 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -151,6 +151,7 @@ static void txgbe_up_complete(struct wx *wx)
 
 	/* make sure to complete pre-operations */
 	smp_mb__before_atomic();
+	clear_bit(WX_STATE_DOWN, wx->state);
 	wx_napi_enable_all(wx);
 
 	switch (wx->mac.type) {
@@ -213,6 +214,9 @@ static void txgbe_disable_device(struct wx *wx)
 	struct net_device *netdev = wx->netdev;
 	u32 i;
 
+	if (test_and_set_bit(WX_STATE_DOWN, wx->state))
+		return;
+
 	wx_disable_pcie_master(wx);
 	/* disable receives */
 	wx_disable_rx(wx);
-- 
2.51.0


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

* Re: [PATCH net-next v1 1/3] net: wangxun: introduce WX_STATE_DOWN to serialize device shutdown state
  2026-05-19  8:00 ` [PATCH net-next v1 1/3] net: wangxun: introduce WX_STATE_DOWN to serialize device shutdown state Jiawen Wu
@ 2026-05-22  7:34   ` Simon Horman
  0 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2026-05-22  7:34 UTC (permalink / raw)
  To: jiawenwu
  Cc: 'Simon Horman', netdev, mengyuanlou, andrew+netdev, davem,
	edumazet, kuba, pabeni, kees, larysa.zaremba, jacob.e.keller

From: 'Simon Horman' <horms@kernel.org>

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
---
net: wangxun: introduce WX_STATE_DOWN to serialize device shutdown state

Replace various netif_running() checks with an explicit WX_STATE_DOWN
state bit to track whether the device datapath and interrupt handling
are operational.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index 746623fa59b40..69fe197376796 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
[ ... ]
> @@ -3314,7 +3316,8 @@ EXPORT_SYMBOL(wx_set_ring);
>  
>  void wx_service_event_schedule(struct wx *wx)
>  {
> -	if (!test_and_set_bit(WX_STATE_SERVICE_SCHED, wx->state))
> +	if (!test_and_set_bit(WX_STATE_SERVICE_SCHED, wx->state) &&
> +	    !test_bit(WX_STATE_DOWN, wx->state))
>  		queue_work(system_power_efficient_wq, &wx->service_task);
>  }

Can this leak the WX_STATE_SERVICE_SCHED bit when the device is down?

If WX_STATE_DOWN is set, test_and_set_bit() executes first and sets
WX_STATE_SERVICE_SCHED, but the second condition evaluates to false.
This skips queue_work(), meaning the service task is never queued.

Because the task does not execute, WX_STATE_SERVICE_SCHED is never cleared.
When the device is brought back up, subsequent calls to
wx_service_event_schedule() might always fail the test_and_set_bit() check,
permanently disabling the service task.

Should the conditions be reversed to check WX_STATE_DOWN before setting the
service schedule bit?

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

* [PATCH net-next v1 2/3] net: wangxun: avoid statistics updates during device teardown
  2026-05-19  8:00 [PATCH net-next v1 0/3] wangxun: improve service task synchronization Jiawen Wu
  2026-05-19  8:00 ` [PATCH net-next v1 1/3] net: wangxun: introduce WX_STATE_DOWN to serialize device shutdown state Jiawen Wu
@ 2026-05-19  8:00 ` Jiawen Wu
  2026-05-19  8:00 ` [PATCH net-next v1 3/3] net: txgbe: rework service event handling Jiawen Wu
  2 siblings, 0 replies; 6+ messages in thread
From: Jiawen Wu @ 2026-05-19  8:00 UTC (permalink / raw)
  To: netdev
  Cc: Mengyuan Lou, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman, Kees Cook,
	Larysa Zaremba, Jacob Keller, Jiawen Wu

After introducing WX_STATE_DOWN, wx_update_stats() now explicitly skips
statistics collection while the device is in teardown or reset state.
Calling wx_update_stats() from the device disable path therefore becomes
redundant.

Remove wx_update_stats() calls from ngbe_disable_device() and
txgbe_disable_device().

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/ngbe/ngbe_main.c   | 2 --
 drivers/net/ethernet/wangxun/txgbe/txgbe_main.c | 2 --
 2 files changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
index f4a2dd6fa493..8678c49b892a 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
@@ -404,8 +404,6 @@ static void ngbe_disable_device(struct wx *wx)
 
 		wr32(wx, WX_PX_TR_CFG(reg_idx), WX_PX_TR_CFG_SWFLSH);
 	}
-
-	wx_update_stats(wx);
 }
 
 static void ngbe_reset(struct wx *wx)
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index f9cd1caaf0a4..6755464b4637 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -265,8 +265,6 @@ static void txgbe_disable_device(struct wx *wx)
 
 	/* Disable the Tx DMA engine */
 	wr32m(wx, WX_TDM_CTL, WX_TDM_CTL_TE, 0);
-
-	wx_update_stats(wx);
 }
 
 void txgbe_down(struct wx *wx)
-- 
2.51.0


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

* [PATCH net-next v1 3/3] net: txgbe: rework service event handling
  2026-05-19  8:00 [PATCH net-next v1 0/3] wangxun: improve service task synchronization Jiawen Wu
  2026-05-19  8:00 ` [PATCH net-next v1 1/3] net: wangxun: introduce WX_STATE_DOWN to serialize device shutdown state Jiawen Wu
  2026-05-19  8:00 ` [PATCH net-next v1 2/3] net: wangxun: avoid statistics updates during device teardown Jiawen Wu
@ 2026-05-19  8:00 ` Jiawen Wu
  2026-05-22  7:34   ` Simon Horman
  2 siblings, 1 reply; 6+ messages in thread
From: Jiawen Wu @ 2026-05-19  8:00 UTC (permalink / raw)
  To: netdev
  Cc: Mengyuan Lou, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman, Kees Cook,
	Larysa Zaremba, Jacob Keller, Jiawen Wu

Convert to use test_and_clear_bit() for link event subtasks. Only re-arm
the WX_FLAG_NEED_MODULE_RESET flag when module is absent. Unsupported or
invalid modules no longer cause the service task to continuously retry
module identification.

Additionally, explicitly cancel service_task during device teardown to
ensure no pending asynchronous service work survives after the device
has entered the DOWN state.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 .../net/ethernet/wangxun/txgbe/txgbe_aml.c    | 10 +++-------
 .../net/ethernet/wangxun/txgbe/txgbe_aml.h    |  2 +-
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   | 19 ++++++-------------
 3 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
index f0514251d4f3..da1d3976ed33 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
@@ -186,19 +186,15 @@ static void txgbe_get_mac_link(struct wx *wx, int *speed)
 		*speed = SPEED_UNKNOWN;
 }
 
-int txgbe_set_phy_link(struct wx *wx)
+void txgbe_set_phy_link(struct wx *wx)
 {
 	int speed, autoneg, duplex, err;
 
 	txgbe_get_link_capabilities(wx, &speed, &autoneg, &duplex);
 
 	err = txgbe_set_phy_link_hostif(wx, speed, autoneg, duplex);
-	if (err) {
+	if (err)
 		wx_err(wx, "Failed to setup link\n");
-		return err;
-	}
-
-	return 0;
 }
 
 static int txgbe_sfp_to_linkmodes(struct wx *wx, struct txgbe_sff_id *id)
@@ -362,7 +358,7 @@ int txgbe_identify_module(struct wx *wx)
 	    id->identifier != TXGBE_SFF_IDENTIFIER_QSFP_PLUS &&
 	    id->identifier != TXGBE_SFF_IDENTIFIER_QSFP28) {
 		wx_err(wx, "Invalid module\n");
-		return -ENODEV;
+		return -EINVAL;
 	}
 
 	if (id->transceiver_type == 0xFF)
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
index 4f6df0ee860b..379c74ad19c6 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
@@ -10,7 +10,7 @@ int txgbe_test_hostif(struct wx *wx);
 int txgbe_read_eeprom_hostif(struct wx *wx,
 			     struct txgbe_hic_i2c_read *buffer,
 			     u32 length, u8 *data);
-int txgbe_set_phy_link(struct wx *wx);
+void txgbe_set_phy_link(struct wx *wx);
 int txgbe_identify_module(struct wx *wx);
 void txgbe_setup_link(struct wx *wx);
 int txgbe_phylink_init_aml(struct txgbe *txgbe);
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index 6755464b4637..ce82e13aa8ae 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -93,31 +93,23 @@ static void txgbe_module_detection_subtask(struct wx *wx)
 {
 	int err;
 
-	if (!test_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags))
+	if (!test_and_clear_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags))
 		return;
 
 	/* wait for SFF module ready */
 	msleep(200);
 
 	err = txgbe_identify_module(wx);
-	if (err)
-		return;
-
-	clear_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags);
+	if (err == -ENODEV)
+		set_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags);
 }
 
 static void txgbe_link_config_subtask(struct wx *wx)
 {
-	int err;
-
-	if (!test_bit(WX_FLAG_NEED_LINK_CONFIG, wx->flags))
+	if (!test_and_clear_bit(WX_FLAG_NEED_LINK_CONFIG, wx->flags))
 		return;
 
-	err = txgbe_set_phy_link(wx);
-	if (err)
-		return;
-
-	clear_bit(WX_FLAG_NEED_LINK_CONFIG, wx->flags);
+	txgbe_set_phy_link(wx);
 }
 
 /**
@@ -233,6 +225,7 @@ static void txgbe_disable_device(struct wx *wx)
 	wx_napi_disable_all(wx);
 
 	timer_delete_sync(&wx->service_timer);
+	cancel_work_sync(&wx->service_task);
 
 	if (wx->bus.func < 2)
 		wr32m(wx, TXGBE_MIS_PRB_CTL, TXGBE_MIS_PRB_CTL_LAN_UP(wx->bus.func), 0);
-- 
2.51.0


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

* Re: [PATCH net-next v1 3/3] net: txgbe: rework service event handling
  2026-05-19  8:00 ` [PATCH net-next v1 3/3] net: txgbe: rework service event handling Jiawen Wu
@ 2026-05-22  7:34   ` Simon Horman
  0 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2026-05-22  7:34 UTC (permalink / raw)
  To: jiawenwu
  Cc: 'Simon Horman', netdev, mengyuanlou, andrew+netdev, davem,
	edumazet, kuba, pabeni, kees, larysa.zaremba, jacob.e.keller

From: 'Simon Horman' <horms@kernel.org>

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
---
net: txgbe: rework service event handling

This commit converts the link event subtasks to use test_and_clear_bit()
and changes how module detection retries are handled. It also explicitly
cancels the service task during device teardown.

> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index 6755464b46371..ce82e13aa8ae0 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> @@ -93,31 +93,23 @@ static void txgbe_module_detection_subtask(struct wx *wx)
>  {
>  	int err;
>  
> -	if (!test_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags))
> +	if (!test_and_clear_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags))
>  		return;
>  
>  	/* wait for SFF module ready */
>  	msleep(200);
>  
>  	err = txgbe_identify_module(wx);
> -	if (err)
> -		return;
> -
> -	clear_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags);
> +	if (err == -ENODEV)
> +		set_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags);
>  }

Does this change prevent the driver from retrying transient firmware errors?

Since txgbe_identify_module() communicates with firmware via
txgbe_identify_module_hostif(), it can fail with transient errors like
-EIO or -ETIMEDOUT.

Before this patch, the WX_FLAG_NEED_MODULE_RESET flag was only cleared upon
successful completion, ensuring the periodic timer would retry on any error.

Because the flag is now cleared immediately and only re-armed on -ENODEV,
could a single transient timeout cause the driver to permanently abandon
module setup?

>  static void txgbe_link_config_subtask(struct wx *wx)
>  {
> -	int err;
> -
> -	if (!test_bit(WX_FLAG_NEED_LINK_CONFIG, wx->flags))
> +	if (!test_and_clear_bit(WX_FLAG_NEED_LINK_CONFIG, wx->flags))
>  		return;
>  
> -	err = txgbe_set_phy_link(wx);
> -	if (err)
> -		return;
> -
> -	clear_bit(WX_FLAG_NEED_LINK_CONFIG, wx->flags);
> +	txgbe_set_phy_link(wx);
>  }

Similarly, does this silently drop link configuration on failure?

Without a way to re-arm WX_FLAG_NEED_LINK_CONFIG, any transient failure
in txgbe_set_phy_link() will result in the driver no longer retrying link
configuration.

[ ... ]

> @@ -233,6 +225,7 @@ static void txgbe_disable_device(struct wx *wx)
>  	wx_napi_disable_all(wx);
>  
>  	timer_delete_sync(&wx->service_timer);
> +	cancel_work_sync(&wx->service_task);
>  
>  	if (wx->bus.func < 2)
>  		wr32m(wx, TXGBE_MIS_PRB_CTL, TXGBE_MIS_PRB_CTL_LAN_UP(wx->bus.func), 0);

Will canceling the service_task here leave the WX_STATE_SERVICE_SCHED bit
permanently set if the work is pending?

The wx_service_event_schedule() function sets WX_STATE_SERVICE_SCHED before
queuing the task, and this bit is normally cleared exclusively at the end
of the work function.

If cancel_work_sync() dequeues the work without running it, the bit is never
cleared. When the device is later brought back up, attempts to schedule the
service task will fail, which seems like it would permanently break module
detection, link configuration, and stats collection until the driver is
reloaded.

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

end of thread, other threads:[~2026-05-22  7:35 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-19  8:00 [PATCH net-next v1 0/3] wangxun: improve service task synchronization Jiawen Wu
2026-05-19  8:00 ` [PATCH net-next v1 1/3] net: wangxun: introduce WX_STATE_DOWN to serialize device shutdown state Jiawen Wu
2026-05-22  7:34   ` Simon Horman
2026-05-19  8:00 ` [PATCH net-next v1 2/3] net: wangxun: avoid statistics updates during device teardown Jiawen Wu
2026-05-19  8:00 ` [PATCH net-next v1 3/3] net: txgbe: rework service event handling Jiawen Wu
2026-05-22  7:34   ` Simon Horman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox